mtkennerly / shawl

Windows service wrapper for arbitrary commands
MIT License
495 stars 15 forks source link

Negative exit codes are not handled properly #24

Closed ilpropheta closed 2 years ago

ilpropheta commented 2 years ago

Hi, thanks for this awesome tool!

This is just for reporting a possible bug: it seems to me that shawl is handling negative exit codes properly. For example, this command line below:

shawl.exe add --pass -2 --name MyService -- Process.exe

results in:

error: Found argument '-2' which wasn't expected, or isn't valid in this context
...

I get the same for any arguments involving exit codes (e.g. --restart-if).

Am I missing something?

Thanks!

Marco

mtkennerly commented 2 years ago

Hi! This one is a bit odd. Internally, Windows treats all exit codes as unsigned 32-bit (source), but Command Prompt reinterprets them as signed. You can see it in this example:

>python -c "import sys; sys.exit(2_147_483_647)"
>echo %errorlevel%
2147483647

>python -c "import sys; sys.exit(2_147_483_648)"
>echo %errorlevel%
-1

So I think Shawl's behavior is technically correct, but not necessarily convenient. I'll look into supporting negative codes as a shortcut in add.

ilpropheta commented 2 years ago

Hi @mtkennerly, thanks for your reply! You are totally right, I haven't taken into account this subtlety and mistakenly thought that Shawl was using the prompt "convention".

I think Shawl is right too.

Just to add more value to this discussion, the options I see here:

  1. Shawl supports negative codes and silently turns them into unsigned (e.g. -2 turns to 4294967294);
  2. Shawl supports negative codes and warns the user that it is turning them into unsigned;
  3. Like 3 but only if an explicit flag is used (like --enable-negative-codes or such). This is like getting a sort of acknowledgment from the user ("I know what Shawl is doing");
  4. Shawl keeps on breaking on negative codes but it reports a better error message;
  5. (just for completeness) no action at all.

Probably I am missing some options but I hope these above can be useful for your decision!

mtkennerly commented 2 years ago

Actually, I'm looking into this now, and the root cause is different than I was thinking earlier. Shawl does support negative numbers, but not at the start of the value, because then it looks like a CLI short flag. For example, --pass 0,-1 is fine, but not --pass -1. The signed/unsigned subtlety I was thinking of is already handled elsewhere.

mtkennerly commented 2 years ago

This is now available in v1.1.1. Let me know if you run into any other issues around this.

ilpropheta commented 2 years ago

Sure, I'll let you know! Many thanks for your support.