Use a .toml file to store settings #420

Diggsey · May 8, 2016

This doesn't bump the metadata version. Running rustup will silently convert your version, default and overrides files into a single settings.toml file.

If your metadata version is older than "12" it will first be converted to the new format, and then rustup will require you to perform a metadata upgrade exactly as it would before.

This paves the way for storing additional information in the settings file (such as the host target).

Diggsey · May 8, 2016

Example toml file:

default_toolchain = "nightly-x86_64-pc-windows-msvc"
version = "12"

[overrides]
"\\\\?\\C:\\Users\\diggs\\workspace\\multirust-rs" = "nightly-x86_64-pc-windows-msvc"

brson · May 8, 2016

brson reviewed May 8, 2016

View changes

brson · May 8, 2016

src/rustup/settings.rs

+
+    pub fn from_toml(mut table: toml::Table, path: &str) -> Result<Self> {
+        let version = try!(get_string(&mut table, "version", path));
+        if !SUPPORTED_METADATA_VERSIONS.contains(&&*version) {


ISTM that "2" shouldn't be considered valid here since there's never been a settings.toml containing version 2.

The upgrade to settings.toml is orthogonal to the metadata version: since it's lossless it gets converted automatically, so if you upgrade from an old version of multirust to rustup, it will result in a settings.toml file with a version of 2, and you will then be required to do a metadata upgrade.

I see. Thanks for the explanation.

brson · May 8, 2016

My only reluctance is that this commits us to deserializing a config file every time someone runs cargo or rustc, but I'm somewhat resigned to that fate. Most if this information must be read anyway so it could even be better to load it all at once. cc @alexcrichton

Can you also add the telemetry flag?

brson · May 8, 2016

brson reviewed May 8, 2016

View changes

brson · May 8, 2016

src/rustup/config.rs

+            let _ = utils::remove_file("version", &legacy_version_file);
+            let _ = utils::remove_file("default", &default_file);
+            let _ = utils::remove_file("overrides", &override_db);
+        }


Can you extract this into its own function?

brson · May 8, 2016

brson reviewed May 8, 2016

View changes

brson · May 8, 2016

src/rustup/settings.rs

+        Settings::parse(&content)
+    }
+    pub fn with<T, F: FnOnce(&Settings) -> Result<T>>(&self, f: F) -> Result<T> {
+        f(&try!(self.read_settings()))


I imagine this file is going to get read several times during each run. Can it instead load the file at startup and reuse it?

brson · May 8, 2016

Looks great. I do think it needs to take care to not read the file multiple times.

Will you also add a test that this conversion works? A simple way to do it is to modify the files in config.rustupdir to look like the old metadata and then test that the default and override work as expected, and that the files are deleted.

Diggsey · May 9, 2016

I think that's everything!

alexcrichton · May 9, 2016

My only reluctance is that this commits us to deserializing a config file every time someone runs cargo or rustc,

Yeah unfortunately toml-rs isn't exactly optimized for speed right now, but something like this shouldn't be that slow and it should be relatively easy to speed up toml-rs wherever possible (if necessary)

brson · May 10, 2016

@Diggsey ok, this looks fine by me. Before merging this big change and risking more breakage I want to get a build out that fixes the windows bustage. I'll merge after that.

Diggsey · May 10, 2016

Great!

bors · May 12, 2016

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

brson · May 12, 2016

brson reviewed May 12, 2016

View changes

brson · May 12, 2016

tests/cli-misc.rs

+                                      "a;nightly\nb;stable").unwrap();
+        rustup_utils::raw::write_file(&config.rustupdir.join("telemetry-on"), "").unwrap();
+        expect_err(config, &["rustup", "default", "nightly"],
+                   "rustup's metadata is out of date. run `rustup self upgrade-data`");


Why is this expecting an error message saying to run upgrade-data? Doesn't this upgrade happen silently?

I think this test should be tweaked just a little. Before writing the old metadata, first run the commands that correspond to the metadata, then write the metadata and delete the settings.toml file. Then run some rustup command to force the silent upgrade. Then run further rustc commands that verify that the metadata is intact - that rustc --version prints the right thing for the default and override (telemetry flag probably not worth testing explicitly).

This is testing an upgrade from metadata version 2, ie. an old metadata version, for which we currently require an explicit upgrade (because it wipes the toolchains). I can add a separate test for just the toml upgrade, but it would be a subset of what is tested here.

Diggsey · May 15, 2016

This is back up to date, just waiting on CI again.

Use a .toml file to store settings

Loading status checks…

d1ae0eb

Diggsey referenced this pull request May 8, 2016
Closed
Override remove from inner directories lies about successful removal #339

Diggsey added some commits May 9, 2016

Store telemetry mode in settings.toml and cache settings in memory

Loading status checks…

bbe8fd6

Extract conversion function and add test

Loading status checks…

8bc3ac9

brson force-pushed the master branch 2 times, most recently from 1acdeb2 to 9830d33 May 12, 2016

Diggsey added some commits May 15, 2016

Merge branch 'master' into toml-settings
# Conflicts: # Cargo.lock # src/rustup-utils/Cargo.toml # src/rustup-utils/src/lib.rs # src/rustup/config.rs

Loading status checks…

874bb39

Fix version

Loading status checks…

e16565b

Diggsey referenced this pull request May 16, 2016
Closed
Problematic test names on 32-bit windows #427

brson merged commit 66af8c1 into master May 17, 2016
2 checks passed

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

bors referenced this pull request May 17, 2016
Merged
Compile on stable Rust #476

Diggsey deleted the toml-settings branch Nov 21, 2016

rust-lang/rustup.rs

Join GitHub today

Use a .toml file to store settings #420

Conversation

Diggsey commented May 8, 2016

This comment has been minimized.

Diggsey commented May 8, 2016

Diggsey referenced this pull request May 8, 2016

brson reviewed May 8, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

brson commented May 8, 2016

brson reviewed May 8, 2016 View changes

This comment has been minimized.

brson reviewed May 8, 2016 View changes

This comment has been minimized.

This comment has been minimized.

brson commented May 8, 2016

Diggsey added some commits May 9, 2016

This comment has been minimized.

Diggsey commented May 9, 2016

This comment has been minimized.

alexcrichton commented May 9, 2016

This comment has been minimized.

brson commented May 10, 2016

This comment has been minimized.

Diggsey commented May 10, 2016

This comment has been minimized.

bors commented May 12, 2016

brson force-pushed the master branch 2 times, most recently from 1acdeb2 to 9830d33 May 12, 2016

brson reviewed May 12, 2016 View changes

This comment has been minimized.

This comment has been minimized.

Diggsey added some commits May 15, 2016

This comment has been minimized.

Diggsey commented May 15, 2016

Diggsey referenced this pull request May 16, 2016

Hide details View details brson merged commit 66af8c1 into master May 17, 2016 2 checks passed

2 checks passed

bors referenced this pull request May 17, 2016

Diggsey deleted the toml-settings branch Nov 21, 2016

brson reviewed May 8, 2016

View changes

brson reviewed May 8, 2016

View changes

brson reviewed May 8, 2016

View changes

brson force-pushed the master branch 2 times, most recently from `1acdeb2` to `9830d33` May 12, 2016

brson reviewed May 12, 2016

View changes

brson merged commit `66af8c1` into master May 17, 2016
2 checks passed