Skip to content

Use the patches table from the workspace manifest if possible#28

Merged
RalfJung merged 2 commits intoRalfJung:masterfrom
tgross35:detect-patches
Jun 27, 2025
Merged

Use the patches table from the workspace manifest if possible#28
RalfJung merged 2 commits intoRalfJung:masterfrom
tgross35:detect-patches

Conversation

@tgross35
Copy link
Contributor

@tgross35 tgross35 commented Jun 11, 2025

Remove special casing for the compiler-builtins patch by instead picking up the patch table from the workspace Cargo.toml. Paths in the patch table are remapped to be relative to the source directory, which matches current behavior.

@tgross35
Copy link
Contributor Author

I'll run a try build with this once it's closer to ready, but you should probably make sure this is on the right track first.

@RalfJung
Copy link
Owner

Thanks!

TBH, this is a lot heavier than I thought it would be. Though a lot of that seems to be due to using toml for the entire manifest? I am not sure that's worth it. Looking at e.g. the logic in let crates =, that's a lot more verbose now with unclear benefit.

Is it possible to use toml just for dealing with the patches?

src/lib.rs Outdated
Comment on lines 592 to 594
// We only care about the patch table.
t.retain(|k, _v| k == "patch");
Some(t)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be just t["patch"]? Seems odd to keep the surrounding table but erase the rest.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it this way so the existing default_patches could keep the existing patch.crates-io part. But yeah, both could be de-nested by one

src/lib.rs Outdated
path = "rustc-std-workspace-std"
}
};
let mut patches = extract_patches(src_dir).unwrap_or_else(default_patches);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this ever fail? The library toml file always has a patch section, doesn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The library/Cargo.toml file is newer since the workspace split right? I don't know how far back this tool needs to support.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the file got added in rust-lang/rust@1f3be75. I like supporting a few months worth of rustc versions, but that's almost a year, so it's fine requiring the toml file to exist.

@tgross35
Copy link
Contributor Author

Is it possible to use toml just for dealing with the patches?

Sure, I can change it if you prefer. I just liked that the string literals could be indented and we still get a nicely formatted output 🙂 Though I guess we get that just by passing the indented string through toml once.

tgross35 added 2 commits June 13, 2025 05:32
Make things a bit nicer to read in code. This means the entire file will
be indented, but we will wind up passing it through a toml
parser/printer so the nice formatting will come back.
Remove special casing for the `compiler-builtins` patch by instead
picking up the `patch` table from the workspace `Cargo.toml`. Paths in
the patch table are remapped to be relative to the source directory,
which matches current behavior.
@tgross35
Copy link
Contributor Author

I updated things so most of the existing logic except for the patches remains unchanged. The first commit indents things just because we can now without creating a wonky file.

@RalfJung
Copy link
Owner

This looks great, thanks. :)

@RalfJung RalfJung merged commit 1cd0fbb into RalfJung:master Jun 27, 2025
9 checks passed
@tgross35 tgross35 deleted the detect-patches branch June 27, 2025 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants