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

Patch bin scripts to support the #! Makefile.PL was invoked with #114

Merged
merged 1 commit into from
Jul 8, 2022

Conversation

toddr
Copy link
Contributor

@toddr toddr commented Nov 27, 2018

setting it to /usr/bin/perl makes EUMM update the #! during install

@codecov
Copy link

codecov bot commented Nov 27, 2018

Codecov Report

Merging #114 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #114   +/-   ##
======================================
  Coverage    21.5%   21.5%           
======================================
  Files          71      71           
  Lines       19914   19914           
  Branches     5728    5728           
======================================
  Hits         4283    4283           
  Misses      14922   14922           
  Partials      709     709
Impacted Files Coverage Δ
script/sqlt-diff 64.28% <ø> (ø) ⬆️
script/sqlt-diagram 57.14% <ø> (ø) ⬆️
script/sqlt-diff-old 60.35% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86c68a0...2551115. Read the comment docs.

@abraxxa
Copy link
Contributor

abraxxa commented Nov 27, 2018

Which breaks building in a different path than deploying to.
-1 from me

@toddr
Copy link
Contributor Author

toddr commented Nov 28, 2018

I'll give you a real work example. Let's say you install SQL::Translator to your perl in /usr/local. The assumption is that if I run /usr/local/bin/sqlt-diff, it'll work. However if my $PATH is set to pick perl out of /usr/bin first, then the script will fail because SQL::Translator was not installed to that perl.

So you want ExtUtils::MakeMaker to update the #! of all of the scripts during make install to point to the path of the perl they were installed to. It will not do this unless your #! is /usr/bin/perl in cpan distro.

https://metacpan.org/pod/ExtUtils::MakeMaker#EXE_FILES

@abraxxa
Copy link
Contributor

abraxxa commented Nov 28, 2018

I know that behaviour and it annoys me very much. We build our Perls using -Duserelocatableinc and EUMM breaks that when deployed to a different directory on the producation boxes than on the build host.

@toddr
Copy link
Contributor Author

toddr commented Nov 28, 2018

We're probably having this discussion in the wrong place but IMO given the history of how EUMM adjusts #!, it seems like the better thing to do would be to add a feature to EUMM to customize the #! to whatever you choose via an ENV var or some other means. Possibly even by having it check for -Drelocatable in %Config.

The key is that EUMM should be consistently updating #! for all scripts it installs into bin. I should be able to specify what #! needs to be to EUMM. Instead what's happening right now is that we're all being left unhappy, depending on what method we prefer (some CPAN modules do env and some don't).

@toddr
Copy link
Contributor Author

toddr commented Nov 28, 2018

@abraxxa BTW, this discussion is ongoing at: Perl-Toolchain-Gang/ExtUtils-MakeMaker#58

@Grinnz
Copy link

Grinnz commented Nov 28, 2018

Using #!/usr/bin/env perl as the shebang breaks installation for nearly everyone else (whether they find out immediately or later), because it won't use the perl that the CPAN installer ensured satisfies the dependencies for this distribution. This is the reality of the situation currently.

@rabbiveesh
Copy link
Contributor

Im hoping to do a release soon, and would like to merge easy issues to help give the impression that there'll be active maintenance.

I think that changing the shebangs is the right thing, but discussion on IRC has led me to believe that #!perl is the best option. That way you can't accidentally run it on the wrong perl during development (a problem that I have stumbled on more than once).

Any objections?

@abraxxa
Copy link
Contributor

abraxxa commented Mar 4, 2022

As said before that breaks relocateable Perls where the perl binary is in a different directory at install time vs. at runtime.
So I'm against merging this.

@Grinnz
Copy link

Grinnz commented Mar 4, 2022

In my opinion, breaking in standard installations is more critical than breaking in relocatable installations, which is why the shebang fix is utilized by nearly all CPAN scripts. Clearly it is not a perfect fix (and the referenced EUMM issue stalled out before anyone did the work) but relocatable installations are very much an advanced usage of perl and such users will need to deal with this issue regardless - I don't think it's appropriate to require the vast majority of users to also deal with it.

setting it to #!perl makes EUMM update the #! during install
@toddr
Copy link
Contributor Author

toddr commented Mar 4, 2022

I've updated the PR to match your decision. This also solves my problem.

@rabbiveesh
Copy link
Contributor

sorry for the delay, thanks for the PR!

@rabbiveesh rabbiveesh merged commit 8faa4e8 into dbsrgits:master Jul 8, 2022
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.

None yet

4 participants