-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
add message when compile starts for crate #12867
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @weihanglo (or someone else) soon. 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 (
|
@@ -1129,6 +1136,13 @@ impl<'cfg> DrainState<'cfg> { | |||
if unit.mode.is_check() { | |||
config.shell().status("Checking", &unit.pkg)?; | |||
} else { | |||
if json { | |||
let msg = machine_message::CompileStarted { | |||
package_id: unit.pkg.package_id(), |
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 should also include the target and platform, to allow distinguishing the different crates of a package and builds for the host vs target (vs artifact-dependencies targets).
There's also other unit types that "start", like run-custom-build
, which would be useful to have a message for too.
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.
include the target and platform
The other cargo messages dont do this, it would be pointless for this one to, unless all of them did?
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.
Would run-custom-build
be the start of a build-script-executed
?
I think I can do that in another PR.
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.
The other cargo messages dont do this, it would be pointless for this one to, unless all of them did?
The others do include the target
, IMO them not including the platform
is a bug (#12869), but maybe best to see what the maintainers say.
Would
run-custom-build
be the start of abuild-script-executed
?
Yep, exactly.
I think I can do that in another PR.
👍
Considering the associated issue is closed (#12864), I'm closing this PR please wait until an issue is discussed and accepted before posting PRs. |
The associated issue was "moved" into #8283 though? |
That was a consolidation; there is still not an accepted issue. Also, that closed issue was what this PR was marked as resolving. |
fixes #12864
adds json output such as:
How should we test and review this PR?
git clone --depth 1 https://github.com/rust-lang/cargo cargo1
git clone --depth 1 https://github.com/bend-n/cargo cargo2
cd cargo2
cargo build
cd ../cargo1
../cargo2/target/debug build --message-format json
or just
cargo test -- json
Notes
should the message have more information?