search

mirage / ocaml-uri

RFC3986 URI parsing library for OCaml
Other
97 stars 57 forks source link

When resolving URI inherit userinfo from the base URI #111

Closed TheCBaH closed 6 years ago

TheCBaH commented 6 years ago

With this change userinfo (Authority) is copied from the base URI: See https://tools.ietf.org/html/rfc3986#section-5.2.2

vbmithr commented 6 years ago

Thanks, could you additionally squash your two patches into only one patch?

TheCBaH commented 6 years ago

Done!

vbmithr commented 6 years ago

@avsm One CI instance keeps failing. Could you have a look?

avsm commented 6 years ago

I suspect that there is a missing opam depext -u flag in order to update the repository source somewhere in the ocaml-ci-scripts. ccing @jpdeplaix in case he has a chance to debug this.

kit-ty-kate commented 6 years ago

Yes, I also believe it's a missing opam depext -u. Could you try to test if it works by adding my local change ? (in .travis.yml)

- install: wget https://raw.githubusercontent.com/ocaml/ocaml-travisci-skeleton/master/.travis-docker.sh
+ install: wget https://raw.githubusercontent.com/jpdeplaix/ocaml-ci-scripts/master/.travis-docker.sh

It it does I will go ahead and open a PR so that it will be fixed for good.

kit-ty-kate commented 6 years ago

Ok, in fact opam depext -u doesn't work if no os-level packages are installed. Here is the build log & fix for this at the ocaml-uri level: https://travis-ci.org/jpdeplaix/ocaml-uri/builds/326787225

(I will restart the builds as soon as the menhir archive is available again) (EDIT: all green)

@avsm I wonder if we should change this behaviour for opam depext -u. I think it makes sense in a context where we want an agnostic command in anticipation for this kind of thing. What do you think ?

avsm commented 6 years ago

@avsm I wonder if we should change this behaviour for opam depext -u. I think it makes sense in a context where we want an agnostic command in anticipation for this kind of thing. What do you think ?

Makes sense to me!

kit-ty-kate commented 6 years ago

This PR should be good to merge if the changes are good for you (see my comment above). To fix the CI for the next PRs simply just merge #115

Poke @vbmithr

vbmithr commented 6 years ago

Fixed via 184eaf32dfd22d3923088440ab8803eb854a0f7c, thanks!