Provide extra context on a query failure.

[ehuss]

This adds error context when a query fails, primarily to tell you which parent package included the dependency that failed. For example, imagine deep within your dependency graph you have a git dependency, and it fails to download. The current error doesn't tell you where in the graph that git dependency was included.

I also slightly tweaked the failed to load source error message. I felt like the existing wording could be misinterpreted that it was an error loading the dependency for the given package. I felt like there were multiple ways to interpret it, so I tried to simplify it to avoid that possibility.

[rust-highfive]

r? @Eh2406

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

[ehuss]

This is just an experiment, and I wanted to get feedback if anyone thinks it looks like a good idea. I'm also open to changing the wording.

[Eh2406]

seems reasonable.

[alexcrichton]

It sounds like the goal here is to sort of figure out why a package was included, right? If so I think 1 package as a parent helps a lot, but perhaps we could go all the way and do the path_to_root method which prints everything?

[ehuss]

A full path might be nice, but I don't know how to do that at this location. I assume you're referring to path_to_bottom? It looks like that requires Context.parents? It looks like I could maybe pass something in, but I'm very unfamiliar with this, so I'm not clear on the relationship of the "candidate" and the parents.

[Eh2406]

Yes, I think something like

describe_path(
                &cx.parents.path_to_bottom(&parent.package_id()),
            )

Will make a message that matches our other errors. Looks like we can pass cx from the activate.

Or perhaps that can be added as part of a chain_err on the result of build_deps in activate, I don't know the capabilities of anyhow.

cc #6199

[alexcrichton]

I think that if the error context was shifted to around here it may be easier to call that describe_path method perhaps? (poking a bit in the dark myself here)

[ehuss]

Hm, I pushed a change that moves the context up so that it can get the path. Unfortunately build_deps also handles feature validation, and the error messages in those cases were worse off (that's not necessarily an error loading a dependency). So I added a special variant to ActivateError to detect that case.

I'm not really sure it's worth it, it seems to complicate things a little. What do you think?

[Eh2406]

Hmmm, I was more thinking of the smaller change like master...Eh2406:pull/7934#diff-995a0a0a73b467f7c6b0cb34d8fba39d (I did not fix the tests.)

[ehuss]

I was more thinking of the smaller change like

That looks good. (The link is broken, but I think this link works: Eh2406@d3f712a) I'll try it out tomorrow.

[ehuss]

Thanks, updated with @Eh2406 suggestion, it looks much better.

[Eh2406]

Looks great! Thanks for adding the new test!
@bors r+

[bors]

📌 Commit 1eca786 has been approved by Eh2406

[bors]

⌛ Testing commit 1eca786 with merge 7ba6e49...

[bors]

☀️ Test successful - checks-azure
Approved by: Eh2406
Pushing 7ba6e49 to master...