-
Notifications
You must be signed in to change notification settings - Fork 213
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
cpanm detects and does not try to double-install modules that depend on themselves #503
base: devel
Are you sure you want to change the base?
Conversation
b67ecf6
to
09738f3
Compare
Menlo-Legacy/lib/Menlo/CLI/Compat.pm
Outdated
my $module_name = $self->find_module_name($configure_state) || $dist->{meta}{name}; | ||
$module_name =~ s/-/::/g; | ||
|
||
my @all_deps = $self->find_prereqs($dist); | ||
my @deps = grep { $_->module ne $module_name } @all_deps; |
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.
Will depending on SelfDep::SubPackage
from the same dist trick this check?
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.
Indeed it would; well-spotted. I was concerned about this as well, but thought it better to catch at least some cases rather than none.
I've pushed some changes to address the sub-package case: now, the code uses Module::Metadata->package_versions_from_directory
to enumerate all packages in $dist->{dir}
. This should catch the SelfDep::SubPackage
case, but I'm worried that the logic is now overbroad and catches things like modules that aren't exposed for public consumption (e.g. test-suite-only stuff).
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.
I'm worried that the logic is now overbroad and catches things like modules that aren't exposed for public consumption (e.g. test-suite-only stuff).
On reflection, this probably isn't a big deal.
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.
Yep. You can hide from the metacpan indexer with:
package # hide from indexer
MyApp::Secret;
I assume the same trick works elsewhere
Some tests are failing, but they're also failing in |
Some modules list themselves as dependencies (looking at you,
Bundle::CPAN
). In combination withcpanm
's tracking of$self->{seen}{$module}
, this causescpanm
to mark such a module as having an unsatisfied dependency -- namely, itself. This PR implements a fix that filters out a module from the list of its dependencies and issues a warning message to that effect.