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

Set Perl minimum version explicitly #16

Merged
merged 1 commit into from Aug 14, 2018

Conversation

@paultcochrane
Copy link
Contributor

@paultcochrane paultcochrane commented Aug 13, 2018

While checking the Perl minimum version value I found that some of the demo scripts were using an explicit version that was below that which was set in the Makefile.PL. Also, I noticed that although a minimum Perl version was set in the Makefile.PL (and hence in the dist's metadata), it wasn't actually set in the dist's main package. The commits in this PR bump the Perl version in the demo scripts to match that set in the dist's metadata and set the minimum version explicitly in the dist's main package so that the dist's code is consistent with its metadata.

If you'd like me to change or update anything, just let me know and I'll update and resubmit as necessary.

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

require 5.004;
require 5.006;

This comment has been minimized.

@vadrer

vadrer Aug 13, 2018
Owner

surprisingly, the tests were borrowed from perl/Tk, and I intentionally have not edited anything that is irrelevant to test passing

This comment has been minimized.

@paultcochrane

paultcochrane Aug 13, 2018
Author Contributor

So, do you want me to update the patch to not change these files?

This comment has been minimized.

This comment has been minimized.

@paultcochrane

paultcochrane Aug 13, 2018
Author Contributor

I removed the commit bumping the required Perl version. Is that all you'd like changed?

@@ -1,6 +1,7 @@
package Tcl::Tk;

use strict;
use 5.006;

This comment has been minimized.

@vadrer

vadrer Aug 13, 2018
Owner

the perl version already checked during Makefile.PL stage, I think adding this extra check - to avoid haskers who were brave enough to bypass Makefile.PL check??

This comment has been minimized.

@paultcochrane

paultcochrane Aug 13, 2018
Author Contributor

As far as I understand it, the Makefile.PL just sets metadata and doesn't actually enforce anything (other than allowing things like cpanm and CPANTS to work automatically). I took my inspiration for the patch from http://blogs.perl.org/users/neilb/2014/08/specify-the-min-perl-version-for-your-distribution.html where Neil Bowers mentions that to set the minimum Perl version, one sets it at the top of the module. If you don't want the change, I'm OK with it; I just wanted to make sure my patch contained all the elements that seem to be deemed necessary by current best practices.

This comment has been minimized.

@vadrer

vadrer Aug 13, 2018
Owner

point is that module will even probably work at 5.002, I hope :)

This comment has been minimized.

@vadrer

vadrer Aug 13, 2018
Owner

also, your link namely mentions "makefile.pl" specifying perl version.
It is present in Tcl::Tk, isn't it?

This comment has been minimized.

@vadrer

vadrer Aug 13, 2018
Owner

what exact place in Tk.pm "too modern" to request namely 5.006, and not version below?

This comment has been minimized.

@paultcochrane

paultcochrane Aug 14, 2018
Author Contributor

OK, I'll set it to use 5; and push in a minute.

This comment has been minimized.

@chrstphrchvz

chrstphrchvz Jun 24, 2019
Contributor

I think there was a misunderstanding of what the recommendation was.

The use 5.x; statement should only specify a version that is equal or higher to the version required, and not a version lower than required.

Because use 5; (i.e. Perl 5.0) is now specified but Perl 5.5 syntax is present, perlver lib/Tcl/Tk.pm now outputs ERROR : ACTUAL DEPENDENCY HIGHER THAN SPECIFIED (use perlver --blame for finding syntax responsible).

I think a decent attempt to address this was already made, so I will forgo proposing to change this.

This comment has been minimized.

@vadrer

vadrer Jun 24, 2019
Owner

ok, show me what is of '5.5' syntax

...and I am more about to delete this use 5 at all, rather than figuring out exact perl version that is required. probably 5.6. but - why care? To please buggy linter?

This comment has been minimized.

@chrstphrchvz

chrstphrchvz Mar 12, 2020
Contributor

The 5.5 syntax in lib/Tcl/Tk.pm is the use of negative lookbehind (?<!_) (see https://github.com/Perl/perl5/blob/perl-5.005/pod/perldelta.pod#regular-expressions)

Running perlver --blame lib/Tcl/Tk.pm repeatedly:

------------------------------------------------------------
 File    : lib/Tcl/Tk.pm
 Line    : 866
 Char    : 2
 Rule    : _regex
 Version : 5.005
 s/(?<!_)__(?!_)/::/g
 ------------------------------------------------------------

 ------------------------------------------------------------
 File    : lib/Tcl/Tk.pm
 Line    : 867
 Char    : 2
 Rule    : _regex
 Version : 5.005
 s/(?<!_)___(?!_)/_/g
 ------------------------------------------------------------

 ------------------------------------------------------------
 File    : .\lib\Tcl\Tk.pm
 Line    : 2360
 Char    : 2
 Rule    : _regex
 Version : 5.005
 ------------------------------------------------------------
 s/(?<!_)__(?!_)/::/g
 ------------------------------------------------------------

 ------------------------------------------------------------
 File    : .\lib\Tcl\Tk.pm
 Line    : 2361
 Char    : 2
 Rule    : _regex
 Version : 5.005
 ------------------------------------------------------------
 s/(?<!_)___(?!_)/_/g
 ------------------------------------------------------------

perlver doesn't see anything else it knows a minimum version for in lib/Tcl/Tk.pm.

This comment has been minimized.

@vadrer

vadrer Mar 12, 2020
Owner

hm... ok, probably I will remove this negative lookbehind.
thank you

@paultcochrane paultcochrane force-pushed the paultcochrane:pr/set-perl-min-version branch from 2581681 to 1ce8a09 Aug 13, 2018
The change implemented here ensures that the main package explicitly
declares the Perl version and that it is consistent with the minimum
Perl version required by the module's syntax.
@paultcochrane paultcochrane force-pushed the paultcochrane:pr/set-perl-min-version branch from 1ce8a09 to bc37cda Aug 14, 2018
@vadrer vadrer merged commit 2d0a94f into vadrer:master Aug 14, 2018
@vadrer
Copy link
Owner

@vadrer vadrer commented Aug 14, 2018

thanks :):)

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.