[ovs-dev] checkpatch: Normalize exit code for Windows

Message ID 20190205150519.16016-1-aserdean@ovn.org
State Changes Requested
Headers show
Series
  • [ovs-dev] checkpatch: Normalize exit code for Windows
Related show

Commit Message

Alin Gabriel Serdean Feb. 5, 2019, 3:05 p.m.
Using python `sys.exit(-1)` on Windows produces mixed results.
Let's take the following results from different shells:
CMD
>python -c "import sys; sys.exit(-1)" & echo %errorlevel%
1
MSYS
$ python -c "import sys; sys.exit(-1)" && echo $?
0
WSL
$ python -c "import sys; sys.exit(-1)"; echo $?
255

this results in the following tests to fail:
checkpatch

 10: checkpatch - sign-offs                          FAILED (checkpatch.at:32)
 11: checkpatch - parenthesized constructs           FAILED (checkpatch.at:32)
 12: checkpatch - parenthesized constructs - for     FAILED (checkpatch.at:32)
 13: checkpatch - comments                           FAILED (checkpatch.at:32)

because of:
 ./checkpatch.at:32: exit code was 0, expected 255

This patch introduces a positive constant for the default exit code.

Signed-off-by: Alin Gabriel Serdean <aserdean@ovn.org>
---
 utilities/checkpatch.py | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Comments

Ben Pfaff Feb. 5, 2019, 4:32 p.m. | #1
On Tue, Feb 05, 2019 at 05:05:19PM +0200, Alin Gabriel Serdean wrote:
> Using python `sys.exit(-1)` on Windows produces mixed results.
> Let's take the following results from different shells:
> CMD
> >python -c "import sys; sys.exit(-1)" & echo %errorlevel%
> 1
> MSYS
> $ python -c "import sys; sys.exit(-1)" && echo $?
> 0
> WSL
> $ python -c "import sys; sys.exit(-1)"; echo $?
> 255
> 
> this results in the following tests to fail:
> checkpatch
> 
>  10: checkpatch - sign-offs                          FAILED (checkpatch.at:32)
>  11: checkpatch - parenthesized constructs           FAILED (checkpatch.at:32)
>  12: checkpatch - parenthesized constructs - for     FAILED (checkpatch.at:32)
>  13: checkpatch - comments                           FAILED (checkpatch.at:32)
> 
> because of:
>  ./checkpatch.at:32: exit code was 0, expected 255
> 
> This patch introduces a positive constant for the default exit code.
> 
> Signed-off-by: Alin Gabriel Serdean <aserdean@ovn.org>

Curious!  I did not know about this particular nonportable behavior.

Acked-by: Ben Pfaff <blp@ovn.org>
Aaron Conole Feb. 5, 2019, 8:39 p.m. | #2
Alin Gabriel Serdean <aserdean@ovn.org> writes:

> Using python `sys.exit(-1)` on Windows produces mixed results.
> Let's take the following results from different shells:
> CMD
>>python -c "import sys; sys.exit(-1)" & echo %errorlevel%
> 1
> MSYS
> $ python -c "import sys; sys.exit(-1)" && echo $?
> 0
> WSL
> $ python -c "import sys; sys.exit(-1)"; echo $?
> 255
>
> this results in the following tests to fail:
> checkpatch
>
>  10: checkpatch - sign-offs                          FAILED (checkpatch.at:32)
>  11: checkpatch - parenthesized constructs           FAILED (checkpatch.at:32)
>  12: checkpatch - parenthesized constructs - for     FAILED (checkpatch.at:32)
>  13: checkpatch - comments                           FAILED (checkpatch.at:32)
>
> because of:
>  ./checkpatch.at:32: exit code was 0, expected 255
>
> This patch introduces a positive constant for the default exit code.
>
> Signed-off-by: Alin Gabriel Serdean <aserdean@ovn.org>
> ---

Acked-by: Aaron Conole <aconole@redhat.com>

With one little fixup:

1. either we should eliminate the result argument to
   ovs_checkpatch_print_result

2. or the test for the result < 0 needs to change to if result:

Otherwise, when an error is introduced, the exit code will be
propagated, but the printout will be confusing:

  03:30:50 aconole {(0a8c1ec04...)} ~/git/ovs$ git commit -m "dpdk: This is a test
  Just testing
  "
  ERROR: C99 style comment
  #10 FILE: lib/dpdk.c:17:
  // BREAK!!

Lines checked: 15, no obvious problems found

:-)

>  utilities/checkpatch.py | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index 8eab31f60..8b305f2ad 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -24,6 +24,7 @@ import sys
>  RETURN_CHECK_INITIAL_STATE = 0
>  RETURN_CHECK_STATE_WITH_RETURN = 1
>  RETURN_CHECK_AWAITING_BRACE = 2
> +EXIT_CODE = 255
>  __errors = 0
>  __warnings = 0
>  empty_return_check_state = 0
> @@ -837,7 +838,7 @@ def ovs_checkpatch_parse(text, filename, author=None, committer=None):
>  
>      run_file_checks(text)
>      if __errors or __warnings:
> -        return -1
> +        return EXIT_CODE
>      return 0
>  
>  
> @@ -920,7 +921,7 @@ if __name__ == '__main__':
>                                         "quiet"])
>      except:
>          print("Unknown option encountered. Please rerun with -h for help.")
> -        sys.exit(-1)
> +        sys.exit(EXIT_CODE)
>  
>      for o, a in optlist:
>          if o in ("-h", "--help"):
> @@ -946,7 +947,7 @@ if __name__ == '__main__':
>              quiet = True
>          else:
>              print("Unknown option '%s'" % o)
> -            sys.exit(-1)
> +            sys.exit(EXIT_CODE)
>  
>      if sys.stdout.isatty():
>          colors = True
> @@ -974,13 +975,13 @@ Subject: %s
>              result = ovs_checkpatch_parse(patch, revision)
>              ovs_checkpatch_print_result(result)
>              if result:
> -                status = -1
> +                status = EXIT_CODE
>          sys.exit(status)
>  
>      if not args:
>          if sys.stdin.isatty():
>              usage()
> -            sys.exit(-1)
> +            sys.exit(EXIT_CODE)
>          result = ovs_checkpatch_parse(sys.stdin.read(), '-')
>          ovs_checkpatch_print_result(result)
>          sys.exit(result)
> @@ -991,5 +992,5 @@ Subject: %s
>              print('== Checking "%s" ==' % filename)
>          result = ovs_checkpatch_file(filename)
>          if result:
> -            status = -1
> +            status = EXIT_CODE
>      sys.exit(status)
Alin Gabriel Serdean Feb. 11, 2019, 4 p.m. | #3
> De la: ovs-dev-bounces@openvswitch.org <ovs-dev-
> bounces@openvswitch.org> În numele Aaron Conole
> Trimis: Tuesday, February 5, 2019 10:39 PM
> Către: Alin Gabriel Serdean <aserdean@ovn.org>
> Cc: dev@openvswitch.org
> Subiect: Re: [ovs-dev] [PATCH] checkpatch: Normalize exit code for Windows
> 
> Alin Gabriel Serdean <aserdean@ovn.org> writes:
> 
> > Using python `sys.exit(-1)` on Windows produces mixed results.
> > Let's take the following results from different shells:
> > CMD
> >>python -c "import sys; sys.exit(-1)" & echo %errorlevel%
> > 1
> > MSYS
> > $ python -c "import sys; sys.exit(-1)" && echo $?
> > 0
> > WSL
> > $ python -c "import sys; sys.exit(-1)"; echo $?
> > 255
> >
> > this results in the following tests to fail:
> > checkpatch
> >
> >  10: checkpatch - sign-offs                          FAILED
(checkpatch.at:32)
> >  11: checkpatch - parenthesized constructs           FAILED
(checkpatch.at:32)
> >  12: checkpatch - parenthesized constructs - for     FAILED
> (checkpatch.at:32)
> >  13: checkpatch - comments                           FAILED
(checkpatch.at:32)
> >
> > because of:
> >  ./checkpatch.at:32: exit code was 0, expected 255
> >
> > This patch introduces a positive constant for the default exit code.
> >
> > Signed-off-by: Alin Gabriel Serdean <aserdean@ovn.org>
> > ---
> 
> Acked-by: Aaron Conole <aconole@redhat.com>
> 
> With one little fixup:
> 
> 1. either we should eliminate the result argument to
>    ovs_checkpatch_print_result
> 
> 2. or the test for the result < 0 needs to change to if result:
> 
> Otherwise, when an error is introduced, the exit code will be propagated,
> but the printout will be confusing:
> 
>   03:30:50 aconole {(0a8c1ec04...)} ~/git/ovs$ git commit -m "dpdk: This
is a
> test
>   Just testing
>   "
>   ERROR: C99 style comment
>   #10 FILE: lib/dpdk.c:17:
>   // BREAK!!
> 
> Lines checked: 15, no obvious problems found
> 
> :-)
> 
[Alin Serdean] Thanks for the feedback. I sent an incremental:
https://patchwork.ozlabs.org/patch/1039912/ . Do you mind taking another
look?

Patch

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 8eab31f60..8b305f2ad 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -24,6 +24,7 @@  import sys
 RETURN_CHECK_INITIAL_STATE = 0
 RETURN_CHECK_STATE_WITH_RETURN = 1
 RETURN_CHECK_AWAITING_BRACE = 2
+EXIT_CODE = 255
 __errors = 0
 __warnings = 0
 empty_return_check_state = 0
@@ -837,7 +838,7 @@  def ovs_checkpatch_parse(text, filename, author=None, committer=None):
 
     run_file_checks(text)
     if __errors or __warnings:
-        return -1
+        return EXIT_CODE
     return 0
 
 
@@ -920,7 +921,7 @@  if __name__ == '__main__':
                                        "quiet"])
     except:
         print("Unknown option encountered. Please rerun with -h for help.")
-        sys.exit(-1)
+        sys.exit(EXIT_CODE)
 
     for o, a in optlist:
         if o in ("-h", "--help"):
@@ -946,7 +947,7 @@  if __name__ == '__main__':
             quiet = True
         else:
             print("Unknown option '%s'" % o)
-            sys.exit(-1)
+            sys.exit(EXIT_CODE)
 
     if sys.stdout.isatty():
         colors = True
@@ -974,13 +975,13 @@  Subject: %s
             result = ovs_checkpatch_parse(patch, revision)
             ovs_checkpatch_print_result(result)
             if result:
-                status = -1
+                status = EXIT_CODE
         sys.exit(status)
 
     if not args:
         if sys.stdin.isatty():
             usage()
-            sys.exit(-1)
+            sys.exit(EXIT_CODE)
         result = ovs_checkpatch_parse(sys.stdin.read(), '-')
         ovs_checkpatch_print_result(result)
         sys.exit(result)
@@ -991,5 +992,5 @@  Subject: %s
             print('== Checking "%s" ==' % filename)
         result = ovs_checkpatch_file(filename)
         if result:
-            status = -1
+            status = EXIT_CODE
     sys.exit(status)