Package ID specification urls must contain a host

[weihanglo]

Resolves #9041

Not sure which commit breaks this. Cargo shipped with rust 1.46 didn't unwrap on a None but 1.47 did. Even checkouted to 149022b (cargo of rust 1.46), it still unwrap unexpectedly.

So I ended up following the Specification grammer to make sure there is a host in the pkgid urls.

See console output

cargo of rust 1.46

$ cargo +1.46 -vV
cargo 1.46.0 (149022b1d 2020-07-17)
release: 1.46.0
commit-hash: 149022b1d8f382e69c1616f6a46b69ebf59e2dea
commit-date: 2020-07-17

$ cargo +1.46 pkgid /path
error: package ID specification `/path` matched no packages

cargo of rust 1.47

$ cargo +1.47 -vV
cargo 1.47.0 (f3c7e066a 2020-08-28)
release: 1.47.0
commit-hash: f3c7e066ad66e05439cf8eab165a2de580b41aaf
commit-date: 2020-08-28

$ cargo +1.47 pkgid /path
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', src/tools/cargo/src/cargo/core/package_id_spec.rs:234:50
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

cargo on commit 149022b

$ git checkout 149022b1d8f382e69c1616f6a46b69ebf59e2dea
$ cargo run -- pkgid /path
   Compiling cargo-platform v0.1.1 ([..]/cargo/crates/cargo-platform)
   Compiling crates-io v0.31.1 ([..]/cargo/crates/crates-io)
   Compiling cargo v0.47.0 ([..]/cargo)
    Finished dev [unoptimized + debuginfo] target(s) in 22.90s
     Running `target/debug/cargo pkgid /path`
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', src/cargo/core/package_id_spec.rs:234:50
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[rust-highfive]

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

[weihanglo]

Oops. Seems some url like file:///path/#bar is valid in some scenarios. Should we enforce the "must have a host" rule or having a "fallback to a empty host" is enough?

[ehuss]

Thanks for taking a look at this. Unfortunately, specs are kind of a mess right now (see #7725). The cargo: protocol probably needs to just be removed. Once you do that, I think it should error correctly when given a bare path.

It might be nice to detect the scenario someone gives a path and provide a better error message (like maybe suggest using file://).

[weihanglo]

Updated. However it still unwraps a None if someone passes cargo:///path without a host deliberately. Should this case be handle by a empty host as a fallback?

[ehuss]

So, I was thinking it could flip the :// check around, maybe something like this:

if spec.contains("://") {
    if let Ok(url) = spec.into_url() {
        return PackageIdSpec::from_url(url);
    }
} else if spec.contains('/') || spec.contains('\\') {
    let abs = std::env::current_dir().unwrap_or_default().join(spec);
    if abs.exists() {
        let maybe_url = Url::from_file_path(abs)
            .map_or_else(|_| "a file:// URL".to_string(), |url| url.to_string());
        bail!(
            "package ID specification `{}` looks like a file path, \
            maybe try {}",
            spec,
            maybe_url
        );
    }
}

And completely remove the cargo:// stuff, including the check for cargo in Display. The check for startswith / will only catch absolute paths on Unix, which I think is a little too narrow.

The error I wrote above still isn't great. Ideally it would actually tell you what the correct value is. query_str is capable of doing that, so if you are interested in trying to provide an accurate error message, that could be nice, though that would be a fair bit more work.

The tests need to be updated, and ideally there would be a UI test (maybe just add another case to suggestion_bad_pkgid).

Also, the documentation should be updated (cargo-pkgid.md, then rebuild the man pages).

[weihanglo]

@ehuss
All you suggestions are patched!
Not sure what part of document should be refined, so I only add an example for querying local package's pkgid.

[ehuss]

Thanks!

Sorry, I think I was mistaken on the docs, but your change seems fine. I was thinking of the pkgid-spec.md file. I'll follow up with an update to that.

@bors r+

[bors]

@ehuss: 🔑 Insufficient privileges: Not in reviewers

[ehuss]

@bors r+

[bors]

📌 Commit c5d2304 has been approved by ehuss

[bors]

⌛ Testing commit c5d2304 with merge 0f3f47e...

[bors]

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 0f3f47e to master...

[ehuss]

Posted #9249 with an update to the spec docs that I was thinking of.