search

cpan-authors / IPC-Run

https://metacpan.org/pod/IPC::Run
Other
21 stars 38 forks source link

Support Win32 commands having nonstandard command line parsing rules #148

Closed nmisch closed 2 years ago

nmisch commented 3 years ago

This fixes a regression affecting Win32 batch files. The regression originated in my https://github.com/toddr/IPC-Run/pull/143. This implements the https://github.com/toddr/IPC-Run/pull/143#issuecomment-851718984 design.

The introduction of IPC::Run::Win32Process makes Win32::Process::Create() errors easier to reach, so I've included a commit to fix their reporting:

before: Bad file descriptor: Win32::Process::Create() at C:\... after: The system cannot find the file specified.: Win32::Process::Create() at C:\...

b) Otherwise, use some amount of quoting sufficient for programs using standard command line parsing rules and for Cygwin programs.

While this implementation handles Cygwin well for the tests in https://github.com/toddr/IPC-Run/pull/143#issuecomment-830974782, I found other cases that don't work. See the new comments in run.t. The Cygwin situation is no worse than it was in the last IPC::Run release, and I see no low-hanging fruit for improving it. I'm content with that.

wchristian commented 3 years ago

That looks hella impressive and comprehensive. Do the tests also make #147 superfluous? Agreed on the cygwin bit.

nmisch commented 3 years ago

On Wed, Jul 07, 2021 at 06:25:27PM -0700, Christian Walde (Mithaldu) wrote:

That looks hella impressive and comprehensive. Do the tests also make #147 superfluous? Agreed on the cygwin bit.

Thanks. #147 tests things that this doesn't test, like forward slashes and different kinds of relative paths. It's hard to predict what things might attract bugs in the future, so I can't swear that this makes #147 superfluous.

wchristian commented 3 years ago

Thanks for clarifying. That makes sense.

Fwiw, your changes seem to have fixed everything #147 gets at, so maybe it's best if @mohawk2 merges both. Happy to rebase if reordering is desired.

nmisch commented 2 years ago

Folks with repository write access: is the matter of reviewing this pull request still in your queue? (https://github.com/toddr/IPC-Run/pull/150 is another one ready to merge.)

nmisch commented 2 years ago

Upon merging this, run.t reached the 6h timeout instead of completing (https://github.com/toddr/IPC-Run/runs/7246813528?check_suite_focus=true). I've been unable to reproduce this, having tried six runs in GitHub Actions and 4700 runs in a loop locally (Windows Server 2022). If you notice it happening again, please let me know, especially if you see which line of the script gets the stall.