Fix build in Windows/msys

[pauloscustodio]

msys does not accept .BAT as executables

[nmisch]

I think this is revealing a bug in IPC::Run::_search_path(). That function should search $PATH like the system shell does. POSIX shells skip non-executable files and continue the search. IPC::Run::_search_path() mimics that via a series of "-x" filetests. On a "mount -o noacl" mount, the msys shell doesn't skip non-executable files; instead, it attempts to execute and gets Access Denied. cmd.exe behaves that way, too. (On a "mount -o acl" mount, msys has POSIX behavior. Cygwin behaves like msys, but Cygwin defaults to "-o acl" while msys defaults to "-o noacl".) The documentation does say, "executables must pass both the C<-f> and C<-x> tests". Given that documentation, one could argue the IPC::Run behavior is not a bug. However, I doubt folks want IPC::Run to search $PATH in a way unlike the OS's norm.

Would you use the Test::More "TODO" mechanism for these, instead of not running them? That way, IPC::Run developers get reminders of the underlying bug. See t/autoflush.t for an example of doing that.

[pauloscustodio]

Thanks for the feed-back. I have now added an empty TODO block just for future reference and replaced the two ok()s by a SKIP block.

Regards Paulo Custódio

On Mon, Jul 4, 2022 at 6:26 AM Noah Misch @.***> wrote:

I think this is revealing a bug in IPC::Run::_search_path(). That function should search $PATH like the system shell does. POSIX shells skip non-executable files and continue the search. IPC::Run::_search_path() mimics that via a series of "-x" filetests. On a "mount -o noacl" mount, the msys shell doesn't skip non-executable files; instead, it attempts to execute and gets Access Denied. cmd.exe behaves that way, too. (On a "mount -o acl" mount, msys has POSIX behavior. Cygwin behaves like msys, but Cygwin defaults to "-o acl" while msys defaults to "-o noacl".) The documentation does say, "executables must pass both the C<-f> and C<-x> tests". Given that documentation, one could argue the IPC::Run behavior is not a bug. However, I doubt folks want IPC::Run to search $PATH in a way unlike the OS's norm.

Would you use the Test::More "TODO" mechanism for these, instead of not running them? That way, IPC::Run developers get reminders of the underlying bug. See t/autoflush.t for an example of doing that.

— Reply to this email directly, view it on GitHub https://github.com/toddr/IPC-Run/pull/155#issuecomment-1173364584, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAARI5NY2QDUAP3R7KADQ43VSJYXVANCNFSM5YNFOLBQ . You are receiving this because you authored the thread.Message ID: @.***>

[nmisch]

I meant something like this:

--- a/t/windows_search_path.t
+++ b/t/windows_search_path.t
@@ -33,2 +33,6 @@ while (@tests) {
     my $got = eval { IPC::Run::_search_path($path) };
+
+    # see https://github.com/toddr/IPC-Run/pull/155 conversation for details
+    local $TODO = qq{on msys or Cygwin noacl mounts, "-x $result" is false}
+      if $result =~ /BAT$/ && !-x $result;
     is( $@,   '',      "No error calling _search_path for '$path'" );

That way, the .BAT test always runs, but its failure doesn't block things.

[pauloscustodio]

Thanks for the comment.

I have now done as suggested to minimize changes to the master version, but I had to use a SKIP block instead of a TODO block. Using a TODO block would require the inclusion of the two tests that fail in cygwin inside the block, and the repetition of the two tests in an else clause.

Hope this helps.

On Tue, Jul 5, 2022 at 7:25 PM Noah Misch @.***> wrote:

I meant something like this:

--- a/t/windows_search_path.t +++ b/t/windows_search_path.t @@ -33,2 +33,6 @@ while @.***) { my $got = eval { IPC::Run::_search_path($path) }; +

• see https://github.com/toddr/IPC-Run/pull/155 conversation for details

• local $TODO = qq{on msys or Cygwin noacl mounts, "-x $result" is false}
• if $result =~ /BAT$/ && !-x $result; is( $@, '', "No error calling _search_path for '$path'" );

That way, the .BAT test always runs, but its failure doesn't block things.

— Reply to this email directly, view it on GitHub https://github.com/toddr/IPC-Run/pull/155#issuecomment-1175362321, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAARI5OORUDLOBGQ3TBOA5LVSR423ANCNFSM5YNFOLBQ . You are receiving this because you authored the thread.Message ID: @.***>

[nmisch]

My goal wasn't to minimize changes. My goal was to change the "make test" exit status from success to failure without ceasing to run any of these tests. I've now tried the change on msys, and it appears to achieve that. Would you like me to push that, or would you prefer me to merge what you have here? For what it's worth, here is the raw TAP output under the change:

On msys:

$ perl -Iblib/lib t/windows_search_path.t
1..11
ok 1 - We're win32 mode?
ok 2 - No error calling _search_path for './temp'
ok 3 - Executable ./temp.EXE found
ok 4 - No error calling _search_path for '.\temp'
ok 5 - Executable .\temp.EXE found
ok 6 - No error calling _search_path for './5.11.5/temp'
ok 7 - Executable ./5.11.5/temp.EXE found
not ok 8 - No error calling _search_path for './5.11.5/temp' # TODO on msys or Cygwin noacl mounts, "-x ./5.11.5/temp.BAT" is false
#   Failed (TODO) test 'No error calling _search_path for './5.11.5/temp''
#   at t/windows_search_path.t line 39.
#          got: 'file not found: ./5.11.5/temp at t/windows_search_path.t line 33.
# '
#     expected: ''
not ok 9 - Executable ./5.11.5/temp.BAT found # TODO on msys or Cygwin noacl mounts, "-x ./5.11.5/temp.BAT" is false
#   Failed (TODO) test 'Executable ./5.11.5/temp.BAT found'
#   at t/windows_search_path.t line 40.
#          got: undef
#     expected: './5.11.5/temp.BAT'
ok 10 - No error calling _search_path for './5.11.5/temp'
ok 11 - Executable ./5.11.5/temp.COM found

On GNU/Linux:

$ perl -I blib/lib t/windows_search_path.t
1..11
ok 1 - We're win32 mode?
ok 2 - No error calling _search_path for './temp'
ok 3 - Executable ./temp.EXE found
ok 4 - No error calling _search_path for '.\temp'
ok 5 - Executable .\temp.EXE found
ok 6 - No error calling _search_path for './5.11.5/temp'
ok 7 - Executable ./5.11.5/temp.EXE found
ok 8 - No error calling _search_path for './5.11.5/temp'
ok 9 - Executable ./5.11.5/temp.BAT found
ok 10 - No error calling _search_path for './5.11.5/temp'
ok 11 - Executable ./5.11.5/temp.COM found

[nmisch]

s/success to failure/failure to success/

[pauloscustodio]

Thanks for the feedback. My goal was different, it was to be able to install on msys without --force, therefore the difference in implementation.

Please go ahead and merge your changes. I've added cpanm --force to my build script on msys.

Reagards, Paulo Custódio

On Sat, Jul 9, 2022 at 8:07 PM Noah Misch @.***> wrote:

s/success to failure/failure to success/

— Reply to this email directly, view it on GitHub https://github.com/toddr/IPC-Run/pull/155#issuecomment-1179592130, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAARI5I3DV7UCLJ4H32DIHDVTHEW7ANCNFSM5YNFOLBQ . You are receiving this because you authored the thread.Message ID: @.***>

[nmisch]

Pushed that way (commit b6b6a9a). I expect commit b6b6a9a achieves your goal, too: cpanm will no longer require --force if pointed at sources containing that commit. If you find otherwise, please share the cpanm command output.

[pauloscustodio]

Solution as pushed to master tested and is OK in Windows/msys2. Thank you.