Specify full path for FIXIN where possible

[shawnlaffan]

Specifying the full path to pl2bat.bat makes it consistent with the other utilities specified in the generated makefiles and avoids possible issues with paths across MSYS/windows shells.

The second commit makes the code somewhat longer than the ternary but easier to read given it avoids listing the path twice. However, I do appreciate that readability is a subjective thing. I can elide it if it is deemed unnecessary.

Updates #459

[haarg]

This would prevent an updated ExtUtils::PL2Bat installed in a local::lib or similar from working.

[Leont]

This would prevent an updated ExtUtils::PL2Bat installed in a local::lib or similar from working.

Yeah, it should search installsitescript, installvendorscript (if defined) and installscript, in exactly that order.

[shawnlaffan]

This would prevent an updated ExtUtils::PL2Bat installed in a local::lib or similar from working.

Yeah, it should search installsitescript, installvendorscript (if defined) and installscript, in exactly that order.

I can easily add those checks but I'm not sure that would address the local::lib case given it only updates env vars and not %Config.

An alternative is to traverse the path to find the first instance of pl2bat.bat.

The ability, or documentation for how, to specify FIXIN in a Makefile.PL would be another approach. That would have a much lower risk of unintended side effects.

[Leont]

I can easily add those checks but I'm not sure that would address the local::lib case given it only updates env vars and not %Config.

Right, what I said would cover non-local::lib scenarios. With local::lib things suddenly get rather messy :-/

[sisyphus]

This would prevent an updated ExtUtils::PL2Bat installed in a local::lib or similar from working.

UPDATED ... original presentation was definitely rubbish.

In the proposed patch, would it make sense to change:

$self->{FIXIN} = -e $p2bpath ? qq{"$p2bpath"} : 'pl2bat.bat';
to:
$self->{FIXIN} = -e $p2bpath && !exists $INC{'ExtUtils/PL2Bat.pm'} ? qq{"$p2bpath"}  : 'pl2bat.bat';

[shawnlaffan]

Or just search the path?

my @path = split $Config{path_sep}, $ENV{PATH}; 
my @found = grep {-e qq{$_\\pl2bat.bat}} @path; 
$self->{FIXIN} = @found ? $found[0] : "pl2bat.bat";

[sisyphus]

Or just search the path?

LGTM. Getting back to the scenario (of running Strawberry Perl in an MSYS2 shell) that inspired this PR, that seems to behave as desired for me:

Owner@DESKTOP-88J497T MINGW64 ~
$ perl -MConfig -wle 'my @path = split $Config{path_sep}, $ENV{PATH}; my @found = grep {-e qq{$_\\pl2bat.bat}} @path; print for @found;'
C:\sp\_64\sp-5.40.0.1-PDL\perl\bin

If my head's screwed on correctly this morning (not guaranteed), I think you'll want to append \pl2bat.bat to $found[0].

[shawnlaffan]

If my head's screwed on correctly this morning (not guaranteed), I think you'll want to append \pl2bat.bat to $found[0].

Good point.

Will update the PR accordingly.

[sisyphus]

If I've got it right, the current proposed patch looks like this:

--- MM_Win32.pm_orig    2024-08-25 21:20:11.103542100 +1000
+++ MM_Win32.pm 2024-08-25 21:22:49.723888800 +1000
@@ -142,9 +142,18 @@
     $self->{NOOP}     ||= 'rem';
     $self->{DEV_NULL} ||= '> NUL';

-    $self->{FIXIN}    ||= $self->{PERL_CORE} ?
-      "\$(PERLRUN) -I$self->{PERL_SRC}\\cpan\\ExtUtils-PL2Bat\\lib $self->{PERL_SRC}\\win32\\bin\\pl2bat.pl" :
-      'pl2bat.bat';
+    if (!$self->{FIXIN}) {
+        if ($self->{PERL_CORE}) {
+            my $psrc = $self->{PERL_SRC};  #  shorten next line
+            $self->{FIXIN} = "\$(PERLRUN) -I${psrc}\\cpan\\ExtUtils-PL2Bat\\lib ${psrc}\\win32\\bin\\pl2bat.pl";
+        }
+        else {
+            my @path = split $Config{path_sep}, $ENV{PATH};
+            my @found = grep {-e qq{$_\\pl2bat.bat}} @path;
+            $self->{FIXIN} = @found ? qq{"$found[0]\\pl2bat.bat"} : "pl2bat.bat";
+        }
+    }
+

     $self->SUPER::init_tools;

(I applied the patch by hand and maybe I've injected an additional empty line into it at the end.)

It seems to me that, with the original rendition, the command:

"\$(PERLRUN) -I$self->{PERL_SRC}\\cpan\\ExtUtils-PL2Bat\\lib $self->{PERL_SRC}\\win32\\bin\\pl2bat.pl"

could be executed if $self->{FIXIN} was either TRUE or FALSE. With the proposed rewrite, I take it that can only happen if $self->{FIXIN} is FALSE.

Does that matter ? (I don't have a good understanding of what's at stake here.)

The proposed patch apparently breaks my scripted build of current blead by refusing to build dist/XSLOADER (for reasons not yet investigated). But this is not necessarily an issue because I'm building perl via a script being run by an old ActiveState build of perl-5.16.0 - and I think it could perhaps be argued that this approach of mine is not very smart. It turns out that if I rename the ActiveState bin/pl2bat.bat to bin/pl2bat.bat_hide then my scripted build proceeds nicely, as has been the case for the last 6 years or so. And that "fix" is totally acceptable to me.

[mohawk2]

If I've got it right, the current proposed patch looks like this:

--- MM_Win32.pm_orig  2024-08-25 21:20:11.103542100 +1000
+++ MM_Win32.pm   2024-08-25 21:22:49.723888800 +1000
@@ -142,9 +142,18 @@
[snip]
-    $self->{FIXIN}    ||= $self->{PERL_CORE} ?
[snip]
+    if (!$self->{FIXIN}) {
+        if ($self->{PERL_CORE}) {
[snip]

It seems to me that, with the original rendition, the command: [snip] could be executed if $self->{FIXIN} was either TRUE or FALSE. With the proposed rewrite, I take it that can only happen if $self->{FIXIN} is FALSE.

No, because the previous code said ||=. It only happened if $self->{FIXIN} was false, just like the new one.

[sisyphus]

No, because the previous code said ||=.

Heh ... just needed to think about what ||= actually does. I read it as (A ||= B) ? one thing : another thing (which wouldn't make any sense, anyhow) and noted that if(A ||= B) is true if both A and B are true. But, of course, it's A ||= (B ? one thing : another thing).

Sorry for the noise.

UPDATE: As if it's not already embarrassing enough, it gets even dumber. The failure I was seeing had nothing to do with the presence of pl2bat.bat in the path - and the removal of said pl2bat.bat from the path had nothing to do with the solution. The problem was simply a botched patch application .

[mohawk2]

Are there any further objections, or can this be merged?

[Leont]

So if I understand this correctly pl2bat will now be searched at configure-time instead of build-time? And that solves problems?

[shawnlaffan]

So if I understand this correctly pl2bat will now be searched at configure-time instead of build-time? And that solves problems?

The motivation is in https://github.com/Perl-Toolchain-Gang/ExtUtils-MakeMaker/issues/459#issue-2469476823 . Basically paths can become mixed up under some circumstances, such that msys2 paths are used in Windows shells.

@mohawk2 - are you able to test the PDL CI system using EUMM from this PR?

[mohawk2]

@mohawk2 - are you able to test the PDL CI system using EUMM from this PR?

No more so than you. If you fork a repo that had a problem (such as PDL-NDBin), then change its version-pinned Strawberry Perl back to latest (check that still fails), then with a new commit, insert a step to install EUMM from your branch, then we'll know. Use cpanm to install from a repo and branch with e.g. cpanm https://github.com/shawnlaffan/ExtUtils-MakeMaker/archive/win32_fixin_path.tar.gz.