Limit registry-index dependency field to registry sources only #16293
Conversation
rustbot
added
A-manifest
|
r? @weihanglo rustbot has assigned @weihanglo. Use |
moabo3li
force-pushed
the
limit_registry_index
branch
2 times, most recently
from
fcbe47d to
dfc940d
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
…n user manifests
moabo3li
force-pushed
the
limit_registry_index
branch
from
dfc940d to
ab36df1
Compare
src/cargo/util/toml/mod.rs
Outdated
| // Check if this is a packaged manifest (in target/package or target\package) | ||
| // by checking if the path contains the pattern | ||
| let path_str: Cow<'_, str> = manifest_ctx.root.to_string_lossy(); | ||
| let is_packaged_manifest = | ||
| path_str.contains("target/package") || path_str.contains("target\\package"); |
There was a problem hiding this comment.
cargo package packages are not guaranteed to be at that location (and we have a pending PR for making cargo publish never have them there) and user packages can be at that location
There was a problem hiding this comment.
I added a new read_cargo_generated_manifest function alongside the existing read_manifest. This new function passes a cargo_generated flag down to to_real_manifest_impl. During package verification, the code now uses PathSource::root_cargo_generated_package() to read the extracted manifest with this flag set to true, which allows the internal-only registry-index field to pass validation.
What do you think?
There was a problem hiding this comment.
I'm concerned about how invasive this is and the impact of each of these pieces needing to work together just right and how that might hit up against future changes. Compared to the risks of not erroring with registry-index, I'm unsure if this adds up.
I opened #15503 and some times we don't know where a change will go until it happens. I am very much grateful for the work you put into this and what we've learned from this. I'd like to get a second opinion from @weihanglo before deciding whether to stop work or continue this. If we decide not to move forward with this, I am sorry for all of the time you put into this for this not actually getting in.
There was a problem hiding this comment.
No problem, thank you for the review.
There was a problem hiding this comment.
Yeah, while this seems working well (and thanks for your efforts!), it is indeed invasive and perhaps fragile as the logic touches too many different component.
I wonder if we should just a bigger warning in cargo-util-schemas doc, and saying that there is no stability guarantee. Something like
There was a problem hiding this comment.
I wonder if we should just a bigger warning in
cargo-util-schemasdoc, and saying that there is no stability guarantee. Something like
Ok, I can open another PR for that if you’d like, and do this.
moabo3li
force-pushed
the
limit_registry_index
branch
from
ab36df1 to
1698cef
Compare
rustbot
added
A-workspaces
moabo3li
force-pushed
the
limit_registry_index
branch
from
1698cef to
861f654
Compare
4 participants
registry-indexis an internal-only field that Cargo uses whenpackaging crates. It should not appear in user-written Cargo.toml
files as it allows bypassing the documented pattern of using
registry = "name"with.cargo/config.toml.Fixes #15503