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

Make rmeta_required no longer depend on whether timing is enabled #10254

Merged
merged 1 commit into from
Jan 4, 2022

Conversation

joshtriplett
Copy link
Member

This doesn't appear to affect the quality of the timing information at
all.

If there's additional information we need from rustc about what it's
doing at any given time, we could add mechanisms to retrieve that
information, but enabling timing shouldn't force building more than we
otherwise would have.

This doesn't appear to affect the quality of the timing information at
all.

If there's additional information we need from rustc about what it's
doing at any given time, we could add mechanisms to retrieve that
information, but enabling timing shouldn't force building more than we
otherwise would have.
@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 Jan 4, 2022
@@ -604,7 +604,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
/// Returns whether when `unit` is built whether it should emit metadata as
/// well because some compilations rely on that.
pub fn rmeta_required(&self, unit: &Unit) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this now pull its weight as a separate function?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ok for now in the sense that it keeps the field private to just this module

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Jan 4, 2022

📌 Commit 25d1480 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 Jan 4, 2022
@bors
Copy link
Contributor

bors commented Jan 4, 2022

⌛ Testing commit 25d1480 with merge 358e79f...

@bors
Copy link
Contributor

bors commented Jan 4, 2022

☀️ Test successful - checks-actions
Approved by: alexcrichton
Pushing 358e79f to master...

@bors bors merged commit 358e79f into rust-lang:master Jan 4, 2022
@ehuss
Copy link
Contributor

ehuss commented Jan 4, 2022

Just to be clear, this does change the output a little. In some cases where cargo does not use pipelining, the information about when codegen starts isn't included. For example:

image

versus

image

I think that should be fine, as it still shows up in most cases.

It would be pretty easy to extend rustc to provide more information, would just need some thought on how to structure that and what information to obtain.

@bjorn3
Copy link
Member

bjorn3 commented Jan 4, 2022

If rmeta_required returns false cargo won't look for the message that the crate metadata is emitted and rustc won't be told to emit a message when it is emitted:

json.push_str(",artifacts");
It doesn't affect whether the crate metadata is actually emitted. That happens unconditionally if no linking is needed:
cmd.arg("--emit=dep-info,metadata,link");
Returning false from rmeta_required disables the mechanism by which -Ztimings figures out how long codegen takes without affecting affecting anything else.

@joshtriplett
Copy link
Member Author

@bjorn3 Ah, interesting! It sounds like we ought to ask rustc to emit that information unconditionally, then.

@joshtriplett joshtriplett deleted the rmeta-required-no-timings branch January 4, 2022 20:01
@joshtriplett
Copy link
Member Author

See #10255

cc @bjorn3 @ehuss

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 5, 2022
Update cargo

10 commits in fcef61230c3b6213b6b0d233a36ba4ebd1649ec3..358e79fe56fe374649275ca7aebaafd57ade0e8d
2021-12-17 02:30:38 +0000 to 2022-01-04 18:39:45 +0000
- Make rmeta_required no longer depend on whether timing is enabled (rust-lang/cargo#10254)
- The first version of pull request template (rust-lang/cargo#10218)
- Stabilize the `strip` profile option, now that rustc has stable `-C strip` (rust-lang/cargo#10088)
- Update docs for windows ssh-agent. (rust-lang/cargo#10248)
- Fix typo: substract -&gt; subtract (rust-lang/cargo#10244)
- timings: Fix tick mark alignment (rust-lang/cargo#10239)
- Remove unused lifetimes (rust-lang/cargo#10238)
- Make levenshtein distance case insensitive. (rust-lang/cargo#10224)
- [docs] Adds basic CI yaml for GitHub Actions (rust-lang/cargo#10212)
- Add function for parsing already-read manifest (rust-lang/cargo#10209)
@ehuss ehuss added this to the 1.59.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants