ensure callers of BuildDirectory::run can determine if the source of the error is in Prepare::prepare

[Skgland]

Without this can be difficult to distinguish whether BuildDirectory::run failed due to f failing or due to Prepare::prepare failing.

Related to #98 as it helps with errors not easily sub-categorized to at least get them into the general category of prepare failures.

[Skgland]

Making it a draft while I add tests (rebasing on top of the first two commit of #98)

[Skgland]

Renamed the added PrepareError::Unknown variant to PrepareError::Uncategorized and marked it as #[doc(hidden)] so that it can act similar to std::io::ErrorKind::Uncategorized.

[Skgland]

@syphar ping for review, as it has been two weeks.

[syphar]

Hi!

I see the same issue as with #98, where I don't know much about the new failure cases, and if they are correct.

The team owning this library is our infra-team, so either one of them, or help from the cargo team is necessary.

I can review the rest if someone approves the failure cases and their detection.

[Skgland]

I don't see how cargo is involved in this one. I thought in the other cargo was involved as there was matching on the cargo error messages, but no matching on error messages is done here.

The main functional change is

326ed9f

Were all added test cases go from not having a PrepareError context to having a catch-all PrepareError::Unspecified context.

Given that there's only one place this can come from I would have thought it's clearly correct.

Is there some information/context I can provide to help get this review?

Any info on getting a review from a cargo team member? I tried asking on Zulip under t-cargo but didn't get any response till now.

[syphar]

I don't see how cargo is involved in this one. I thought in the other cargo was involved as there was matching on the cargo error messages, but no matching on error messages is done here.

The main functional change is

326ed9f

Were all added test cases go from not having a PrepareError context to having a catch-all PrepareError::Unspecified context.

Given that there's only one place this can come from I would have thought it's clearly correct.

Is there some information/context I can provide to help get this review?

I can check again, I was coming from the new cargo buildtest cases, where I also would need to understand if the error you're having in the build-test is what you are extracting.

[syphar]

I don't see how cargo is involved in this one. I thought in the other cargo was involved as there was matching on the cargo error messages, but no matching on error messages is done here.

I'm not sure what I'm missing. There are a bunch of new cargo.toml error test cases added, and I can't really judge if all is correct here.

[Skgland]

Ah, I think I misunderstood what the problem was and oversimplified my explanation, sorry.

For this PR they don't really make a difference so I could remove all but one.
They basically all currently fail with an error without PrepareError context and this is tested in the commit adding the tests. After the change they all fail but now with PrepareError::Unspecified.
While the tests definition includes the error message that is expected in the std out it's currently not checked (we don't have the stdout neither before nor after the change to compare against).

I would expect checking out the commit adding the tests and running them should confirm that the tests have a correct baseline expectation. And one could just run cargo check for each crate and check that the output contains the expected test. The updated expectations will be checked in CI as part of the test run.

While yes cargo as a tool is involved I don't see where this requires special cargo expertise.