[diagnostics] Errors for failure to parse a manifest in a git dependency are lost

[Ekleog]

Disclaimer: this has been tested only for cargo 1.26.0 (0e7c5a931 2018-04-06), I don't have another cargo at hand right now.

Problem

The error message is unhelpful: it only says that the crate wasn't found:

    Updating git repository `https://github.com/rust-lang-nursery/futures-rs`
error: no matching package named `futures-util-preview` found                   
location searched: https://github.com/rust-lang-nursery/futures-rs?rev=b7769180b091f22da12d35ea808779ddfa17b7e3
required by package [...]`

Steps

Have this in a manifest on cargo stable:

[dependencies.futures-preview]
git = "https://github.com/rust-lang-nursery/futures-rs"
rev = "b7769180b091f22da12d35ea808779ddfa17b7e3"

Possible Solution(s)

Improve the error message

Notes

Output of cargo version:

cargo 1.26.0 (0e7c5a931 2018-04-06)

[Eh2406]

Thanks for the report! I agree it is pretty minimal, what more information would make it more helpful!

[ehuss]

I would imagine in this case it could say something like:

error: unable to load dependency `futures-util-preview` at (LOCATION)

Caused by:
  feature `edition` is required

this Cargo does not support nightly features, but if you
switch to nightly channel you can add
`cargo-features = ["edition"]` to enable this feature

Right now it seems to consider parsing a Cargo.toml as a pure pass/fail, and if it fails it is ignored. There probably should be more nuance ("found what you're looking for, but there is a problem").

[Ekleog]

I would imagine in this case it could say something like: [...] Right now it seems to consider parsing a Cargo.toml as a pure pass/fail, and if it fails it is ignored. There probably should be more nuance ("found what you're looking for, but there is a problem").

Well, actually when downloading the crate from crates.io, it doesn't fail the same way (sorry for not having mentioned it in the bug report, it was in my head but I didn't think of writing it while following the sections). It already includes the error report. Now, in crates.io there's a single Cargo.toml, while on such a git dep on a workspace there are multiple Cargo.toml to consider, so it's maybe harder to do?

[ehuss]

So looking at it closer, it looks like this is a special case where just one Cargo.toml in the git repo parsed successfully (testcrate). If you take it out, you get a better error message:

error: failed to load source for a dependency on `futures-preview`

Caused by:
  Unable to update https://github.com/rust-lang-nursery/futures-rs?rev=b7769180b091f22da12d35ea808779ddfa17b7e3

Caused by:
  failed to parse manifest at `/Users/eric/.cargo/git/checkouts/futures-rs-4ca77cb4f4f05ac4/b776918/futures-sink/Cargo.toml`

Caused by:
  editions are unstable

Caused by:
  feature `edition` is required

this Cargo does not support nightly features, but if you
switch to nightly channel you can add
`cargo-features = ["edition"]` to enable this feature

The code here will only report the errors if every manifest fails. That might be tricky to fix, because it intentionally ignores manifests it can't parse.

I'm not sure the best way to deal with it. One idea is to maybe print warnings for manifests that fail to parse, but that might be very noisy. Perhaps @alexcrichton has ideas?

[Ekleog]

I think ideally (not considering implementation complexity, I haven't checked the code), the code would: * Check each manifest one by one: * As soon as a manifest parses and matches, accept it * If a manifest doesn't parse, record an error * If no manifest matched, output the generated errors as hints

[ehuss]

Here is a test in cargo's testsuite that demonstrates this problem:

#[cargo_test]
fn shows_error_for_bad_manifest() {
    // Checks for an error when a git dependency has multiple manifests, and the one you want fails to load.
    let git_project = git::new("git_proj", |project| {
        project
            .file(
                "git1/Cargo.toml",
                r#"
                    [package]
                    name = "git1"
                    version = "0.1.0"

                    this syntax is not valid
                "#,
            )
            .file("git1/src/lib.rs", "")
            .file("git2/Cargo.toml", &basic_lib_manifest("git2"))
            .file("git2/src/lib.rs", "")
    });

    let p = project()
        .file(
            "Cargo.toml",
            &format!(
                r#"
                    [package]

                    name = "foo"
                    version = "0.1.0"

                    [dependencies.git1]
                    git = '{}'
                "#,
                git_project.url()
            ),
        )
        .file("src/lib.rs", "")
        .build();

    p.cargo("check")
        .with_status(101)
        // FIXME: This error message is not very good. It loses the error from parsing git1.
        .with_stderr(
            "\
[UPDATING] git repository [..]
error: no matching package found
searched package name: `git1`
perhaps you meant:      git2
location searched: file:///[..]
required by package `foo v0.1.0 [..]`
",
        )
        .run();
}

Somehow, read_packages should probably surface all the errors it encountered to the caller so that if the caller can't find the package it is looking for, it could display all the errors. That might be hard, since the callers currently don't know which packages it is looking for.

[epage]

As part of our diagnostic work, I've been wanting to make manifest parsing use "error recovery", meaning the return type could be something like read(...) -> (Manifest, Vec<Errors>). That would help with at least being able to introspect about a package (get its name, etc) even if a failure occurs.