search

Perl5-Alien / Alien-Base

Base classes for Alien:: modules (deprecated, see Alien-Build)
Other
35 stars 19 forks source link

build_uri: fix URI composition #144

Closed salva closed 8 years ago

salva commented 8 years ago

Rely as much as possible into the URI module when composing two URIs.

Before this patch, the code used was too simplistic and only covered the more basic cases. Specifically it failed to handle the links from GitHub release pages.

Also, one of the tests in http_uri.t was wrong.

plicease commented 8 years ago

Do you have an example Alien that pulls from GitHub. I am more or less in agreement on this, but would like to try it out.

salva commented 8 years ago

@plicease: https://github.com/salva/p5-Alien-Libssh2 - It is still a WIP.

Regarding commit eb5f59a, it generalizes the code using the URI object to extract the filename part from the URI as it was already been done for absolute references.

plicease commented 8 years ago

Thanks. I will take a closer look later this week, probably Thursday or Friday.

plicease commented 8 years ago

This fails on windows:

https://ci.appveyor.com/project/plicease/alien-base/build/1.0.5

salva commented 8 years ago

ummmh, it works on my Windows machine!

It looks as if the tests were being run against an Alien::Base without the patch in this PR... and actually, it says Creating new 'Build' script for 'Alien-Base' version '0.024', but on this branch the version is still 0.023_01!

plicease commented 8 years ago

I believe you! I will do some checking today if I can figure out if it is a real thing or not.

As for the version, this was run against this branch:

https://github.com/Perl5-Alien/Alien-Base/commits/absuri

which are your changes rebased on top of the current master.

plicease commented 8 years ago

Works now, I had a bug in the configuration:

https://ci.appveyor.com/project/plicease/alien-base/build/1.0.7

plicease commented 8 years ago

I've tried this on my Alien distros and confirmed that it works better for Alien::Libssh2 for me on my system as well. In general since we are already requiring the URI module we should be using it for any URL manipulations since it handles a number of edge cases that typical ad-hoc code will not. Aye.

I will be merging this first thing Tuesday and doing a dev release, unless somebody raises an issue against.

plicease commented 8 years ago

merged