Allow configuring unstable flags via config file #8393

bearcage · 2020-06-21T21:59:53Z

Summary

This fixes #8127 by mapping the unstable key in .cargo/config to Z flags.

It should have no impact on stable/beta cargo, and on nightlies it gives folks the ability to configure Z flags for an entire project or workspace. This is meant to make it easier to try things like the new features resolver behavior, mtime-on-use, build-std, or timing reports across a whole project.

I've also included a small (but entirely independent) ergonomics change -- treating dashes and underscores identically in Z flags. That's along for the ride in this PR as the last commit, and is included here because it makes for more idiomatic toml file keys (print_im_a_teapot = yes vs print-im-a-teapot = yes). Please let me know if y'all would prefer that be in a separate PR, or not happen at all.

Test Plan

Apologies if I've missed anything -- this is my first cargo contrib and I've tried to hew to the contributing guide. If I've slipped up, please let me know how and I'll fix it.

NB. My linux machine doesn't have multilib set up, so I disabled cross tests.

cargo test passes for each commit in the stack
I formatted each commit in the stack with rustfmt
New tests are included alongside the relevant change for each change
I've validated each test by locally undoing the code change they support and confirming failure.
The CLI wins, for both enable and disabling Z flags, as you'd expect.

Keys in unstable which do not correspond with a Z flag will trigger an error indicating the invalid flag came from a config file read:

Invalid [unstable] entry in Cargo config

Caused by:
  unknown `-Z` flag specified: an-invalid-flag

If you'd like to see a test case which isn't represented here, I'm happy to add it. Just let me know.

Documentation

I've included commits in this stack updating the only docs page that seemed relevant to me, skimming the book -- unstable.md.

rust-highfive · 2020-06-21T21:59:57Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

alexcrichton

Seems like a reasonable feature to me, I find the description of #8127 pretty compelling! I'm curious though if we could rely on serde for deserialization here?

Additionally can you add a test where a flag is specified but we're on the stable channel? (an error should be returned). Ideally the stable/beta channel would error if anything exists in the unstable table as well, not just the known keys.

alexcrichton · 2020-06-23T14:58:20Z

src/cargo/core/features.rs

-        let v = parts.next();
+    /// Read unstable options from a hashmap.
+    /// Intended for consuming unstable settings from config files
+    pub fn from_table(&mut self, flags: &HashMap<String, String>) -> CargoResult<()> {


Instead of from_table, perhaps CliUnstable could implement Deserialize? That way we could do config.get::<CliUnstable>("unstable")?

That'd be my preference too -- I didn't add it initially for fear of making too large an unsolicited diff, but I'll see what it looks like to swap over the little bespoke deserializer table.

Hm, on closer inspection I think kicking this over to serde is orthogonal -- both the parse and from_table functions are updating-in-place, rather than deserializing to construct a new instance. I think that makes a difference here because we rely on being able to differentiate between "false because that's the default" and "explicitly set to false" to give the CLI the ability to override a config file in turning off a Zflag.

bearcage · 2020-06-23T17:42:09Z

I gather it's probably fair game since we're strictly talking about unstable flags / features here, but a hard error on Zflags in config would be a break with how mtime-on-use is configured today in that stable cargo simply ignores the setting.

I can see a reasonable argument either way --

Pro: A hard error matches the behavior of setting these same flags at the command line, and reduces the chances of a nasty surprise change-in-behavior if you switch channels and recompile.

Con: A hard error means you can't leave unstable options in config and toggle back and forth between stable and nightly, or configure CI runs on nightlies to assert that a new feature does not break a build that works on stable.

Let me know how you'd like to see it work, and I'll get it changed.

alexcrichton · 2020-06-23T19:43:02Z

Oh right nevermind, I forgot that we ignore it on stable/beta! That seems fine for now!

src/doc/src/reference/unstable.md

bearcage · 2020-06-26T00:13:42Z

@alexcrichton would you still like me to include a Deserialize impl in this PR, given it won't slot into the update path it's modifying (I think, anyway)?

alexcrichton

I personally think that when using .cargo/config.toml we should go through serde as much as possible. That gives us env vars, good error mesages, etc. Avoiding it will likely result in us quickly having a list of issues to fix and I think it's best to head that off.

alexcrichton · 2020-06-29T18:06:54Z

src/cargo/util/config/mod.rs

+                // NB. It sucks to parse these twice, but doing it again here
+                //     allows the CLI to override config files for both enabling
+                //     and disabling.
+                self.unstable_flags.parse(unstable_flags)?;


Could this perhaps move from the beginning of the function to the end to avoid doing this twice?

It can, but it'll introduce a couple funky edge cases around bits of the config deserializer that are themselves gated on Zflags. In particular I'm thinking of advanced_env (used twice in that file).

EDIT: I haven't gone through the whole call graph from higher up in the initialization function to see where else we might wind up leaning on the cli parsing happening first, but I will, once I've got your other comments addressed.

Ah either way is fine, if it needs to go near the beginning it's gotta go near the beginning, alas!

I gave this another go and I think (unfortunately) parsing the cli flags twice is the simplest answer for allowing explicit disables at the command line for flags that are otherwise enabled in config.

src/cargo/util/config/mod.rs

src/cargo/core/features.rs

bearcage · 2020-07-03T00:04:06Z

EDIT: also I rebased on master, out of force of habit.

Just a quick update — I'm still working on this, I just hit an oddity in the Deserializer that's treating missing fields as an error even with #[serde(default)] on the struct. Curiously, forcing every field to use the Option deserializer first and explicitly defaulting them works,
(a trick I shamelessly borrowed from dtolnay's answer to a totally unrelated question about serde usage elsewhere on the internet). That looks something like:

#[serde(deserialize_with="null_default")]
and

fn null_default<'de, D, T>(deserializer: D) -> Result<T, D::Error>
where
    D: serde::Deserializer<'de>,
    T: Deserialize<'de> + Default,
{
    let option = Option::deserialize(deserializer)?;
    Ok(option.unwrap_or_default())
}

I'm digging through the Deserializer now to see what's going on there.

bearcage · 2020-07-03T22:41:06Z

Thinking out loud a bit here:

After talking it over with a buddy (thx @packysauce!) I think there's a bit of a layering problem in the de module that's preventing serde from doing defaults or deny_unknown here — when we're deserializing a struct, ConfigMapAccess is providing an iterator over the union of two sets: the fields we're interested in for this struct (the fields member slice), and k/v pairs available in the config table we're currently considering. As I understand them, serde MapAccess implementers are expected to serve up kv pairs of the underlying map without any particular care for what set of keys the caller is interested in. With that in mind, handling env reading inline complicates the Deserializer.

As I understand it, that means there are two problems:

Serde's handling for defaulting and denying-unknown will always be a noop, because we never hand back a k/v pair that the Deserialize impl didn't explicitly ask for as a field, and if we're missing a field we error out early ourselves instead of letting the derived Deserialize impl do it. This is caused by the union done in ConfigMapAccess, but fixing it would complicate point 2.
Environment variables don't fit super neatly in that model — if we modified ConfigMapAccess to return all k/v pairs from the config object under consideration plus all env vars which match the prefix under consideration, env vars which match member structs would show up as unexpected field keys (e.g. CARGO_UNSTABLE_SOMETABLE_SUBKEY would show up as an unknown key in CARGO_UNSTABLE even if CARGO_INSTABLE_SOMETABLE would have been valid).

While I could work around this for CliUnstable by annotating every non-Option field with a deserialize_with, I think the "Right" solution here is probably a larger refactor to split the env reading out of the Deserializer impl such that Config::get would try reading each source in decreasing priority order independently, with its own purpose-specific deserializer, rather than using a combined deserializer.

Alternatively, I can make the change suggested in 1 above — tweak the map accessor to report all fields in the joined set of env/config — but I suspect it'll lead to a bunch of funky corner case bugs around required/unknown fields and env vars.

EDIT: added alternative.

alexcrichton · 2020-07-06T15:24:55Z

I'd definitely believe that the config bits with serde aren't exactly the best they could be, I've never really fully grok'd how to write a serde deserializer. That being said I'm happy with whatever works here, I think switching all the fields to Option would also be fine.

This makes it easier to populate unstable options from configuration files. Might also help make unstable option tests easier to write.

Obviously this only works with nightlies and all that, but if you're e.g. testing resolver v2, want to always have Ztiming on in your workspace, or something like that, this makes it easier to do.

This makes it a little easier to match whatever the natural formatting is for where you're setting unstable options -- CLI or config file.

Co-authored-by: Eric Huss <eric@huss.org>

These masquerade like construction functions, but they're in-place mutators since they're meant to be used one after another in a precedence chain.

Per comments on the PR, a couple changes need to be backed out.

Tests are currently failing, looks like there's something in the Deserializer impl that's forcing field-missing errors even when the serde `default` annotation is applied.

This is a workaround in case y'all would like to land this change before I'm able to finish up a Deserializer refactor.

The current behavior, with the default-false workaround in place, is not able to identify extra members of the `unstable` table, so it can't error on unexpected members. I'll add this test back in with a Deserializer refactor to clean up the extra logic added to CliUnstable.

bearcage · 2020-07-08T05:42:35Z

I've never totally understood the serde model myself, either — I'm learning my way through it on this PR. Definitely tell me if I've got it wrong!

Changes in this round:

Rebased on master
Added a default-to-false deserialize_with workaround to CliUnstable
Dropped a (probably helpful) test that's broken by that change.

I'm gonna keep hacking on top of this to flip the Deserializer impl around to allow serde's default/deny_unknown_fields to do the heavy lifting, but I didn't want y'all to have to wait on me finishing that up to land this and close the linked issue, if you'd like to.

In the meantime please don't hesitate to ask if you'd like me to make some more changes to this first.

alexcrichton · 2020-07-08T19:29:08Z

Looks good to me! I'd be fine landing this as-is. FWIW I don't think we want to add deny_unknown_fields because that'll make it painful to use this from stable Cargo when nightly adds a new feature

This patch changes how ConfigMapAccess iterates k/v pairs when deserializing structs. Previously we produced keys for exactly the set of fields needed for a struct, and errored out in the deserializer if we can't find anything for that field. This patch makes us produces keys from the union of two sets: 1. All fields that are both needed for the struct and can be found in the environment. 2. All fields in the config table. This change allows serde's codegen to handle both missing and unknown fields via the usual derive annotations (default or deny_unknown_fields respectively)

Serde's errors for missing fields are a lil' different than the ones from our Deserializer.

bearcage · 2020-07-09T03:23:53Z

Just kidding, this was way simpler than I was making it out to be — the new_map ctor for ConfigMapAccess does the right thing already, it's just the struct behavior is trying to be a little too clever in how it's prefiltering fields.

I patched new_struct and dropped the workaround. If this still looks good I think it's ready to rock! Otherwise I'm happy to revert this Deserializer change and ship the deserialize_with version.

bearcage · 2020-07-09T21:17:09Z

Happy to help - thanks for the reviews!

bors · 2020-07-09T21:36:58Z

💔 Test failed - checks-azure

bearcage · 2020-07-09T23:37:48Z

Test failure, bummer.

I couldn't find a way to pull the proptest codegen'd case off the builder host, so I've left my workstation running a copy of the failing proptest (prop_limited_independence_of_irrelevant_alternatives in resolver) per core until I get a reproducer. Fingers crossed it doesn't take toooooo long.

EDIT: 128000 iterations down, no repro yet. Kicking off 5120000 more.

ehuss · 2020-07-09T23:47:29Z

@bors retry
#6258 (comment)

bors · 2020-07-09T23:47:37Z

⌛ Testing commit d035daa with merge f35ebaf...

bearcage · 2020-07-09T23:56:51Z

thx ehuss!

I just got a repro case locally, but skimming the linked thread it looks like y'all already have a handle on where the edge case proptest is turning up is so I'll let it lie.

bors · 2020-07-10T00:10:40Z

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

Eh2406 · 2020-07-10T14:24:27Z

Technically there have to be an infinite supply of edge cases. The problem is NP-Complete so there have to be exponencial cases. So far (at least recently) the proptest are finding particularly un-realistic examples and my investigations have not found ways to make them fast that are worth the additional code complexity. But each one is a new adventure, that can be completely unrelated.

Update cargo 4 commits in 4f74d9b2a771c58b7ef4906b2668afd075bc8081..43cf77395cad5b79887b20b7cf19d418bbd703a9 2020-07-08 17:13:00 +0000 to 2020-07-13 17:35:42 +0000 - fix: add space to comments (rust-lang/cargo#8476) - Allow configuring unstable flags via config file (rust-lang/cargo#8393) - Add support for rustc's `-Z terminal-width`. (rust-lang/cargo#8427) - Avoid colliding with older Cargo fingerprint changes (rust-lang/cargo#8473)

phil-opp · 2020-07-16T10:28:26Z

I just tried this feature with the latest nightly and it seems to work great overall. Thanks a lot @bearcage for implementing this!

One minor thing I noticed was that the -Zbuild=core command line argument behaves slightly different than setting unstable.build-std = ["core"] in a .cargo/config file. The former seems to automatically derive that compiler_builtins is also needed when building core, while the latter seems not. With the config option, a "can't find crate for compiler_builtins" error occurs unless I add compiler_builtins to the unstable.build-std list. This isn't a problem for me, but I wanted to report it in case this is unintended.

This fixes an issue where the deserializer for `-Zbuild-std` was a bit fancier than the `unstable.build-std` directive. cc rust-lang#8393

alexcrichton · 2020-07-16T15:50:13Z

@phil-opp looks like that's a bug

phil-opp · 2020-07-16T17:07:46Z

Thanks a lot for the quick investigation and fix!

…h2406 Ensure `unstable.build-std` works like `-Zbuild-std` This fixes an issue where the deserializer for `-Zbuild-std` was a bit fancier than the `unstable.build-std` directive. cc #8393

simnalamburt · 2020-07-27T11:56:59Z

Thanks to this PR, now we can use the rust-analyzer with the unstable cargo option! Yay

Reference: rust-lang/cargo#8393

…er build If a user adds `-Zbuild-std` to the command line to rebuild the std it is only propagated to the build of each version, but not the runner. These flags, however, can also be configured with [environment variables](rust-lang/cargo#8393). So if a user uses the `CARGO_UNSTABLE_BUILD_STD` environment variable to rebuild the std of the crate, it is propagated to the build of the runner. Since the runner adds `panic=abort`, if the user does not add `CARGO_UNSTABLE_BUILD_STD=std,panic_abort`, there are duplicate lang item errors. These errors might be confusing since the build of the runner is not necessarily known, and its profile even less. See #7

rust-highfive assigned alexcrichton Jun 21, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 21, 2020

bearcage mentioned this pull request Jun 21, 2020

Add a build.cargoflags configuration key #8127

Closed

alexcrichton reviewed Jun 23, 2020

View reviewed changes

ehuss reviewed Jun 24, 2020

View reviewed changes

src/doc/src/reference/unstable.md Outdated Show resolved Hide resolved

alexcrichton reviewed Jun 29, 2020

View reviewed changes

bearcage and others added 10 commits July 7, 2020 23:38

Allow populating CliUnstable from a HashMap

1cea4cb

This makes it easier to populate unstable options from configuration files. Might also help make unstable option tests easier to write.

Allow setting unstable options in .cargo/config

4aede8c

Obviously this only works with nightlies and all that, but if you're e.g. testing resolver v2, want to always have Ztiming on in your workspace, or something like that, this makes it easier to do.

Allow both dashes and underscores for Z flags

9cb1ef8

This makes it a little easier to match whatever the natural formatting is for where you're setting unstable options -- CLI or config file.

Update src/doc/src/reference/unstable.md

c60da96

Co-authored-by: Eric Huss <eric@huss.org>

Clarify CliUnstable::parse/update_with_table

6d7421c

These masquerade like construction functions, but they're in-place mutators since they're meant to be used one after another in a precedence chain.

Revert misfeatures

48a6f59

Per comments on the PR, a couple changes need to be backed out.

Switch unstable config parsing to serde

bd938f0

Tests are currently failing, looks like there's something in the Deserializer impl that's forcing field-missing errors even when the serde `default` annotation is applied.

[fixup] use single-line serde annotations

14c32d4

Treat all CliUnstable values as default-false

c532c49

This is a workaround in case y'all would like to land this change before I'm able to finish up a Deserializer refactor.

bearcage added 2 commits July 8, 2020 21:14

Minor test fix (error text comparison)

724273e

Serde's errors for missing fields are a lil' different than the ones from our Deserializer.

bearcage added 2 commits July 8, 2020 21:33

clearer comment on doubleparse of cli flags

782b32b

fix nightly rustdoc-extern tests

90b9c6a

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

ehuss mentioned this pull request Jul 9, 2020

Tracking issue for proptest resolve errors. #6258

Open

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 Jul 9, 2020

bors merged commit f35ebaf into rust-lang:master Jul 10, 2020

bearcage deleted the unstable_flags_in_config branch July 12, 2020 22:55

ehuss mentioned this pull request Jul 14, 2020

Update cargo rust-lang/rust#74332

Merged

alexcrichton added a commit to alexcrichton/cargo that referenced this pull request Jul 16, 2020

Ensure unstable.build-std works like -Zbuild-std

78314ca

This fixes an issue where the deserializer for `-Zbuild-std` was a bit fancier than the `unstable.build-std` directive. cc rust-lang#8393

alexcrichton mentioned this pull request Jul 16, 2020

Ensure unstable.build-std works like -Zbuild-std #8491

Merged

This was referenced Jul 17, 2020

Migrate code from cargo-xbuild to -Zbuild-std phil-opp/blog_os#835

Merged

cargo install does not ignore unstable table in .cargo/config.toml file #8512

Closed

simnalamburt added a commit to simnalamburt/flashbench that referenced this pull request Jul 28, 2020

Use cargo build instead of cargo xbuild

4343851

Reference: rust-lang/cargo#8393

phil-opp mentioned this pull request Aug 24, 2020

De-stabilize target spec JSON rust-lang/rust#71009

Open

ehuss added this to the 1.47.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow configuring unstable flags via config file #8393

Allow configuring unstable flags via config file #8393

bearcage commented Jun 21, 2020

rust-highfive commented Jun 21, 2020

alexcrichton left a comment

alexcrichton Jun 23, 2020

bearcage Jun 23, 2020

bearcage Jun 24, 2020 •

edited

Loading

bearcage commented Jun 23, 2020

alexcrichton commented Jun 23, 2020

bearcage commented Jun 26, 2020 •

edited

Loading

alexcrichton left a comment

alexcrichton Jun 29, 2020

bearcage Jun 30, 2020 •

edited

Loading

alexcrichton Jun 30, 2020

bearcage Jul 9, 2020

bearcage commented Jul 3, 2020 •

edited

Loading

bearcage commented Jul 3, 2020 •

edited

Loading

alexcrichton commented Jul 6, 2020

bearcage commented Jul 8, 2020

alexcrichton commented Jul 8, 2020

bearcage commented Jul 9, 2020 •

edited

Loading

bearcage commented Jul 9, 2020

bors commented Jul 9, 2020

bearcage commented Jul 9, 2020 •

edited

Loading

ehuss commented Jul 9, 2020

bors commented Jul 9, 2020

bearcage commented Jul 9, 2020 •

edited

Loading

bors commented Jul 10, 2020

Eh2406 commented Jul 10, 2020

phil-opp commented Jul 16, 2020

alexcrichton commented Jul 16, 2020

phil-opp commented Jul 16, 2020

simnalamburt commented Jul 27, 2020

Allow configuring unstable flags via config file #8393

Allow configuring unstable flags via config file #8393

Conversation

bearcage commented Jun 21, 2020

Summary

Test Plan

Documentation

rust-highfive commented Jun 21, 2020

alexcrichton left a comment

Choose a reason for hiding this comment

alexcrichton Jun 23, 2020

Choose a reason for hiding this comment

bearcage Jun 23, 2020

Choose a reason for hiding this comment

bearcage Jun 24, 2020 • edited Loading

Choose a reason for hiding this comment

bearcage commented Jun 23, 2020

alexcrichton commented Jun 23, 2020

bearcage commented Jun 26, 2020 • edited Loading

alexcrichton left a comment

Choose a reason for hiding this comment

alexcrichton Jun 29, 2020

Choose a reason for hiding this comment

bearcage Jun 30, 2020 • edited Loading

Choose a reason for hiding this comment

alexcrichton Jun 30, 2020

Choose a reason for hiding this comment

bearcage Jul 9, 2020

Choose a reason for hiding this comment

bearcage commented Jul 3, 2020 • edited Loading

bearcage commented Jul 3, 2020 • edited Loading

alexcrichton commented Jul 6, 2020

bearcage commented Jul 8, 2020

alexcrichton commented Jul 8, 2020

bearcage commented Jul 9, 2020 • edited Loading

bearcage commented Jul 9, 2020

bors commented Jul 9, 2020

bearcage commented Jul 9, 2020 • edited Loading

ehuss commented Jul 9, 2020

bors commented Jul 9, 2020

bearcage commented Jul 9, 2020 • edited Loading

bors commented Jul 10, 2020

Eh2406 commented Jul 10, 2020

phil-opp commented Jul 16, 2020

alexcrichton commented Jul 16, 2020

phil-opp commented Jul 16, 2020

simnalamburt commented Jul 27, 2020

bearcage Jun 24, 2020 •

edited

Loading

bearcage commented Jun 26, 2020 •

edited

Loading

bearcage Jun 30, 2020 •

edited

Loading

bearcage commented Jul 3, 2020 •

edited

Loading

bearcage commented Jul 3, 2020 •

edited

Loading

bearcage commented Jul 9, 2020 •

edited

Loading

bearcage commented Jul 9, 2020 •

edited

Loading

bearcage commented Jul 9, 2020 •

edited

Loading