Skip to content
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 "build-finished" JSON message. #8069

Merged
merged 1 commit into from
Apr 9, 2020
Merged

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Apr 5, 2020

This adds a JSON message when a build is finished. This is useful for tools to know when to stop parsing JSON, which is particularly useful for commands like cargo test or cargo run where additional output may follow.

Closes #7978

@rust-highfive
Copy link

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 5, 2020
@ehuss
Copy link
Contributor Author

ehuss commented Apr 5, 2020

@alexcrichton I thought some more about the comment you left, but I really think it is better just to have a single message. I wouldn't want tools to have to listen for N messages to know when things are done. And generally I don't need to know when something launches.

We can add "launched" messages in the future if it looks like people want it. Also, I think the libtest JSON message already has a separate message for "starting", so test frameworks may already cover that need.

@matklad
Copy link
Member

matklad commented Apr 5, 2020

Seems reasonable for me to add, it's not like this will burden us with backward compatibility, and it does seem useful.

However, I do have a gut feeling that a "proper" solution here is allowing clients to split Cargo's command in two phases, building and running. Sort-of like a universal --no-run argument, so that clients can do cargo run --example foo --no-run and then either cargo run --example foo again or just run the binaries themselves. But it seems unlikely that we actually add such a --no-run flag, and, even if we do, such addition wouldn't be precluded by the BuildFinished message. We might even want to have BuildFinished message anyway, to, for example, convey the [optimized + debuginfo] in 60 seconds info in machine-redable form.

@ehuss
Copy link
Contributor Author

ehuss commented Apr 5, 2020

cargo build --example foo I think is 100% identical to a hypothetical cargo run --no-run. The JSON even gives you the path to the executable, so a tool can just run it directly (there might be some rare dylib path issues, but programs probably shouldn't rely on those, or the tool can just execute cargo run if it wants to be safe/simple). My tool just executes cargo run and once it sees the text ^\s*Running it stops looking for JSON messages. I can switch to looking for "build-finished" with this PR.

@alexcrichton alexcrichton added the T-cargo Team: Cargo label Apr 6, 2020
@alexcrichton
Copy link
Member

@rfcbot fcp merge

I'm personally ok doing this, and it also seems plausible to take the strategy that @matklad mentions but I don't think there's any reason they need to be mutually exclusive!

@rfcbot
Copy link
Collaborator

rfcbot commented Apr 6, 2020

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge final-comment-period FCP — a period for last comments before action is taken and removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. labels Apr 6, 2020
@rfcbot
Copy link
Collaborator

rfcbot commented Apr 9, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@alexcrichton
Copy link
Member

That's enough of sign-off for me, and we can always back out if there are other objections!

@bors: r+

@bors
Copy link
Contributor

bors commented Apr 9, 2020

📌 Commit c889bbf has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 9, 2020
@bors
Copy link
Contributor

bors commented Apr 9, 2020

⌛ Testing commit c889bbf with merge 239f2bf...

@bors
Copy link
Contributor

bors commented Apr 9, 2020

☀️ Test successful - checks-azure
Approved by: alexcrichton
Pushing 239f2bf to master...

@bors bors merged commit 239f2bf into rust-lang:master Apr 9, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 14, 2020
Update cargo

12 commits in 390e8f245ef2cd7ac698b8a76abf029f9abcab0d..74e3a7d5b756d7c0e94399fc29fcd154e792c22a
2020-04-07 17:46:45 +0000 to 2020-04-13 20:41:52 +0000
- Update dependencies to support illumos target (rust-lang/cargo#8093)
- Whitelist another known spurious curl error (rust-lang/cargo#8102)
- Fix nightly test matching rustc "warning" output. (rust-lang/cargo#8098)
- Update default for codegen-units. (rust-lang/cargo#8096)
- Fix freshness when linking is interrupted. (rust-lang/cargo#8087)
- Add `cargo tree` command. (rust-lang/cargo#8062)
- Add "build-finished" JSON message. (rust-lang/cargo#8069)
- Extend -Zpackage-features with more capabilities. (rust-lang/cargo#8074)
- Disallow invalid dependency names through crate renaming (rust-lang/cargo#8090)
- Use the same filename hash for pre-release channels. (rust-lang/cargo#8073)
- Index the commands section (rust-lang/cargo#8081)
- Upgrade to mdBook v0.3.7 (rust-lang/cargo#8083)
@rfcbot rfcbot added finished-final-comment-period FCP complete and removed final-comment-period FCP — a period for last comments before action is taken labels Apr 19, 2020
@ehuss ehuss added this to the 1.44.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge FCP with intent to merge finished-final-comment-period FCP complete S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-cargo Team: Cargo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Difficult to know when to stop parsing JSON messages.
6 participants