Migrate towards exclusively using serde for `Config` #7456

alexcrichton · 2019-09-27T21:47:56Z

This series of commits was spawned off a thought I had while reading #7253 (comment), although it ended up not really touching on that at all. I was a little unsettled about how unstructured the config accesses are throughout Cargo and we had sort of two systems (one serde which is nice, one which is more manual) for reading config values.

This PR converts everything to run through serde for deserializing values, except for get_list which is funky. There's only one usage of that with the paths key though and we can probably fix this soon-ish.

In any case, the highlights of this PR are:

This PR is surprisingly large. I did a lot of movement in config.rs to try to make the file smaller and more understandable.
The Value type which retains information about where it was deserialized from is very special, and has special treatment with serde's data model. That's what allows us to use that and serde at the same time.
The ConfigRelativePath and ConfigKey structures have been revamped internally, but morally serve the same purposes as before.
Cargo now has structured struct access for a bunch of its configuration (net, http, build, etc). I would ideally like to move toward a world where this is the only way to read configuration, or at least everything conventionally runs through those paths.
Functionally, this PR should have no difference other than tweaks to error messages here and there, and perhaps more strict validation on commands where we validate more configuration on each run than we previously did.
This isn't a 100% transition for Cargo yet, but I figured it would be a good idea to post this and get some feedback first.
In the long run I want to remove get_env, get_cv, and get_*_priv from Config as internal details. I'd like to move this all to de.rs and have it walk down the tree of configuration as we deserialize a value. For now though these all remain in place and that refactoring is left to a future PR.

alexcrichton · 2019-09-27T21:48:02Z

r? @ehuss

rust-highfive · 2019-09-27T21:48:07Z

r? @nrc

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

bors · 2019-09-30T19:29:20Z

☔ The latest upstream changes (presumably #7308) made this pull request unmergeable. Please resolve the merge conflicts.

alexcrichton · 2019-10-03T19:19:05Z

r? @ehuss

ehuss

This looks great!

src/cargo/util/config/mod.rs

src/cargo/ops/registry.rs

src/cargo/util/config/key.rs

src/cargo/util/config/de.rs

Also make it a little less allocation-heavy by tweaking the API to encourage incremental building of the key and incremental destruction as we walk throughout the configuration tree.

Rewrite helpers like `get_bool` to use `get::<Option<Value<bool>>>` instead of duplicating the logic that's already with the typed access of configuration. This is more along the effort to centralize all deserialization of configuration into typed values instead of using ad-hoc accessors in a number of locations.

This callsite doesn't need the full power of `get_list`, knowing the definition path of each element along the list.

Gives us one nice place to access and document all HTTP-related configuration

Going through and removing users of raw `get_*` functions!

Less need for `get_bool` and friends!

Add a typed structure which lists all `build` key configuration throughout Cargo.

No need for lots of extra helpers/parsing when using serde!

alexcrichton · 2019-10-08T00:12:10Z

Ok updated! Want to take another look @ehuss?

ehuss

Thanks, r=me with small fixes.

ehuss · 2019-10-08T15:19:42Z

src/cargo/util/config/mod.rs

+/// single string or a string list itself. For example these deserialize to
+/// equivalent values:
+///
+/// ```


Suggested change

/// ```

/// ```toml

ehuss · 2019-10-08T15:22:51Z

src/cargo/util/config/key.rs

+    // updated with `push` methods and looks like `CARGO_FOO_BAR` for pushing
+    // `foo` and then `bar`.
+    env: String,
+    // The current environment variable this configuration key maps to. This is


Suggested change

// The current environment variable this configuration key maps to. This is

// The current toml key this configuration key maps to. This is

alexcrichton · 2019-10-09T16:55:11Z

@bors: r=ehuss

bors · 2019-10-09T16:55:12Z

📌 Commit 9a12e48 has been approved by ehuss

bors · 2019-10-09T16:55:19Z

⌛ Testing commit 9a12e48 with merge 94bf478...

Migrate towards exclusively using serde for `Config` This series of commits was spawned off a thought I had while reading #7253 (comment), although it ended up not really touching on that at all. I was a little unsettled about how unstructured the config accesses are throughout Cargo and we had sort of two systems (one serde which is nice, one which is more manual) for reading config values. This PR converts everything to run through serde for deserializing values, except for `get_list` which is funky. There's only one usage of that with the `paths` key though and we can probably fix this soon-ish. In any case, the highlights of this PR are: * This PR is surprisingly large. I did a lot of movement in `config.rs` to try to make the file smaller and more understandable. * The `Value` type which retains information about where it was deserialized from is very special, and has special treatment with serde's data model. That's what allows us to use that and serde at the same time. * The `ConfigRelativePath` and `ConfigKey` structures have been revamped internally, but morally serve the same purposes as before. * Cargo now has structured `struct` access for a bunch of its configuration (`net`, `http`, `build`, etc). I would ideally like to move toward a world where this is the *only* way to read configuration, or at least everything conventionally runs through those paths. * Functionally, this PR should have no difference other than tweaks to error messages here and there, and perhaps more strict validation on commands where we validate more configuration on each run than we previously did. * This isn't a 100% transition for Cargo yet, but I figured it would be a good idea to post this and get some feedback first. * In the long run I want to remove `get_env`, `get_cv`, and `get_*_priv` from `Config` as internal details. I'd like to move this all to `de.rs` and have it walk down the tree of configuration as we deserialize a value. For now though these all remain in place and that refactoring is left to a future PR.

bors · 2019-10-09T17:29:51Z

☀️ Test successful - checks-azure
Approved by: ehuss
Pushing 94bf478 to master...

…chton Update cargo, cc, books Update `cc` to include new parallel building support. ## nomicon 3 commits in 4374786f0b4bf0606b35d5c30a9681f342e5707b..5004ad30d69f93553ceef74439fea2159d1f769e 2019-09-17 18:33:21 +0200 to 2019-10-12 19:52:40 +0200 - further clarify C11 and C/C++11 terminology (rust-lang/nomicon#169) - atomics: C11 -> C++20 (rust-lang/nomicon#168) - use sound/unsound terminology ## cargo 12 commits in a429e8cc4614a46a86322a0777a477e2baa83f1c..3a9abe3f065554a7fbc59f440df2baba4a6e47ee 2019-10-04 17:36:12 +0000 to 2019-10-15 15:55:35 +0000 - Fix typo in git index initialization error path (rust-lang/cargo#7512) - Reject feature flags in a virtual workspace. (rust-lang/cargo#7507) - Rename `overrides` to `package` in profiles. (rust-lang/cargo#7504) - Allow publishing with dev-dependencies without a version. (rust-lang/cargo#7333) - Stabilize cache-messages (rust-lang/cargo#7450) - don't lock the package cache when cleaning target dir. (rust-lang/cargo#7502) - Document rustc wrapper (rust-lang/cargo#7499) - Migrate towards exclusively using serde for `Config` (rust-lang/cargo#7456) - Re-enable some MSVC tests. (rust-lang/cargo#7492) - when -Z unstable-options not specified, don't validate --profile (rust-lang/cargo#7489) - Improve error message for cyclic dependencies (rust-lang/cargo#7470) - Some minor clippy fixes. (rust-lang/cargo#7484) ## book 7 commits in 04806c80be0f54b1290287e3f85e84bdfc0b6ec7..9bb8b161963fcebc9d9ccd732ba26f42108016d5 2019-10-01 20:20:22 -0400 to 2019-10-14 18:42:55 -0500 - Make a portion of text less ambiguous (rust-lang/book#2092) - fix heading level (rust-lang/book#2117) - Add missing "of" before `"duck typing"`. (rust-lang/book#1951) - ch18-03: no need to debug print destructured int (rust-lang/book#1991) - Subtle fix to introduce ? on Option in Chapter 9.2 (rust-lang/book#2047) - make wording clearer (rust-lang/book#1976) - Update the version of rand we use ## rust-by-example 5 commits in a6288e7407a6c4c19ea29de6d43f40c803883f21..0b111eaae36cc4b4997684be853882a59e2c7ca7 2019-10-01 10:09:14 -0300 to 2019-10-14 18:34:25 -0300 - Some fix to three files (rust-lang/rust-by-example#1280) - Add reference to Generics (rust-lang/rust-by-example#1281) - Confusing and long sentence (rust-lang/rust-by-example#1282) - Explicit mention of slice range meaning (rust-lang/rust-by-example#1277) - Updated aliasing for nll (rust-lang/rust-by-example#1276)

Update cargo, books ## nomicon 3 commits in 4374786f0b4bf0606b35d5c30a9681f342e5707b..5004ad30d69f93553ceef74439fea2159d1f769e 2019-09-17 18:33:21 +0200 to 2019-10-12 19:52:40 +0200 - further clarify C11 and C/C++11 terminology (rust-lang/nomicon#169) - atomics: C11 -> C++20 (rust-lang/nomicon#168) - use sound/unsound terminology ## cargo 12 commits in a429e8cc4614a46a86322a0777a477e2baa83f1c..3a9abe3f065554a7fbc59f440df2baba4a6e47ee 2019-10-04 17:36:12 +0000 to 2019-10-15 15:55:35 +0000 - Fix typo in git index initialization error path (rust-lang/cargo#7512) - Reject feature flags in a virtual workspace. (rust-lang/cargo#7507) - Rename `overrides` to `package` in profiles. (rust-lang/cargo#7504) - Allow publishing with dev-dependencies without a version. (rust-lang/cargo#7333) - Stabilize cache-messages (rust-lang/cargo#7450) - don't lock the package cache when cleaning target dir. (rust-lang/cargo#7502) - Document rustc wrapper (rust-lang/cargo#7499) - Migrate towards exclusively using serde for `Config` (rust-lang/cargo#7456) - Re-enable some MSVC tests. (rust-lang/cargo#7492) - when -Z unstable-options not specified, don't validate --profile (rust-lang/cargo#7489) - Improve error message for cyclic dependencies (rust-lang/cargo#7470) - Some minor clippy fixes. (rust-lang/cargo#7484) ## book 7 commits in 04806c80be0f54b1290287e3f85e84bdfc0b6ec7..9bb8b161963fcebc9d9ccd732ba26f42108016d5 2019-10-01 20:20:22 -0400 to 2019-10-14 18:42:55 -0500 - Make a portion of text less ambiguous (rust-lang/book#2092) - fix heading level (rust-lang/book#2117) - Add missing "of" before `"duck typing"`. (rust-lang/book#1951) - ch18-03: no need to debug print destructured int (rust-lang/book#1991) - Subtle fix to introduce ? on Option in Chapter 9.2 (rust-lang/book#2047) - make wording clearer (rust-lang/book#1976) - Update the version of rand we use ## rust-by-example 5 commits in a6288e7407a6c4c19ea29de6d43f40c803883f21..0b111eaae36cc4b4997684be853882a59e2c7ca7 2019-10-01 10:09:14 -0300 to 2019-10-14 18:34:25 -0300 - Some fix to three files (rust-lang/rust-by-example#1280) - Add reference to Generics (rust-lang/rust-by-example#1281) - Confusing and long sentence (rust-lang/rust-by-example#1282) - Explicit mention of slice range meaning (rust-lang/rust-by-example#1277) - Updated aliasing for nll (rust-lang/rust-by-example#1276)

rust-highfive assigned nrc Sep 27, 2019

rust-highfive assigned ehuss and unassigned nrc Sep 27, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 27, 2019

alexcrichton force-pushed the config-only-serde branch from a61edd4 to 31c313e Compare October 3, 2019 19:19

ehuss reviewed Oct 4, 2019

View reviewed changes

alexcrichton force-pushed the config-only-serde branch from 31c313e to 37d076c Compare October 8, 2019 00:11

alexcrichton added 19 commits October 7, 2019 17:12

Move config.rs to config/mod.rs

022814d

Extra serde config support to a separate file

0f73320

Refactor ConfigKey to its own file

c0baf84

Also make it a little less allocation-heavy by tweaking the API to encourage incremental building of the key and incremental destruction as we walk throughout the configuration tree.

Remove a usage of get_list

5bba426

This callsite doesn't need the full power of `get_list`, knowing the definition path of each element along the list.

Centralize HTTP configuration in one struct

2357bb0

Gives us one nice place to access and document all HTTP-related configuration

Access term config through a deserialized type

cbda532

Going through and removing users of raw `get_*` functions!

Consolidate net configuration into a typed structure

8d65906

Less need for `get_bool` and friends!

Remove no-longer-needed Config::get_bool

a458cd8

Remove dead Config::get_i64 method

09d9165

Consolidate build key configuration

d7d8ca1

Add a typed structure which lists all `build` key configuration throughout Cargo.

Simplify cargo-new configuration reading

44a31d6

No need for lots of extra helpers/parsing when using serde!

Handle rebase conflicts

fe7b5fc

Run rustfmt

3a6cd74

Use #[serde(rename_all)] instead of listing them all

dab42bb

Add documentation for StringList

a0d94ff

Remove unnecessary clone

0a9a46e

Document the new ConfigKey implementation

2387e1a

Add comments of Value<T>'s deserialization protocol

e0667f5

alexcrichton force-pushed the config-only-serde branch from 37d076c to e0667f5 Compare October 8, 2019 00:12

ehuss reviewed Oct 8, 2019

View reviewed changes

More review comments

9a12e48

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 Oct 9, 2019

bors merged commit 9a12e48 into rust-lang:master Oct 9, 2019

bors deleted the config-only-serde branch October 9, 2019 17:29

ehuss mentioned this pull request Oct 15, 2019

Update cargo, books rust-lang/rust#65445

Merged

ehuss mentioned this pull request Jan 30, 2020

Cargo makes target-dir path relative to user's home directory #7843

Open

ehuss added this to the 1.40.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate towards exclusively using serde for `Config` #7456

Migrate towards exclusively using serde for `Config` #7456

alexcrichton commented Sep 27, 2019

alexcrichton commented Sep 27, 2019

rust-highfive commented Sep 27, 2019

bors commented Sep 30, 2019

alexcrichton commented Oct 3, 2019

ehuss left a comment

alexcrichton commented Oct 8, 2019

ehuss left a comment

ehuss Oct 8, 2019

ehuss Oct 8, 2019

alexcrichton commented Oct 9, 2019

bors commented Oct 9, 2019

bors commented Oct 9, 2019

bors commented Oct 9, 2019

	// The current environment variable this configuration key maps to. This is
	// The current toml key this configuration key maps to. This is

Migrate towards exclusively using serde for Config #7456

Migrate towards exclusively using serde for Config #7456

Conversation

alexcrichton commented Sep 27, 2019

alexcrichton commented Sep 27, 2019

rust-highfive commented Sep 27, 2019

bors commented Sep 30, 2019

alexcrichton commented Oct 3, 2019

ehuss left a comment

Choose a reason for hiding this comment

alexcrichton commented Oct 8, 2019

ehuss left a comment

Choose a reason for hiding this comment

ehuss Oct 8, 2019

Choose a reason for hiding this comment

ehuss Oct 8, 2019

Choose a reason for hiding this comment

alexcrichton commented Oct 9, 2019

bors commented Oct 9, 2019

bors commented Oct 9, 2019

bors commented Oct 9, 2019

Migrate towards exclusively using serde for `Config` #7456

Migrate towards exclusively using serde for `Config` #7456