Optional: Consider using File::Fetch for HTTP code #183

Open
wants to merge 1 commit into
from

Projects

None yet

3 participants

@kentfredric
Contributor
  • File::Fetch is fatpackable
  • File::Fetch supports more backends
  • File::Fetch is available already on most perls, and makes fatpacking not completely nessecary

This code sample is probably not incredibly useful to you, and what you already do is probably fine for you.

However, in the off chance you hadn't seen/considered using File::Fetch to do the legwork of backend HTTP, here is an example that is a workable compatible option.

@kentfredric
Contributor

I also implemented this patch to keep the backend and resolve order feature compatible, but it arbitrarily limits the backends that could be used this way, and I would personally rather support more backends, not less.

@miyagawa
Owner
miyagawa commented Jan 2, 2013

Looks good - Can you not edit the cpanm and redo the pull request? I do the fatpack and regenerating cpanm as a release process, not in the series of commits/pull requests.

@miyagawa
Owner
miyagawa commented Jan 2, 2013

You can push -f a new change to the same branch and this pull will be automatically updated.

@kentfredric kentfredric Refactor code to use File::Fetch to handle automatic backends for HTTP.
* File::Fetch is fatpackable
* File::Fetch supports more backends
* File::Fetch is available already on most perls, and makes fatpacking not completely nessecary
63d10a9
@kentfredric
Contributor

Sorry for the delay, split the File::Temp work around into its own pull req, and this is now based on that, both are without changes to /cpanm itself now.

@miyagawa
Owner
miyagawa commented Jan 6, 2013

cherry-picked only the changes to script.pm at a5c8775

@miyagawa miyagawa closed this Jan 6, 2013
@miyagawa
Owner
miyagawa commented Jan 6, 2013
➜  cpanminus git:(devel) ✗ perl -Ilib script/cpanm.PL --scandeps --reinstall Bundle::DBI
'lwp' said it fetched 'FileFetch.xIWU6M/META.yml', but it was not created at /Users/miyagawa/dev/cpanminus/script/../lib/App/cpanminus/script.pm line 1837.
'wget' said it fetched 'FileFetch.xIWU6M/META.yml', but it was not created at /Users/miyagawa/dev/cpanminus/script/../lib/App/cpanminus/script.pm line 1837.
'curl' said it fetched 'FileFetch.xIWU6M/META.yml', but it was not created at /Users/miyagawa/dev/cpanminus/script/../lib/App/cpanminus/script.pm line 1837.'httptiny' said it fetched 'FileFetch.xIWU6M/META.yml', but it was not created at /Users/miyagawa/dev/cpanminus/script/../lib/App/cpanminus/script.pm line 1837.

This breaks some of the bundle features.

➜  cpanminus git:(devel) ✗ prove -lbr xt/bundle.t                                
xt/bundle.t .. 1/? 
#   Failed test at xt/bundle.t line 6.
#                   'DBI-1.623
# \_ DBI-Shell-11.95
#  \_ IO-Tee-0.64
# '
#     doesn't match '(?^:DBD-Multiplex)'
# Looks like you failed 1 test of 1.
xt/bundle.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/1 subtests 
@miyagawa miyagawa reopened this Jan 6, 2013
@kentfredric
Contributor

Odd, running it manually I discover something interesting,

That URL for IO::Tee is returning an empty body, not 404ing :(

curl -v 'http://search.cpan.org/meta/IO-Tee-0.64/META.yml'
* About to connect() to search.cpan.org port 80 (#0)
*   Trying 194.106.223.155...
* connected
* Connected to search.cpan.org (194.106.223.155) port 80 (#0)
> GET /meta/IO-Tee-0.64/META.yml HTTP/1.1
> User-Agent: curl/7.28.1
> Host: search.cpan.org
> Accept: */*
> 
< HTTP/1.1 200 OK
< Date: Mon, 07 Jan 2013 21:53:47 GMT
< Server: Plack/Starman (Perl)
< Content-Type: text/yaml
< Via: 1.1 BC5-ACLD
< Content-Length: 0
< Connection: Keep-Alive
< Age: 0
< 
* Connection #0 to host search.cpan.org left intact
* Closing connection #0
[kent@k2 tmp]$ 
@miyagawa
Owner
miyagawa commented Jan 7, 2013

Interesting. Be it a bug in the CPAN server or cpanminus or File::Fetch, this breaks the compatibility and at least scandeps code, and has to be fixed/worked around.

@kentfredric
Contributor

Indeed, I think its at least a bug that File::Fetch reports HTTP 200 + 0 Bytes as an error condition, and have filed a bug to that effect: https://rt.cpan.org/Ticket/Display.html?id=82549 , though, given how File::Fetch works and why it does this, its more understandable that you wouldn't want to use it in the given state.

@doherty
doherty commented Mar 7, 2013

Is there a reason we didn't want to use the core module HTTP::Tiny?

@miyagawa
Owner
miyagawa commented Mar 7, 2013

We do fallback to HTTP::Tiny when no other alternatives are available.

If you are asking why we don't use HTTP::Tiny all the time, that is because HTTP::Tiny doesn't support SSL.

@miyagawa
Owner
miyagawa commented Mar 7, 2013

HTTP::Tiny isn't core till 5.14, but doesn't matter if one module is core or not, as long as we fatpack it (we do for HTTP::Tiny).

@miyagawa
Owner
miyagawa commented May 7, 2013

Any updates on the File::Fetch side? Would love to eliminate all the util code if we can replace with File::Fetch.

@kentfredric
Contributor

File::Fetch still doesn't really support whats needed, and it really needs to be somewhat reworked for a subset of fetch methods that have a way of getting headers out, and get those headers out and process them properly.

Without that, I doubt there'll be much progress in the near future, because it doesn't seem like there's a reliable way to determine "404" vs "Empty file" without headers.

@kentfredric
Contributor

IMHO, at very least, _*_fetch need to return some sort of basic response object indicating success/fail, because as it is, the calling context determines that, which is kinda stupid really.

https://metacpan.org/source/BINGOS/File-Fetch-0.42/lib/File/Fetch.pm#L500

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