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

Porting/sync-with-cpan has implicit undeclared dependencies #18109

Closed
jkeenan opened this issue Sep 2, 2020 · 6 comments · Fixed by #18167
Closed

Porting/sync-with-cpan has implicit undeclared dependencies #18109

jkeenan opened this issue Sep 2, 2020 · 6 comments · Fixed by #18167
Assignees
Labels
discovered-thru-p7-research Issue was discovered in the course of research in the core-p7 branch

Comments

@jkeenan
Copy link
Contributor

jkeenan commented Sep 2, 2020

On the face of it, Porting/sync-with-cpan appears to depend solely on libraries that ship with the core distribution:

use Getopt::Long;
use Archive::Tar;
use File::Basename qw( basename );
use File::Path qw( remove_tree );
use File::Find;
use File::Spec::Functions qw( tmpdir rel2abs );
use Config qw( %Config );

However, its wget() function preferentially requires HTTP::Tiny, then calls this:

    eval {
        require HTTP::Tiny;
        my $http= HTTP::Tiny->new();
        $http->mirror( $url => $saveas );
        1
    } or

... where the variables have values like these:

$url    = 'https://cpan.metacpan.org/authors/id/B/BI/BINGOS/ExtUtils-Install-2.16.tar.gz';
$saveas = 'ExtUtils-Install-2.16.tar.gz';

Note that in that eval, the existing code does not check for $http->response. Note further that we're making an https call, not an http call.

But to make an https support, HTTP::Tiny requires IO::Socket::SSL 1.42 and Net::SSLeay 1.49 -- neither of which ship with the core distribution. The following program extracts the HTTP::Tiny->mirror call that sync-with-cpan does.

$ cat http-tiny-problem.pl 
use feature 'say';
use HTTP::Tiny;

my $url = 'https://cpan.metacpan.org/authors/id/B/BI/BINGOS/ExtUtils-Install-2.16.tar.gz';
 #my $url = 'http://cpan.metacpan.org/authors/id/B/BI/BINGOS/ExtUtils-Install-2.16.tar.gz';
my $saveas = 'ExtUtils-Install-2.16.tar.gz';
my $http= HTTP::Tiny->new();
my $response = $http->mirror( $url => $saveas );
if ( $response->{success} ) {
    say "MMM1: $saveas is up to date";
}
else {
    say "MMM2: HTTP::Tiny response apparently unsuccessful";
    say "MMM3: $response->{status} $response->{reason}";
}

If, in an environment where neither IO::Socket::SSL nor Net::SSLeay is installed, I call this program, I get:

MMM2: HTTP::Tiny response apparently unsuccessful
MMM3: 599 Internal Exception

If I run the program above through the debugger, I eventually come to this:

HTTP::Tiny::mirror(/home/jkeenan/testing/alpha-dev-02-strict/lib/perl7/7.0.0/HTTP/Tiny.pm:305):
305:	    close $fh
306:	        or _croak(qq/Error: Caught error closing temporary file $tempfile: $!\n/);
  DB<10> x $response
0  HASH(0x560ea90da5c0)
   'content' => 'IO::Socket::SSL 1.42 must be installed for https support
Net::SSLeay 1.49 must be installed for https support
'
   'headers' => HASH(0x560ea90668a8)
      'content-length' => 110
      'content-type' => 'text/plain'
   'reason' => 'Internal Exception'
   'status' => 599
   'success' => ''
   'url' => 'https://cpan.metacpan.org/authors/id/B/BI/BINGOS/ExtUtils-Install-2.16.tar.gz'

So, in the way that Porting/sync-with-cpan is typically used, it has undeclared implicit dependencies. Those committers who run this program typically have lots of CPAN modules installed. Hence, we're never tripped up on this. But this means you pretty much can't run `Porting/sync-with-cpan on just the core distribution alone.

How shall we address this?

Thank you very much.
Jim Keenan

@jkeenan jkeenan added Needs Triage discovered-thru-p7-research Issue was discovered in the course of research in the core-p7 branch labels Sep 2, 2020
@Grinnz
Copy link
Contributor

Grinnz commented Sep 2, 2020

I previously suggested adding proper error checking to the HTTP::Tiny call, which would expose the error message you found via the debugger; the error checking that is there is insufficient.

@Grinnz
Copy link
Contributor

Grinnz commented Sep 2, 2020

Your example program could be improved to report such errors by changing the last line in the else block to:

say $response->{status} == 599 ? "MMM3: $response->{reason}: $response->{content}" : "MMM3: $response->{status} $response->{reason}";

@xsawyerx
Copy link
Member

Anything under Porting is not subject to only depending on core. It's a set of scripts to help maintain the language and we can accept some bootstrapped environment modules with some of them.

I suggest we:

  • Improve the error checking (as @Grinnz suggests).
  • Explicitly declare the dependency.

@jkeenan jkeenan self-assigned this Sep 25, 2020
@jkeenan
Copy link
Contributor Author

jkeenan commented Sep 25, 2020

Anything under Porting is not subject to only depending on core. It's a set of scripts to help maintain the language and we can accept some bootstrapped environment modules with some of them.

I suggest we:

* Improve the error checking (as @Grinnz suggests).

* Explicitly declare the dependency.

I'll take this ticket and work on it between other projects.

jimk

@xsawyerx
Copy link
Member

xsawyerx commented Sep 25, 2020

Anything under Porting is not subject to only depending on core. It's a set of scripts to help maintain the language and we can accept some bootstrapped environment modules with some of them.
I suggest we:

* Improve the error checking (as @Grinnz suggests).

* Explicitly declare the dependency.

I'll take this ticket and work on it between other projects.

You are already working on a lot of things at the same time. Let me take this off your hands.

@xsawyerx
Copy link
Member

Something's wrong with me. You already fixed this in 34c2009.

We just need the error checking to be better.

xsawyerx added a commit that referenced this issue Sep 25, 2020
The original code uses HTTP::Tiny and if it fails, we try alternatives.

This isn't the best strategy because it might fail for legitimate
reasons and we need to separate whether it failed for a reason that
other user agents will fail or if it failed without crashing.

But, meh. We try to detect whether it succeeded in the function call
(as in, didn't crash) but failed in the request itself, and then we
surface it.

I've enabled an unavailable proxy configuration and tested it. This
is what it looks like:

    $ perl Porting/sync-with-cpan CPAN
    Cannot retrieve file: http://www.cpan.org/modules/02packages.details.txt
    Status: 599
    Reason: Internal Exception
    Content: Could not connect to 'proxy.xxx.yyy.com:8080': No address associated with hostname

Removing the proxy configuration, it succeeded.
xsawyerx added a commit that referenced this issue Sep 26, 2020
The original code uses HTTP::Tiny and if it fails, we try alternatives.

This isn't the best strategy because it might fail for legitimate
reasons and we need to separate whether it failed for a reason that
other user agents will fail or if it failed without crashing.

But, meh. We try to detect whether it succeeded in the function call
(as in, didn't crash) but failed in the request itself, and then we
surface it.

I've enabled an unavailable proxy configuration and tested it. This
is what it looks like:

    $ perl Porting/sync-with-cpan CPAN
    Cannot retrieve file: http://www.cpan.org/modules/02packages.details.txt
    Status: 599
    Reason: Internal Exception
    Content: Could not connect to 'proxy.xxx.yyy.com:8080': No address associated with hostname

Removing the proxy configuration, it succeeded.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discovered-thru-p7-research Issue was discovered in the course of research in the core-p7 branch
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants