Quote Win32 arguments. - Githubissues

theory commented 6 years ago

Taking a stab at addressing an issue reported in #9, #14, #22 and #24. I think using Win32::ShellQuote is likely to be the correct approach, but somehow I haven't quite got the syntax down. Perhaps Jan Dubois could point me in the right direction. @jandubois: Any pointers?

theory commented 6 years ago

I think I've fixed it in 2727f07, just a silly oversight on my part. Sorry for the noise, @jandubois. @pjf: The new tests properly pass on Windows. \o/. Not sure if it covers the cases reported in the other tickets, but I think it's on the right track, at least.

theory commented 6 years ago

Probably ought to supersede #13.

theory commented 6 years ago

2b36a43 fixes capture test failures @haarg pointed out on IRC, but now I get test failures from t\win32.t:

  '"exit"' is not recognized as an internal or external command,
  operable program or batch file.

  #   Failed test 'Running with 42 ok'
  #   at t\win32.t line 52.
  #          got: '"cmd.exe" unexpectedly returned exit value 1 at t\win32.t line 49.
  # '
  #     expected: ''

  #   Failed test '42 exit value'
  #   at t\win32.t line 53.
  #          got: undef
  #     expected: '42'
  '"exit"' is not recognized as an internal or external command,
  operable program or batch file.

  #   Failed test 'Capturing with 42 ok'
  #   at t\win32.t line 61.
  #          got: '"cmd.exe" unexpectedly returned exit value 1 at t\win32.t line 58.
  # '
  #     expected: ''

  #   Failed test 'Capture ok with 42 exit value'
  #   at t\win32.t line 62.
  #          got: '1'
  #     expected: '42'
  '"exit"' is not recognized as an internal or external command,
  operable program or batch file.

  #   Failed test 'Running with 1000 ok'
  #   at t\win32.t line 52.
  #          got: '"cmd.exe" unexpectedly returned exit value 1 at t\win32.t line 49.
  # '
  #     expected: ''

  #   Failed test '1000 exit value'
  #   at t\win32.t line 53.
  #          got: undef
  #     expected: '1000'
  '"exit"' is not recognized as an internal or external command,
  operable program or batch file.

  #   Failed test 'Capturing with 1000 ok'
  #   at t\win32.t line 61.
  #          got: '"cmd.exe" unexpectedly returned exit value 1 at t\win32.t line 58.
  # '
  #     expected: ''

  #   Failed test 'Capture ok with 1000 exit value'
  #   at t\win32.t line 62.
  #          got: '1'
  #     expected: '1000'
  '"exit"' is not recognized as an internal or external command,
  operable program or batch file.

  #   Failed test 'Running with 100000 ok'
  #   at t\win32.t line 52.
  #          got: '"cmd.exe" unexpectedly returned exit value 1 at t\win32.t line 49.
  # '
  #     expected: ''

  #   Failed test '100000 exit value'
  #   at t\win32.t line 53.
  #          got: undef
  #     expected: '100000'
  '"exit"' is not recognized as an internal or external command,
  operable program or batch file.

  #   Failed test 'Capturing with 100000 ok'
  #   at t\win32.t line 61.
  #          got: '"cmd.exe" unexpectedly returned exit value 1 at t\win32.t line 58.
  # '
  #     expected: ''

  #   Failed test 'Capture ok with 100000 exit value'
  #   at t\win32.t line 62.
  #          got: '1'
  #     expected: '100000'

  #   Failed test 'RT \#48319 - Check for STDOUT replumbing'
  #   at t\win32.t line 129.
  #          got: ''
  #     expected: '12'
  # Looks like you failed 13 tests of 33.
  t\win32.t .. 
  Dubious, test returned 13 (wstat 3328, 0xd00)
  Failed 13/33 subtests 
    (less 4 skipped subtests: 16 okay)

  Test Summary Report
  -------------------
  t\win32.t (Wstat: 3328 Tests: 33 Failed: 13)
    Failed tests:  2-13, 29
    Non-zero exit status: 13
  Files=1, Tests=33,  6 wallclock secs ( 0.04 usr +  0.02 sys =  0.06 CPU)
  Result: FAIL

I suspect the issue may be misquoting args to cmd.exe. :-(

theory commented 6 years ago

Well, I thought 83b7720 was the last failure, and it was the last in t\win32.t, but e46af29 broke a test in t\args.t:

Argument ">" isn't numeric in exit at output.pl line 9.

#   Failed test 'Should have redirected text run'
#   at t\args.t line 77.
#          got: 'foo
# bar baz
# '
#     expected: 'Hello
# Goodbye
# '
Argument ">" isn't numeric in exit at output.pl line 9.

#   Failed test 'Should have redirected text systemx'
#   at t\args.t line 82.
#          got: 'foo
# bar baz
# '
#     expected: 'Hello
# Goodbye
# '
# Looks like you failed 2 tests of 56.
t\args.t .. 
Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/56 subtests 

Test Summary Report
-------------------
t\args.t (Wstat: 512 Tests: 56 Failed: 2)
  Failed tests:  51, 54
  Non-zero exit status: 2
Files=1, Tests=56, 15 wallclock secs ( 0.04 usr +  0.03 sys =  0.07 CPU)
Result: FAIL

This appears to be due to my changing run() to use a subprocess instead of CORE::system in e46af29. I don't know how to get it to understand that '>' is a shell character and not a string.

theory commented 6 years ago

Looks as though redirection (>, <, |) are not supported by CreateProcess. Perl's system command must magically handle that stuff somehow. So we can either:

Figure out what CORE::system() does for shell character parsing on Windows, and dupe it; or
Give up on handling exit codes that don't fit into 8 bits.

theory commented 6 years ago

Well, I did all that work to get all the tests passing, bug then noticed this note in the docs:

As of C<IPC::System::Simple> v0.06, the C<run> subroutine I<when
called with multiple arguments> will make available the full 32-bit
exit value on Win32 systems.  This is different from the
previous versions of C<IPC::System::Simple> and from Perl's
in-build C<system()> function, which can only handle 8-bit return values.

I've made it so that it now returns 32-bit values from run even for the single-argument form. However, it explicitly relies on cmd.exe (or command.com) to do so. If that's not desirable, we can revert back to 2b36a43 and try to figure out some other way to get t\win32.t to test exit codes greater than 8 bits. The trouble is, the introduction of shell quoting means that cmd.exe /x/d/c doesn't work, because it gets quoted as "cmd.exe" "/x/d/c" which isn't valid! My assumption is that almost no one uses IPC::System::Simple to call cmd.exe (or command.com), but if anyone does, it is a breaking change for them. THat's exactly what this line does in t\win32.t:

    $exit = run([$big_exitval], @{&EXIT_CMD}, $big_exitval);

If there is some other way to make an app exit with a large exit code with multiple-arg calls to run(), I don't know what it is. $^X, '-e', "exit $big_exitval works for 42 and 1000, but not 100000. :-(

theory commented 6 years ago

Okay, everything is consistent now, except that one can no longer use the multiple-argument forms or x variants to execute cmd.exe or command.com. Otherwise, I think it works a lot more like one would expect now!

theory commented 5 years ago

Hi, any chance of an release soonish with this fix? I have Windows users still suffering from the lack of shell quoting. :-( Thank you!

theory commented 5 years ago

Happy to report that a Windows Sqitch user used this patch and it worked! Any chance of a code review and possibility of a merge and release? Thank you!

jkeenan commented 4 years ago

@theory, would you be able to rebase this pull request on IPC-System-Simple master (9248ce9cdd38172cc94a66c8c460589ae5d5acf8) and submit as a new pull request?

Background to this request:

Last year @pjf granted me commit bits to the CPAN and github.com repositories for this distribution. I used that to clear up bugs (including some for which I was the reporter) on *nix platforms. Almost all the remaining issues (https://github.com/pjf/ipc-system-simple/issues) are Windows-related -- but I have neither access to Windows nor sufficient experience to fully address those issues.

Recently there has been renewed attention to IPC-System-Simple's problems on Windows in https://github.com/pjf/ipc-system-simple/issues/34, https://github.com/pjf/ipc-system-simple/issues/32, https://github.com/pjf/ipc-system-simple/pull/35 and other issues and pull requests. @dylanstreb; @PhilterPaper. Because of my own limitations (above) I'm not in a position to make a final decision.

The substance of your p.r. looks reasonable and I would like, at the very least, to create a branch for it in the main github repository so that other people could try it out. But since the p.r. is a couple of years old it doesn't apply cleanly and, in any event, I'd like to get one single patch that can apply cleanly on top of the HEAD of master.

Is that feasible?

Thank you very much. Jim Keenan

theory commented 4 years ago

Rebased.

jkeenan commented 4 years ago

@theory, thanks for the rebase. I have created a branch theory:win32quote and pushed it to the repository. I have also asked people who have filed other Win32-related issues to test it.

In my own repository I added an AppVeyor configuration file and activated a build. However, the build failed quickly. https://ci.appveyor.com/project/jkeenan/ipc-system-simple AppVeyor installed Strawberry Perl and cpanm, but cpanm was then unable to configure IPC-System-Simple. The log file terminated with this:

[00:01:02] cpanm --installdeps .
[00:01:03] --> Working on .
[00:01:03] Configuring C:/projects/ipc-system-simple ... N/A
[00:01:03] ! Configuring . failed. See C:\Users\appveyor\.cpanm\work\1584617438.2928\build.log for details.
[00:01:03] Command exited with code 1

But, needless to say, I don't have access to the cpanm build.log file. Do you have any clue as to which this might happen?

Thank you very much. Jim Keenan

theory commented 4 years ago

Does it process dist.ini and therefore know it needs to install Win32::ShellQuote? If not, on Windows you might have to install it with cpanm before you install and test IPC::System::Simple itself.

jkeenan commented 4 years ago

Does it process dist.ini and therefore know it needs to install Win32::ShellQuote? If not, on Windows you might have to install it with cpanm before you install and test IPC::System::Simple itself.

The AppVeyor log doesn't provide any information on that. However, the Travis logs for Linux build clearly indicate that Dist::Zilla is being used. So my guess is that you're correct is saying we need Win32::ShellQuote. Do you know how to indicate that in dist.ini?

Thank you very much. Jim Keenan

jkeenan commented 4 years ago

Does it process dist.ini and therefore know it needs to install Win32::ShellQuote? If not, on Windows you might have to install it with cpanm before you install and test IPC::System::Simple itself.

The AppVeyor log doesn't provide any information on that. However, the Travis logs for Linux build clearly indicate that Dist::Zilla is being used. So my guess is that you're correct is saying we need Win32::ShellQuote. Do you know how to indicate that in dist.ini?

Thank you very much. Jim Keenan

Actually, dist.ini already mentions Win32::ShellQuote.

[OSPrereqs / MSWin32]
Win32::ShellQuote = 0

But I have no experience with modifying dist.ini; patches welcome.

Thank you very much. Jim Keenan

theory commented 4 years ago

I'm saying that if it's using cpanm, it probably is not picking up the dependency from dist.ini, because cpanm does not, to the best of my knowledge, run dzil.

theory commented 4 years ago

I suggest you replace line 15 with

  - cpanm --verbose Win32::ShellQuote
  - cpanm --verbose --installdeps .

jkeenan commented 4 years ago

I suggest you replace line 15 with

  - cpanm --verbose Win32::ShellQuote
  - cpanm --verbose --installdeps .

Unfortunately, that made no difference. From the log file for run 1.0.5 at https://ci.appveyor.com/project/jkeenan/ipc-system-simple:

[00:00:48] cpanm --verbose Win32::ShellQuote
[00:00:48] cpanm (App::cpanminus) 1.7044 on perl 5.030002 built for MSWin32-x64-multi-thread
[00:00:48] Work directory is C:\Users\appveyor/.cpanm/work/1584817491.1608
[00:00:48] You have make C:\strawberry\c\bin\gmake.exe
[00:00:48] You have LWP 6.43
[00:00:48] You have "C:\Program Files\Git\usr\bin\tar.exe", "C:\Program Files\Git\usr\bin\gzip.exe" and "C:\Program Files\Git\usr\bin\bzip2.exe"
[00:00:48] You have "C:\Program Files\Git\usr\bin\unzip.exe"
[00:00:48] Searching Win32::ShellQuote () on cpanmetadb ...
[00:00:48] Win32::ShellQuote is up to date. (0.003001)
[00:00:48] cpanm --verbose --installdeps .
[00:00:48] cpanm (App::cpanminus) 1.7044 on perl 5.030002 built for MSWin32-x64-multi-thread
[00:00:48] Work directory is C:\Users\appveyor/.cpanm/work/1584817492.2552
[00:00:48] You have make C:\strawberry\c\bin\gmake.exe
[00:00:48] You have LWP 6.43
[00:00:48] You have "C:\Program Files\Git\usr\bin\tar.exe", "C:\Program Files\Git\usr\bin\gzip.exe" and "C:\Program Files\Git\usr\bin\bzip2.exe"
[00:00:48] You have "C:\Program Files\Git\usr\bin\unzip.exe"
[00:00:48] Entering C:/projects/ipc-system-simple
[00:00:48] --> Working on .
[00:00:48] Configuring C:/projects/ipc-system-simple ... N/A
[00:00:48] ! Configuring . failed. See C:\Users\appveyor\.cpanm\work\1584817492.2552\build.log for details.
[00:00:48] Command exited with code 1

theory commented 4 years ago

SIGH Sadly, cpanminus does not appear to have a way to send the log output to STDERR or STDOUT. Try something like

- cpanm --verbose --installdeps . || cat C:\Users\appveyor\.cpanm\work\*\build.log

Or something like that. I don't know the DOS file globbing syntax, and the backslashes might need to be escaped.

Looking at how I dealt with testing Sqitch on Windows, I gave up on using cpanm to install local dependencies, because there is no cpanfile. Instead, I use cpanm to manually install all dependencies (actually I install the existing release of Sqitch, and if there are new modules I add them to be installed, too, until a new release has them specified — as is the case for Win32::ShellQuote here), and then just run prove — or rather, a custom prove, though I have no idea why I don't just call the strawberry-installed prove.

Testing on Windows is a PITA. Worth it, though.

theory commented 4 years ago

So maybe

  - cpanm --no-interactive --no-man-pages --notest --installdeps IPC::System::Simple Win32::ShellQuote
  - perl -MApp::Prove -e "my \$a = App::Prove->new;; (@ARGV); \$a->process_args(@ARGV); exit( \$app->run ? 0 : 1 );"

But I'm kind of guessing at the dollar escapes here. Avoiding those might be why I just added a simple prove script to the repo. But maybe you can just use prove on appveyor?

theory commented 4 years ago

Using GitHub workflows, it looks like I rely on the system prove, so maybe no need for the special script?

jkeenan commented 4 years ago

Using GitHub workflows, it looks like I rely on the system prove, so maybe no need for the special script?

I'm trying the KISS approach ... and I got a PASS! See: https://ci.appveyor.com/project/jkeenan/ipc-system-simple/builds/31624928

Thank you very much. Jim Keenan

jkeenan commented 4 years ago

After junking dist.ini and going back to a simple Makefile.PL, I got PASSes on both Travis and AppVeyor. I manually merged the branch. I've now released IPC-System-Simple-1.27-TRIAL to CPAN. Will now monitor CPANtesters.

@theory, thanks very much for your p.r. and subsequent advice.

theory commented 4 years ago

Glad to finally see this getting out there. Thank you!

pjf / ipc-system-simple

Quote Win32 arguments. #29