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

set -Zmtime_on_use from config or ENV #7411

Merged
merged 3 commits into from
Sep 24, 2019
Merged

set -Zmtime_on_use from config or ENV #7411

merged 3 commits into from
Sep 24, 2019

Conversation

Eh2406
Copy link
Contributor

@Eh2406 Eh2406 commented Sep 23, 2019

This lets you set the -Zmtime_on_use in config, it worked for me with

[unstable]
mtime_on_use=true

I did not find the ENV that would allow work. Suggestions?
Does this need tests?

Closes: #6978

@rust-highfive
Copy link

r? @nrc

(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 Sep 23, 2019
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

We don't really have a ton of precedent for this in terms of an [unstable] section of the config, almost everything requires some sort of opt-in via -Z which this does already have. The ask here I think is for something where you don't have to say anything on each invocation of cargo and you still get access to unstable features. That somewhat goes against how we've viewed unstable features historically (you need to constantly opt-in), but that's also the purpose of [unstable] vs something more bland like [build].

I don't really mind too much one way or the other myself, this is pretty minor

@@ -626,6 +626,12 @@ impl Config {
self.target_dir = cli_target_dir;
self.cli_flags.parse(unstable_flags)?;

if nightly_features_allowed() {
if let Some(val) = self.get_bool("unstable.mtime_on_use")?.map(|t| t.val) {
Copy link
Member

Choose a reason for hiding this comment

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

Could this try using the newer .get::<Option<bool>> API as well?

@@ -386,6 +386,7 @@ impl CliUnstable {
"advanced-env" => self.advanced_env = true,
"config-profile" => self.config_profile = true,
"dual-proc-macros" => self.dual_proc_macros = true,
// can also be set in .cargo/config or with and ENV
Copy link
Member

Choose a reason for hiding this comment

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

Could the unstable documentation be updated as well?

@Eh2406
Copy link
Contributor Author

Eh2406 commented Sep 23, 2019

Pipelining was unstable and used this model for testing instead of a -Z flag. Unfortunately we are not confident enough in the current strategy for mtime_on_use to just stabilize it behind a flag. After Pipelining we discussed this at a meeting and decided we were ok with it for mtime_on_use as recorded in #6978. I would not be Ok with us deciding to make this automatic for all -Z flags, as you said it blurs the meaning of unstable. Would you like to discuss it at the next meeting?

I will fix your other comments when I next have my editor open.

@alexcrichton
Copy link
Member

Nah that all sounds good to me! I had forgotten about that!

r=me with the nits updated

@Eh2406
Copy link
Contributor Author

Eh2406 commented Sep 24, 2019

@bors r=alexcrichton

@bors
Copy link
Collaborator

bors commented Sep 24, 2019

📌 Commit 473f999 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 Sep 24, 2019
@bors
Copy link
Collaborator

bors commented Sep 24, 2019

⌛ Testing commit 473f999 with merge 400221e...

bors added a commit that referenced this pull request Sep 24, 2019
set -Zmtime_on_use from config or ENV

This lets you set the `-Zmtime_on_use` in config, it worked for me with
```toml
[unstable]
mtime_on_use=true
```
I did not find the ENV that would allow work. Suggestions?
Does this need tests?

Closes: #6978
@bors
Copy link
Collaborator

bors commented Sep 24, 2019

☀️ Test successful - checks-azure
Approved by: alexcrichton
Pushing 400221e to master...

@bors bors merged commit 473f999 into rust-lang:master Sep 24, 2019
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Sep 25, 2019
4 commits in aa6b7e01abce30091cc594cb23a15c46cead6e24..ab6fa8908c9b6c8f7e2249231c395eebfbc49891
2019-09-24 17:19:12 +0000 to 2019-09-25 17:00:56 +0000
- Fix macOS collection of CPU data (rust-lang/cargo#7429)
- Don't ever capture CPU state if timings are disabled (rust-lang/cargo#7428)
- Change build-std to use --sysroot (rust-lang/cargo#7421)
- set -Zmtime_on_use from config or ENV (rust-lang/cargo#7411)
bors added a commit to rust-lang/rust that referenced this pull request Sep 25, 2019
Update cargo

4 commits in aa6b7e01abce30091cc594cb23a15c46cead6e24..ab6fa8908c9b6c8f7e2249231c395eebfbc49891
2019-09-24 17:19:12 +0000 to 2019-09-25 17:00:56 +0000
- Fix macOS collection of CPU data (rust-lang/cargo#7429)
- Don't ever capture CPU state if timings are disabled (rust-lang/cargo#7428)
- Change build-std to use --sysroot (rust-lang/cargo#7421)
- set -Zmtime_on_use from config or ENV (rust-lang/cargo#7411)
@Eh2406 Eh2406 deleted the mtime branch September 26, 2019 18:46
@ehuss ehuss added this to the 1.40.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.

mtime-on-use as a config option
6 participants