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

Replace hardcoded shebang perl path with perl from env #19

Merged
merged 1 commit into from Aug 14, 2018

Conversation

@paultcochrane
Copy link
Contributor

@paultcochrane paultcochrane commented Aug 13, 2018

Not all systems have perl installed under /usr/local; using the perl
from the environment will hence work more reliably.

Also, the shebang line should be in the first line of the script (see e.g. https://en.wikipedia.org/wiki/Shebang_(Unix)#History) ; I've thus moved the shebang lines that I've changed here that weren't at the top of the file to the top.


use warnings;

This comment has been minimized.

@vadrer

vadrer Aug 13, 2018
Owner

is it really necessary?
probably '-w' above is the solution?
or even BEGIN{$^W=1}
?
for my taste, warnings.pm became considerably worse in recent perls

@@ -1,5 +1,6 @@
#!/usr/local/bin/perl -w
#!/usr/bin/env perl

This comment has been minimized.

@vadrer

vadrer Aug 13, 2018
Owner

given
https://metacpan.org/source/SREZIC/Tk-804.034/demos/widget
I would prefer to either remove that line at all or make it similar

@paultcochrane paultcochrane force-pushed the paultcochrane:pr/generalise-demo-shebangs branch from 6ca90f4 to 701efa7 Aug 14, 2018
@paultcochrane
Copy link
Contributor Author

@paultcochrane paultcochrane commented Aug 14, 2018

I just pushed an update to the patch to conform to the shebang line you want to have. Hope that does the job correctly.

@@ -19,6 +19,7 @@

require 5.002;
use strict;
use warnings;

This comment has been minimized.

@vadrer

vadrer Aug 14, 2018
Owner

this "use warnings" contradicts to "use 5.002";

Ok, I do not want "use warnings;" in tcl/tk while they have this :
die sprintf "Incorrect use of pragma '%s' at %s line %d.\n", __PACKAGE__, +(caller)[1,2] if __FILE__ !~ ( '(?x) \b '.__PACKAGE__.' \.pmc? \z' ) && __FILE__ =~ ( '(?x) \b (?i:'.__PACKAGE__.') \.pmc? \z' );

This comment has been minimized.

@vadrer

vadrer Aug 14, 2018
Owner

...sorry for being dumb in this regard.....

Not all systems have perl installed under /usr/local; using perl should
work more reliably.
@paultcochrane paultcochrane force-pushed the paultcochrane:pr/generalise-demo-shebangs branch from 701efa7 to 7dd0b18 Aug 14, 2018
@paultcochrane
Copy link
Contributor Author

@paultcochrane paultcochrane commented Aug 14, 2018

Whoops, sorry. I forgot to remove the use warnings in my last update. Have removed it in this one.

@vadrer vadrer merged commit fca5bbc into vadrer:master Aug 14, 2018
@vadrer
Copy link
Owner

@vadrer vadrer commented Aug 14, 2018

thank you! :)

@chrstphrchvz
Copy link
Contributor

@chrstphrchvz chrstphrchvz commented Aug 15, 2018

I had brought these issues up on the mailing list about a month ago, but wasn't sure how to proceed.

I considered adding use warnings anywhere there was a -w in the shebang (assuming the presence of -w indicated the intention for the particular script to work with warnings enabled), for things like tests which typically don't get executed directly. (See https://perldoc.perl.org/warnings.html#What's-wrong-with-*-w*-and-%24%5eW for why to use warnings instead of -w and $^W.)

A common spurious warning seems to be the used only once: possible typo for things from other packages.

Perl/Tk explained its choice of /usr/bin/perl instead of /usr/bin/env perl as being due to a MakeMaker requirement (though before that it was using /usr/local/bin/perl until a debian user reported an issue about it). There's a long and unfinished discussion on getting MakeMaker to replace /usr/bin/env perl. The argument for using that instead of /usr/bin/perl is that the former is what is determined by the PATH while the latter isn't.

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.