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

Changes to dependencies #2

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@openstrike
Contributor

openstrike commented Nov 26, 2015

There are 3 minor changes to Makefile.PL here:

  1. I could not find any reference to Archive::Tar within the codebase and the tests seem to pass fine without it. Therefore this dependency has been removed from the list.
  2. perlver suggests that throughout the code the minimum version required is 5.6.0 so this has now been stipulated.
  3. As a result of (2) there seemed no point checking for versions above 5.005 any more. The affected fields have therefore been inserted unconditionally instead.
Added minumum version of perl.
Removed Archive::Tar dependency.
@mschilli

This comment has been minimized.

Owner

mschilli commented Nov 30, 2015

Not sure how many people are still running 5.005, but if there are any, their ancient ExtUtils::MakeMaker will trip over MIN_PERL_VERSION. I'm not sure how perlver arrives at a minimum required perl version of 5.006, do you know why?

@openstrike

This comment has been minimized.

Contributor

openstrike commented Nov 30, 2015

Good point about supporting the older EUMM versions. I'll update the PR for that shortly (edit: done now)

perlver mentions both the warnings pragma and variables declared with "our" as reasons for requiring 5.006 as a minimum.

@openstrike

This comment has been minimized.

Contributor

openstrike commented Jan 6, 2017

Are you happy with the amended PR or are there any further changes required?

@mschilli

This comment has been minimized.

Owner

mschilli commented Feb 19, 2017

Sorry for the delay on this, finally got around to take a closer look: I'm curious as to if there is anything broken that this change is going to fix? It seems that the existing checks verify around perl5.005 and provide backward compatibility (which, granted, doesn't have much weight 20 years later), but it didn't break anything for newer perls. Is there some kind of smoking test suite that has trouble with this approach? Following engineering rule #1, I usually don't fix what's not broken :).

@openstrike

This comment has been minimized.

Contributor

openstrike commented Feb 20, 2017

Thanks for revisiting this.

The benefit here is the same as that of specifying a minimum version of perl in any code. It doesn't affect the newer, compatible versions at all but it means that if someone tries to run it with a perl which is too old (in this case 5.005) then instead of getting arbitrary errors they get the informative message that their perl is too old.

Here's what perlver says when I run it:

   ---------------------------------------------------------------------  
 | file                                   | explicit | syntax | external |
 | --------------------------------------------------------------------- |
 | Makefile.PL                            | ~        | ~      | n/a      |
 | eg/memtest.pl                          | ~        | ~      | n/a      |
 | eg/check-type                          | ~        | ~      | n/a      |
 | eg/extract-comments                    | ~        | ~      | n/a      |
 | adm/release                            | ~        | v5.6.0 | n/a      |
 | t/004Perl.t                            | ~        | v5.6.0 | n/a      |
 | t/007PHP.t                             | ~        | v5.6.0 | n/a      |
 | t/001Basic.t                           | ~        | v5.6.0 | n/a      |
 | t/009Java.t                            | ~        | v5.6.0 | n/a      |
 | t/002C.t                               | ~        | v5.6.0 | n/a      |
 | t/005HTML.t                            | ~        | v5.6.0 | n/a      |
 | t/008JavaScript.t                      | ~        | v5.6.0 | n/a      |
 | t/006Python.t                          | ~        | v5.6.0 | n/a      |
 | t/003Makefile.t                        | ~        | v5.6.0 | n/a      |
 | lib/File/Comments.pm                   | ~        | v5.6.0 | n/a      |
 | lib/File/Comments/Plugin.pm            | ~        | v5.6.0 | n/a      |
 | lib/File/Comments/Plugin/Python.pm     | ~        | v5.6.0 | n/a      |
 | lib/File/Comments/Plugin/C.pm          | ~        | v5.6.0 | n/a      |
 | lib/File/Comments/Plugin/Shell.pm      | ~        | v5.6.0 | n/a      |
 | lib/File/Comments/Plugin/JavaScript.pm | ~        | v5.6.0 | n/a      |
 | lib/File/Comments/Plugin/Makefile.pm   | ~        | v5.6.0 | n/a      |
 | lib/File/Comments/Plugin/Perl.pm       | ~        | v5.6.0 | n/a      |
 | lib/File/Comments/Plugin/Java.pm       | ~        | v5.6.0 | n/a      |
 | lib/File/Comments/Plugin/HTML.pm       | ~        | v5.6.0 | n/a      |
 | lib/File/Comments/Plugin/PHP.pm        | ~        | v5.6.0 | n/a      |
 | --------------------------------------------------------------------- |
 | Minimum explicit version : ~                                          |
 | Minimum syntax version   : v5.6.0                                     |
 | Minimum version of perl  : v5.6.0                                     |
   ---------------------------------------------------------------------  

So, either the code won't run on perl 5.005 as it stands in which case this patch would be a benefit or else there's a bug in perlver (which would be more worrying). I don't have a 5.005 installation to test it on and am unaware of any smokers that old. If the tests do actually all pass on 5.005 then that part of the PR can be ditched (because I entirely agree with your viewpoint about not fixing things which aren't broken) and a bug should be raised against perlver.

I guess you've linked to the wrong commit in your message above?

@mschilli

This comment has been minimized.

Owner

mschilli commented Apr 30, 2017

Whoops, just saw that this is still open, sorry! Could it be that "perlver" just says "v5.6.0" is required because that's the oldest perl version it supports? I don't see anything in the code that would require it, filing a bug against perlver sounds like the right thing to do.

@mschilli mschilli closed this Apr 30, 2017

@openstrike

This comment has been minimized.

Contributor

openstrike commented May 8, 2017

Perlver goes back to 5.4.0, so that's not the problem. Here's a session showing how "use warnings" is one of the culprits:

$ echo 'use strict;' > foo.pl
$ perlver foo.pl


   -------------------------------------  
 | file   | explicit | syntax | external |
 | ------------------------------------- |
 | foo.pl | ~        | ~      | n/a      |
 | ------------------------------------- |
 | Minimum explicit version : ~                             |
 | Minimum syntax version   : ~                             |
 | Minimum version of perl  : v5.4.0 (default)              |
   -------------------------------------  

$ echo 'use warnings;' > foo.pl
$ perlver foo.pl


   -------------------------------------  
 | file   | explicit | syntax | external |
 | ------------------------------------- |
 | foo.pl | ~        | v5.6.0 | n/a      |
 | ------------------------------------- |
 | Minimum explicit version : ~                             |
 | Minimum syntax version   : v5.6.0                        |
 | Minimum version of perl  : v5.6.0                        |
   -------------------------------------  

Explicitly, here's what it complains about in File::Comments:

$ perlver --blame lib/File/Comments.pm 

 ------------------------------------------------------------
 File    : lib/File/Comments.pm
 Line    : 10
 Char    : 1
 Rule    : _perl_5006_pragmas
 Version : 5.006
 ------------------------------------------------------------
 use warnings;
 ------------------------------------------------------------

And corelist reports that warnings was indeed introduced in 5.6.0. I cannot see how any of this is a bug in perlver.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment