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

use metacpan download_url endpoint #522

Merged
merged 2 commits into from Apr 19, 2018

Conversation

@haarg
Copy link
Contributor

haarg commented Nov 27, 2016

No description provided.

@haarg haarg referenced this pull request Nov 27, 2016
@miyagawa

This comment has been minimized.

Copy link
Owner

miyagawa commented Nov 27, 2016

Is this the same as #520?

@haarg

This comment has been minimized.

Copy link
Contributor Author

haarg commented Nov 27, 2016

This is against the old cpanm codebase. #520 is against Menlo.

@skaji

This comment has been minimized.

Copy link
Contributor

skaji commented Jan 2, 2017

So, if LWP is installed, but not LWP::Protocol::https, then cpanm will fail to access MetaCPAN.

@miyagawa

This comment has been minimized.

Copy link
Owner

miyagawa commented Jan 2, 2017

@haarg

This comment has been minimized.

Copy link
Contributor Author

haarg commented Jan 2, 2017

While this does change the URL used from http to https, api.metacpan.org always redirects to https. So the issue with LWP exists even without this PR.

@skaji

This comment has been minimized.

Copy link
Contributor

skaji commented Jan 2, 2017

@miyagawa

Is it not an issue in Menlo because it uses HTTP::Tiny?

Yes, it is. No, it isn't. (maybe you mean HTTP::Tinyish?)

@haarg

api.metacpan.org always redirects to https.

Oops. I think we used to be able to access api.metacpan.org via http.
Did metacpan-team change the scheme of api.metacpan.org during meta::hack?

$self->{mirrors} = [ 'http://cpan.metacpan.org' ];
}
return $self->cpan_module($module, $distfile, $module_version);
$self->{mirrors} = [ 'https://cpan.metacpan.org' ];

This comment has been minimized.

Copy link
@skaji

skaji Jan 2, 2017

Contributor

I don't have a strong opinion, but do we always need to choose cpan.metacpan.org as a mirror here?

This comment has been minimized.

Copy link
@haarg

haarg Jan 2, 2017

Author Contributor

Part of the reason for this is that cpan.metacpan.org is a backpan. This routine is mainly used for complex version specifications, which will often need a backpan, and by --metacpan, which it makes sense to switch the mirror for. I also don't really see a downside. It could be refactored to handle this somewhat differently, but I'd rather that be addressed in a separate PR.

This comment has been minimized.

Copy link
@skaji

skaji Jan 3, 2017

Contributor

I see.

This comment has been minimized.

Copy link
@ranguard

ranguard Jan 11, 2017

FYI: backpan.metacpan.org exists (it's just an alias for cpan.metacpan.org - but makes the distinction clear)

@@ -2923,7 +2798,7 @@ sub init_tools {
}

# use --no-lwp if they have a broken LWP, to upgrade LWP
if ($self->{try_lwp} && $self->has_working_lwp($self->{mirrors})) {
if ($self->{try_lwp} && $self->has_working_lwp($self->{mirrors}, $self->{metacpan})) {

This comment has been minimized.

Copy link
@skaji

skaji Jan 3, 2017

Contributor

$self->{metacpan} is not a flag for whether cpanm accesses MetaCPAN or not.

@skaji skaji referenced this pull request Jan 4, 2017
@haarg

This comment has been minimized.

Copy link
Contributor Author

haarg commented Jan 7, 2017

I was actually wrong about api.metacpan.org always redirecting to https. That was being done by my browser.

@haarg

This comment has been minimized.

Copy link
Contributor Author

haarg commented Jan 10, 2017

It would be possible for metacpan to allow non-https access to the API. Honestly, I think it's rather questionable for cpanm to use non-https at all, at least without explicit opt-in. But if that would make this changeover easier to accept, we can make something work.

@miyagawa

This comment has been minimized.

Copy link
Owner

miyagawa commented May 12, 2017

been fuzzy on the details of this - i merged the menlo version of the PR today, what should be done on this?

@skaji

This comment has been minimized.

Copy link
Contributor

skaji commented May 12, 2017

It is no wonder that MetaCPAN serves via https.
So how about checking LWP::Protocol::https always?

diff --git a/lib/App/cpanminus/script.pm b/lib/App/cpanminus/script.pm
index 38b6bcc..ae63240 100644
--- a/lib/App/cpanminus/script.pm
+++ b/lib/App/cpanminus/script.pm
@@ -2927,12 +2927,11 @@ sub file_mirror {
 }
 
 sub has_working_lwp {
-    my($self, $mirrors) = @_;
-    my $https = grep /^https:/, @$mirrors;
+    my $self = shift;
     eval {
         require LWP::UserAgent; # no fatpack
         LWP::UserAgent->VERSION(5.802);
-        require LWP::Protocol::https if $https; # no fatpack
+        require LWP::Protocol::https; # no fatpack
         1;
     };
 }
@@ -2947,7 +2946,7 @@ sub init_tools {
     }
 
     # use --no-lwp if they have a broken LWP, to upgrade LWP
-    if ($self->{try_lwp} && $self->has_working_lwp($self->{mirrors})) {
+    if ($self->{try_lwp} && $self->has_working_lwp) {
         $self->chat("You have LWP $LWP::VERSION\n");
         my $ua = sub {
             LWP::UserAgent->new(
@haarg

This comment has been minimized.

Copy link
Contributor Author

haarg commented May 12, 2017

I didn't plan on pursuing this any further. Menlo will be using the new API, and we now have a shim in place so cpanm 1.7 will use the v1 API behind the scenes. So cpanm is no longer holding us up from dropping the MetaCPAN v0 API.

haarg added 2 commits Nov 24, 2016
The code it was testing no longer exists.
@haarg haarg force-pushed the haarg:metacpan-download-url-api branch from dced3f3 to 91c5235 Apr 19, 2018
@miyagawa miyagawa closed this Apr 19, 2018
@miyagawa miyagawa reopened this Apr 19, 2018
@miyagawa miyagawa merged commit ae0be72 into miyagawa:devel Apr 19, 2018
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.