Support relative URLs, closes #2252

[marcbowes]

See #2252 for a description of the problem.

This PR adds support for having a relative URL to a registry. The registry is considered relative to the .cargo/config file that defined the URL.

[rust-highfive]

r? @huonw

(rust_highfive has picked a reviewer for you, use r? to override)

[marcbowes]

I had to move this up so that support for auto-inserting cargo:// runs before the attempt to parse the URL.

The good_parsing() test (in src/cargo/core/package_id_spec.rs) catches this:

        ok("crates.io/foo", PackageIdSpec {
            name: "foo".to_string(),
            version: None,
            url: Some(url("cargo://crates.io/foo").unwrap()),
        });

Without this change, "crates.io/foo" would be parsed as a relative path to the current directory.

[marcbowes]

I think this is ready for some feedback. The build is failing on nightly, but I don't think that's me.

[huonw]

It seems like it should be relative to the file that contains the URL rather than the directory cargo is invoked in?

[marcbowes]

There are roughly 3 options, I think:

1. Relative to the current working directory (as is). Shouldn't be too surprising to people, but I think it would mean that cargo build would now only work if you're in the project root. I think this is a regression.
2. Relative to the project root.
3. Relative to the file that defines the URL (your suggestion).

I'm most in favor of option 2. However, both 2/3 would require plumbing. In option 3, all calls to to_url would require an additional argument. In option 2, we could use CARGO_MANIFEST_DIR to avoid the additional argument.

[huonw]

r? @alexcrichton

[alexcrichton]

This may be a bit heavy-handed for adding support, as there's a number of locations throughout Cargo which rely on this path returning an error for relative paths.

I'm a little skeptical that we want to fix the issue in question right now because the registry.index key is largely just used for testing rather than intended for general usage. For working with a cached registry, perhaps #2212 would be the best option?

[marcbowes]

@alexcrichton is there something we can do that helps move towards #2212 but isn't quite a full blown implementation of a new registry type? For example, in #2212 I'd imagine there would be either a new protocol (e.g. registry://) or an environment variable (e.g. CARGO_REGISTRY_PATH). If we lay down the entry point for that here but wire it up to the current (git based) registry type, that would be a small amount of work and would make my life a lot easier.

[alexcrichton]

Hm ok, this seems reasonable to land in the meantime then. Would it be possible to isolate this logic to just that of parsing registry.index?

[marcbowes]

Just to confirm (we spoke on IRC): this doesn't require adding a new scheme, just isolating the relative support to just that key.

Would you like this to be relative to the .cargo/config file (as per @huonw) or relative to the cwd?

[alexcrichton]

Ah I'd make it relative to the directory containing the .cargo/config file (like other configuration keys are as well)

[marcbowes]

I took a crack at this. It worked on a test repository but I haven't added tests yet.

Before:

> cat .cargo/config
[registry]
index = "hi/there"

> which cargo
/usr/local/bin/cargo

> cargo build
failed to parse manifest at `/private/tmp/test-relative-registry/Cargo.toml`

Caused by:
  invalid url `hi/there`: relative URL without a base

After

> ~/code/rust/cargo/target/release/cargo build
    Updating registry `file:///private/tmp/test-relative-registry/.cargo/hi/there`
Unable to update registry file:///private/tmp/test-relative-registry/.cargo/hi/there

To learn more, run the command again with --verbose.

Note in the second case it still failed to build (I don't have a test repository setup), but the path was right.

I've started on adding a test but was fighting with support/registry.rs which is pretty gnarly.

[alexcrichton]

Yeah this is looking pretty good to me, let me know if you need any help writing tests! It's also fine to avoid a whole new trait for just one caller, that method can just be inlined somewhere.

[marcbowes]

I was going to inline it and then I wasn't sure if making fn mapper(... public would be frowned upon?

[alexcrichton]

Ah you may want to make a helper method in the url module to create the parser, but that shouldn't be too bad.

[marcbowes]

Good idea. Will do.

I don't need help with the tests, I just ran out of time. Basically, Package::new (in support) calls init which assumes we're going to use the test registry. I just need to create some extra functions to customize.

[bors]

☔ The latest upstream changes (presumably #2486) made this pull request unmergeable. Please resolve the merge conflicts.

[alexcrichton]

Closing due to inactivity, but feel free to resubmit with a rebase!