-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
doc: state PackageId
/SourceId
string is opaque
#12313
Conversation
r? @ehuss (rustbot has picked a reviewer for you, use r? to override) |
src/doc/man/cargo-metadata.md
Outdated
/* The Package ID, a unique identifier for referring to the package. | ||
Consumers should generally see it as an opaque string. There is | ||
no guarantee of the format across different versions of Cargo. | ||
*/ | ||
"id": "my-package 0.1.0 (path+file:///path/to/my-package)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cargo_metadata
does make mention that PackageId
is opaque
An “opaque” identifier for a package. It is possible to inspect the repr field, if the need arises, but its precise format is an implementation detail and is subject to change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A part of me would love for us to exercise this opacity once: to switch from PackageId
to PackageIdSpec
which would resolve #7267.
src/doc/man/cargo-metadata.md
Outdated
Consumers should generally see it as an opaque string. There is | ||
no guarantee of the format across different versions of Cargo. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we should maybe be a little more specific about the guidance for the source string. The lines just above this explicitly state the format of the string, so having this statement about it being opaque seems a little confusing since it contradicts what came just before it.
I think we can probably safely commit to source id's having an <kind>+
prefix, but leaving the rest to be a little less defined.
Perhaps something along the lines of:
The value in the source after the
+
is not explicitly defined, and may change between versions of Cargo and may not directly correlate to other things, such as registry definitions in a config file. The entire source string should generally be treated as an opaque string to differentiate packages (such as those with the same name, but coming from different sources). New source kinds may be added in the future which will have different+
prefixed identifiers.
(That wording could probably be cleaned up a little.)
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and removed the entire mention of <kind>+
. I think if we want to commit to that, we need to list all possible kinds and explain that. Not sure if we want to do it now so remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it already lists all kinds (all kinds that can appear in cargo metadata
). The other kinds can't be exposed here AFAIK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can put it back then.
I was wondering local-registry
and directory
but they won't show up in cargo metadata
as they appears only in source replacment scenario. However, should we mention sparse
as they are already available for alternative registries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yea, I forgot about sparse. That should probably be added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
Also added a paragraph to clarify the compatibility. Please tell me if it is too much.
2c4064a
to
ae350f2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great to me. Just some minor wording comments.
Cargo tries to maintain the stability of every output it emits. However, for things like `PackageId` and `SourceId` users should see them as opaque strings. We generally don't change it but we want to reserve the right to change when it is required.
Updated. Thanks for the suggestions! |
Thanks! @bors r+ |
☀️ Test successful - checks-actions |
Update cargo 10 commits in 45782b6b8afd1da042d45c2daeec9c0744f72cc7..694a579566a9a1482b20aff8a68f0e4edd99bd28 2023-07-05 16:54:51 +0000 to 2023-07-11 22:28:29 +0000 - fix(embedded): Always generate valid package names (rust-lang/cargo#12349) - fix(embedded): Error on unsupported commands (rust-lang/cargo#12350) - chore(ci): Automatically test new packages by using `--workspace` (rust-lang/cargo#12342) - contrib docs: Add some more detail about how publishing works (rust-lang/cargo#12344) - docs: Put cargo-add change under nightly (rust-lang/cargo#12343) - Minor: Use "number" instead of "digit" when explaining Cargo's use of semver (rust-lang/cargo#12340) - Update criterion (rust-lang/cargo#12338) - Add profile strip to config docs. (rust-lang/cargo#12337) - update re: multiple versions that differ only in the metadata tag (rust-lang/cargo#12335) - doc: state `PackageId`/`SourceId` string is opaque (rust-lang/cargo#12313) r? `@ghost`
Update cargo 10 commits in 45782b6b8afd1da042d45c2daeec9c0744f72cc7..694a579566a9a1482b20aff8a68f0e4edd99bd28 2023-07-05 16:54:51 +0000 to 2023-07-11 22:28:29 +0000 - fix(embedded): Always generate valid package names (rust-lang/cargo#12349) - fix(embedded): Error on unsupported commands (rust-lang/cargo#12350) - chore(ci): Automatically test new packages by using `--workspace` (rust-lang/cargo#12342) - contrib docs: Add some more detail about how publishing works (rust-lang/cargo#12344) - docs: Put cargo-add change under nightly (rust-lang/cargo#12343) - Minor: Use "number" instead of "digit" when explaining Cargo's use of semver (rust-lang/cargo#12340) - Update criterion (rust-lang/cargo#12338) - Add profile strip to config docs. (rust-lang/cargo#12337) - update re: multiple versions that differ only in the metadata tag (rust-lang/cargo#12335) - doc: state `PackageId`/`SourceId` string is opaque (rust-lang/cargo#12313) r? ``@ghost``
What does this PR try to resolve?
In #12280 we found that
SourceId
serialization might be out-of-syncwith different lockfiles. However, we don't really want people to rely
on the exact format of serialized
PackageId
andSourceId
. Unluckily,cargo metadata
already emits such a format.This PR tries its best to inform uses that there is no guarantee of
their formats across Cargo versions.
How should we test and review this PR?
This is doc-only change, though it is a bit controversial that we use
strong words to state the instability.
Additional information
Some follow-up questions:
--format-version
gives?--format-version
for such change in fix: encode URL params correctly for SourceId in Cargo.lock #12280. I personally don't want though.cargo metadata
we explainssource
format quite a lot. Should we remove it or guarantee that some part of it is quite stable?