Add location information in patch error messages #14473

antoniospg · 2024-08-31T12:59:47Z

What does this PR try to resolve?

Similar to what was explained in the issue page, I've tried to preserve location information in Workspace::root_patch and use it in registry::patch to improve the error messages.
I've also updated the tests to take this change in consideration.

rustbot · 2024-08-31T12:59:52Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

epage · 2024-09-03T20:25:25Z

tests/testsuite/patch.rs

@@ -1437,7 +1437,7 @@ fn replace_with_crates_io() {
        .with_status(101)
        .with_stderr_data(str![[r#"
 [UPDATING] `dummy-registry` index
-[ERROR] failed to resolve patches for `https://github.com/rust-lang/crates.io-index`
+[ERROR] failed to resolve patches for `https://github.com/rust-lang/crates.io-index` specified in `[ROOT]/foo/Cargo.toml`


Please make all commits atomic which includes all tests must pass for each commit

epage · 2024-09-03T20:28:19Z

src/cargo/core/resolver/encode.rs

+        for dep in deps.iter().map(|(d, _)| d) {
            build_dep(dep, ws, &mut ret, &mut visited);
        }


I feel like for (dep, _) in deps would be cleaner.

epage · 2024-09-03T20:34:00Z

src/cargo/core/workspace.rs

+        let patch_cv = self.gctx.get_cv_with_env(&ConfigKey::from_str("patch"))?;
+        let manifest_path = PathOrDefinition::Path(self.root_manifest().into());
+
+        let patch_from_manifest = match self.root_maybe() {
            MaybePackage::Package(p) => p.manifest().patch(),
            MaybePackage::Virtual(vm) => vm.patch(),
        };

-        let from_config = self.config_patch()?;
+        // Insert location information for each dep in the patch
+        let mut from_config: HashMap<_, Vec<_>> = HashMap::new();
+        for (url, dep) in self.config_patch()? {
+            from_config.insert(
+                url,
+                dep.iter()
+                    .map(|dep| {
+                        (
+                            dep.clone(),
+                            PathOrDefinition::Definition(
+                                patch_cv.as_ref().unwrap().definition().clone(),
+                            ),
+                        )
+                    })
+                    .collect(),
+            );
+        }
+
+        let mut from_manifest: HashMap<_, Vec<_>> = HashMap::new();
+        for (url, dep) in patch_from_manifest {
+            from_manifest.insert(
+                url.clone(),
+                dep.iter()
+                    .map(|dep| (dep.clone(), manifest_path.clone()))
+                    .collect(),
+            );
+        }
+


This was

manifest

config

config

manifest

But became

config

manifest

manifest

config

manifest

config

manifest

Already the original code wasn't idea, it should either be completely parallel or consistently interleaved. As the code grows, I would suggest interleaving makes the code harder to follow and we should be completely parallel.

epage · 2024-09-03T20:34:42Z

src/cargo/ops/resolve.rs

@@ -813,7 +813,7 @@ fn register_patch_entries(
 ) -> CargoResult<HashSet<PackageId>> {
    let mut avoid_patch_ids = HashSet::new();
    for (url, patches) in ws.root_patch()?.iter() {
-        for patch in patches {
+        for patch in patches.iter().map(|(d, _)| d) {


I feel like for (patch, _) in patches would be cleaner.

epage · 2024-09-03T20:35:22Z

src/cargo/ops/resolve.rs

+        for dep_with_location in patches {
+            let (dep, _) = dep_with_location;


I think I'd prefer destructuring the tuple in the for and then re-creating it in the push

epage · 2024-09-03T20:40:27Z

src/cargo/core/workspace.rs

+        let mut from_config: HashMap<_, Vec<_>> = HashMap::new();
+        for (url, dep) in self.config_patch()? {
+            from_config.insert(
+                url,
+                dep.iter()
+                    .map(|dep| {
+                        (
+                            dep.clone(),
+                            PathOrDefinition::Definition(
+                                patch_cv.as_ref().unwrap().definition().clone(),
+                            ),
+                        )
+                    })
+                    .collect(),
+            );
+        }
+
+        let mut from_manifest: HashMap<_, Vec<_>> = HashMap::new();
+        for (url, dep) in patch_from_manifest {
+            from_manifest.insert(
+                url.clone(),
+                dep.iter()
+                    .map(|dep| (dep.clone(), manifest_path.clone()))
+                    .collect(),
+            );
+        }


Should this be done in the functions we call to get a patch? Please understand, this is a serious question and not an indirect way of saying what to do.

For manifests, its a little weird because it will always be the same path. However, it feels weird to be calling back into config to get the location. It feels like we are re-implementing things. Calling back into config can also have some negative performance implications.

Not sure if this is what you said, but maybe it's better to move theget_cv_with_envcall to Workspace::config_patch as it's already calling the config there with the following:

let config_patch: Option< BTreeMap<String, BTreeMap<String, TomlDependency<ConfigRelativePath>>>, > = self.gctx.get("patch")?;

But it would still have the negative performance implications. I'm not sure if there's a better way to get the config location.

epage · 2024-09-03T20:40:44Z

src/cargo/core/workspace.rs

@@ -506,13 +528,43 @@ impl<'gctx> Workspace<'gctx> {
    /// Returns the root `[patch]` section of this workspace.
    ///
    /// This may be from a virtual crate or an actual crate.
-    pub fn root_patch(&self) -> CargoResult<HashMap<Url, Vec<Dependency>>> {
-        let from_manifest = match self.root_maybe() {
+    pub fn root_patch(&self) -> CargoResult<HashMap<Url, Vec<(Dependency, PathOrDefinition)>>> {


I'm a bit mixed on tuple vs struct.

We could have a struct like:

#[derive(Clone, Debug)] pub struct Patch { dep: Dependency, location: PathOrDefinition, }

and have the PathOrDefinition::get_location_string implemented for it instead. But I'm not sure what would be the other benefits, as we would still need to destructure it in a lot of places or keep using iter().map(|patch| patch.dep) instead. But I'm open for better ideas.

Between rust-lang/rust#126810 and rust-lang/rust#126810 the output of rustc for resolution errors is going to change in such a way that some existing cargo tests will fail. Change them to support both the current and future output, so that those PRs can land in `rustc`.

link flags, fixes rust-lang#14330. Co-authored-by: Weihang Lo <weihanglo@users.noreply.github.com>

In a discussion on an issue, it became confusing to talk about "resolved" manifests and dependency resolution, so I'm switching manifests to use the other term I considered, "normalized".

Starting from rust-lang/rust#128534 (nightly-2024-08-05), stnadard library has its own Cargo workspace. Hence `-Zbuild-std` no longer need to fake a virtual workspace. This also adjusts Cargo.toml in `mock-std` to align with std's Cargo.toml

It was designed for `-Zbuild-std`

rustbot assigned ehuss Aug 31, 2024

rustbot added A-dependency-resolution Area: dependency resolution and the resolver A-lockfile Area: Cargo.lock issues A-registries Area: registries A-workspaces Area: workspaces S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 31, 2024

antoniospg changed the title ~~Preserve loc patches~~ Add location information in patch error messages Aug 31, 2024

epage reviewed Sep 3, 2024

View reviewed changes

antoniospg force-pushed the preserve_loc_patches branch from 2730b80 to 00f0c08 Compare September 8, 2024 01:02

rustbot added A-build-execution Area: anything dealing with executing the compiler A-semver Area: semver specifications, version matching, etc. Command-update labels Sep 8, 2024

renovate bot and others added 11 commits September 7, 2024 22:09

chore(deps): update rust crate gix to 0.64.0

f6c1d8b

chore(deps): update rust crate windows-sys to 0.59

3506d1c

Fix a typo in 1.81 changes

b5d1a27

docs(unstable): Link the nightly unstable page, not a specific feature

48849fe

docs(unstable): Make the nightly link standout a bit more

488f8dc

faq: rephrase offline usage.

9c0c0be

link flags, fixes rust-lang#14330. Co-authored-by: Weihang Lo <weihanglo@users.noreply.github.com>

refactor(toml): Rename 'resolved' to 'normalized'

a8b72e2

In a discussion on an issue, it became confusing to talk about "resolved" manifests and dependency resolution, so I'm switching manifests to use the other term I considered, "normalized".

test(build-std): expand assertion to ensure no index update

dd28870

refactor: remove unused Workspace::new_virtual

693fbf3

It was designed for `-Zbuild-std`

antoniospg closed this Sep 8, 2024

antoniospg deleted the preserve_loc_patches branch September 8, 2024 01:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add location information in patch error messages #14473

Add location information in patch error messages #14473

antoniospg commented Aug 31, 2024

rustbot commented Aug 31, 2024

epage Sep 3, 2024

epage Sep 3, 2024

epage Sep 3, 2024

epage Sep 3, 2024

epage Sep 3, 2024

epage Sep 3, 2024

antoniospg Sep 8, 2024

epage Sep 3, 2024

antoniospg Sep 8, 2024

		for dep_with_location in patches {
		let (dep, _) = dep_with_location;

Add location information in patch error messages #14473

Add location information in patch error messages #14473

Conversation

antoniospg commented Aug 31, 2024

What does this PR try to resolve?

rustbot commented Aug 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment