-
Notifications
You must be signed in to change notification settings - Fork 78
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
Version range warning #260
base: master
Are you sure you want to change the base?
Conversation
my ($self) = @_; | ||
my @eumm_versions = sort grep defined, map { | ||
$_->{'ExtUtils::MakeMaker'} | ||
} grep defined, @{$self}{@PREREQ_KEYS}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CONFIGURE_REQUIRES is the only place that is useful for the place this is being used, so checking all of the phases is incorrect. It's also possible to provide the configure requires only via META_ADD/META_MERGE, so this could result in spurious warnings.
22954b9
to
f367bec
Compare
} grep defined, @{$self}{qw(CONFIGURE_REQUIRES)}; | ||
return 0 unless @eumm_versions; | ||
$eumm_versions[0]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$self->{CONFIGURE_REQUIRES} && $self->{CONFIGURE_REQUIRES}{'ExtUtils::MakeMaker'} || 0
f367bec
to
ffdfb87
Compare
Based on discussion on IRC, I think we want to drop this idea. |
Yes we do |
If we do actually do something like this, then:
|
I'm very sorry for stirring up a hornet's nest. However it's good to know this nest is here so we don't step in it again, and maybe have some ideas for tidying it up in the future :) |
Another problem with this idea is that many Makefile.PL files will adjust how they call EUMM based on the EUMM version. So using a newer EUMM option without listing it in configure requires is perfectly fine. Adding these warnings would show false positives to users, and we can't detect users vs authors. Arrayref AUTHOR doesn't make sense to warn about. AUTHOR isn't really relevant on the user side, and if it is "broken" it doesn't impact installation. You are welcome to keep working on this if you want, but I don't think it's possible to do in a way that I will find acceptable. |
Fix #215.