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

Use a .toml file to store settings #420

Merged
merged 5 commits into from
May 17, 2016
Merged

Use a .toml file to store settings #420

merged 5 commits into from
May 17, 2016

Conversation

Diggsey
Copy link
Contributor

@Diggsey Diggsey commented 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
Copy link
Contributor Author

Diggsey commented 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"


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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Thanks for the explanation.

@brson
Copy link
Contributor

brson commented 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?

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

Choose a reason for hiding this comment

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

Can you extract this into its own function?

@brson
Copy link
Contributor

brson commented 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
Copy link
Contributor Author

Diggsey commented May 9, 2016

I think that's everything!

@alexcrichton
Copy link
Member

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
Copy link
Contributor

brson commented 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
Copy link
Contributor Author

Diggsey commented May 10, 2016

Great!

@bors
Copy link
Contributor

bors commented May 12, 2016

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

@brson brson force-pushed the master branch 2 times, most recently from 1acdeb2 to 9830d33 Compare May 12, 2016 14:08
"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`");
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

# Conflicts:
#	Cargo.lock
#	src/rustup-utils/Cargo.toml
#	src/rustup-utils/src/lib.rs
#	src/rustup/config.rs
@Diggsey
Copy link
Contributor Author

Diggsey commented May 15, 2016

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

@brson brson merged commit 66af8c1 into master May 17, 2016
@bors bors mentioned this pull request May 17, 2016
@Diggsey Diggsey deleted the toml-settings branch November 21, 2016 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants