Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Specify full path for FIXIN where possible #460

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

shawnlaffan
Copy link

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

Otherwise fall back to the existing behaviour and
let the system find it on the path.
Makes the code somewhat more readable.
@haarg
Copy link
Member

haarg commented Aug 21, 2024

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

@Leont
Copy link
Member

Leont commented Aug 21, 2024

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
Copy link
Author

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
Copy link
Member

Leont commented Aug 21, 2024

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
Copy link
Contributor

sisyphus commented Aug 22, 2024

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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.

And specify its full path in the generated Makefile.
@sisyphus
Copy link
Contributor

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
Copy link
Member

mohawk2 commented Aug 25, 2024

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
Copy link
Contributor

sisyphus commented Aug 26, 2024

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
Copy link
Member

mohawk2 commented Sep 2, 2024

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

@Leont
Copy link
Member

Leont commented Sep 6, 2024

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

@shawnlaffan
Copy link
Author

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 #459 (comment) . 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
Copy link
Member

mohawk2 commented Sep 7, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants