-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
feat(package): add unstable --message-format
flag
#15311
Conversation
src/cargo/ops/cargo_package/mod.rs
Outdated
/// Generates a file. | ||
Generated(GeneratedFile), | ||
} | ||
|
||
impl serde::ser::Serialize for FileContents { |
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.
Can we have the schema defined in cargo-util-schemas
with a schema.json file?
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.
Doesn't seem like a blocker. I would like to defer until stabilization, or any request from users.
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 is a schema we are expecting people to use programmatically but
- We are keeping internal the helpers for doing so, leaving them to the documentation.
- The serialization schema is tied directly to the implementation in a location that doesn't call out this is for serialization. This makes it harder for us to track schema changes (e.g. a refactor to rename a field). At least the
impl Serialize
is hand written which helps call it out but it might not always be that way. We do have tests but those can't exhaustively cover every case.
I also would like to see every existing serialization schema moved there.
I'd like to flip the script and instead of waiting on a need, for us to proactively do it. It will reduce the work we need to do later to take care of this if we do it in the first place. I held off on this for sboms mostly because I brought it up late in the process.
Yes, this could be done at stabilization. We'd need to track that as work to do for that. I also like to see stabilization make as small of a change as possible so its easier to review and has less risk.
Maybe waiting until later in the pre-stabilization process can help if there is expected churn in the overall design that it would require major rework (e.g. --list json
vs --message-format json
can end up with very different needs)
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.
Extracted. Separating internal implementation makes a lot of sense!
According to our JSON compatibility policy, the schema should have room to add more fields, the current path
and kind
won't move away I feel like, so it should be fine doing it now than later.
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.
There isn't a schema.json
file.
However,
- Its unclear what that file should look like for messages (should we put all messages in it?)
- Those are most for people code-genning off of it (less needed with
cargo-util-schemas
) or editors (less needed for programmatic files)
So not going to block on that.
src/bin/cargo/commands/package.rs
Outdated
.arg( | ||
flag( | ||
optional_opt( | ||
"list", | ||
"Print files included in a package without making one", | ||
"Print files included in a package without making one: (unstable) json", | ||
) | ||
.short('l'), | ||
.short('l') | ||
.value_name("FMT"), |
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.
Should we instead use --message-format
?
One reason is I wonder if we should focus on ndjson messages rather than a single json blob. This makes it easier to extend with more feature. For example, we can't extend cargo metadata
with warnings or errors without adding them to that single schema which feels weird.
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.
That might work, though conflating its meaning other commands with this might be a bit confusing. I personally may assume it is for diagnostics, for example missing metadata during packaging. The output of cargo package --list
is not a diagnostic message.
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've taken message
to mean "output message" which is generic, not specific to the type of message. The shape would probably need to be consistent across the different messages though.
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.
Just realized that with --message-format=json
the JSON messages are printed to stdout, so we are not really conflating these two stuffs. The existing compiler message are also emitted as ndjson via stdout, so reasonable 👍🏾.
There is the other question for using --message-format
: Should it also affect the package verification build? How Cargo verfies packages is kinda an implementation detail, so my answer is no. However we've mentioned in doc Cargo verifies it complies. Not sure if people get confused when seeing this.
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.
In the future --message-format=json
would also affect status message Cargo emits, right? So here comes another counterpoint: This adds some more work on users side, if they want to keep the human-readable status messages.
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.
In the future
--message-format=json
would also affect status message Cargo emits, right? So here comes another counterpoint: This adds some more work on users side, if they want to keep the human-readable status messages.
Thats true when interacting with most of our json APIs and why some have asked for a way to tee.
src/doc/man/cargo-package.md
Outdated
{ | ||
/* The Package ID Spec of the package. */ | ||
"path+file:///home/foo#0.0.0": { |
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.
Like I said at https://github.com/rust-lang/cargo/pull/15311/files#r1995637878, should we do a message per package or a message per file?
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 am good with per package.
How would message per file look like? Tools may need more work to associate all of them, but tools usually want information for the entire package I assume?
Also, with message-per-file, Cargo then needs to guarantee that there is no interleaving in between. Otherwise tools either need to consume all messages to determine if everything in a single package is collected.
src/doc/man/cargo-package.md
Outdated
/* Relative path in the archive file. */ | ||
"Cargo.toml.orig": { | ||
/* An absolute path to the actual file content. */ | ||
"original_path": "/home/foo/Cargo.toml", |
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.
snake case or kebab case?
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.
We do that i -Zsbom
, cargo metadata
, and --message-format=json
. This follows.
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.
sbom is just because of cargo metadata
. We deferred what it should actually do to the RFC. (I brought this up there).
Uck, didn't know --message-format=json
does that.
My hope for the new plumbing commands is that we reuse public schemas as much as possible, like TomlManifest
. Unless we copy/paste it, that will be kebab case.
So for right now, our schema document gives an unqualified statement to use kebab case. We should probably discuss this and figure out if there needs to be more qualifiers to it.
This comment has been minimized.
This comment has been minimized.
be412aa
to
dea55c7
Compare
--list json
option for JSON output format--message-format
flag
03adae7
to
6a907ac
Compare
The behavior is not implemented yet
This is is for `cargo package --list` in JSON format
This is needed for `cargo package --list` JSON message to access the orignal file path information for the generated file kind
6a907ac
to
e354769
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.
Thanks! Looking forward to using this in cargo-release
!
btw I added some of the decisions made during the review discussion to the PR description so we can better track them as we move towards stabilization (so the rest of the team can re-affirm or disagree) |
Update cargo 10 commits in 307cbfda3119f06600e43cd38283f4a746fe1f8b..a6c604d1b8a2f2a8ff1f3ba6092f9fda42f4b7e9 2025-03-20 20:00:39 +0000 to 2025-03-26 18:11:00 +0000 - fix(package): update tracking issue for `--message-format` (rust-lang/cargo#15354) - docs(contrib): Expand the description of team meetings (rust-lang/cargo#15349) - feat(package): add unstable `--message-format` flag (rust-lang/cargo#15311) - feat(complete): Added completion for `--profile` (rust-lang/cargo#15308) - Uplift windows Cygwin DLL import libraries (rust-lang/cargo#15193) - do not pass cdylib link args to test (rust-lang/cargo#15317) - fix: revert the behavior checking lockfile's VCS status (rust-lang/cargo#15341) - Temporarily ignore cargo_test_doctest_xcompile_ignores (rust-lang/cargo#15348) - docs: fix typo in the "Shared cache" section (rust-lang/cargo#15346) - Fix some issues with future-incompat report generation (rust-lang/cargo#15345) r? ghost
What does this PR try to resolve?
#11666
This adds an unstable
--message-format
flag tocargo package
to help--list
output in newline-delimited JSON format.See https://github.com/weihanglo/cargo/blob/package-list-fmt/src/doc/man/cargo-package.md#package-options for more on what is provided.
Open questions
--list json
or--message-format json
, see feat(package): add unstable--message-format
flag #15311 (comment)--message-format
flag #15311 (comment)plain
orhuman
, see feat(package): add unstable--message-format
flag #15311 (comment)--message-format
flag #15311 (comment)How should we test and review this PR?
If we don't want to show absolute paths in the JSON output,
could switch to relative path to either package root or cwd
(I prefer the latter though).