Support Win32 commands having nonstandard command line parsing rules #148
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Trivial examples worked, but my commit fbd6d18 broke even those. Cover all known cases, subject to assumptions now described in documentation. As a surviving incompatibility compared to the last release, IPC::Run still adds quotation marks to arguments where quotation marks are optional. That is consequential in a batch file running "echo %*", for example. One can argue that it's a good thing, but it is an incompatibility. Discourage executing batch files with arguments, particularly when argument values contain non-alphanumeric characters. IPC::Run now handles them well, but the batch file itself may not.
This unblocks general use of programs like cmd.exe and cscript.exe as stages in IPC::Run pipelines.
Per its documentation, it reports via GetLastError(), not $!.
|
That looks hella impressive and comprehensive. Do the tests also make #147 superfluous? Agreed on the cygwin bit. |
|
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.
|
|
Folks with repository write access: is the matter of reviewing this pull request still in your queue? (#150 is another one ready to merge.) |
|
Upon merging this, run.t reached the 6h timeout instead of completing |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes a regression affecting Win32 batch files. The regression
originated in my #143. This implements the #143 (comment) 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:\...While this implementation handles Cygwin well for the tests in #143 (comment),
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.