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

Add env variable to override the fallback settings file path #2545

Merged
merged 1 commit into from
Nov 17, 2020

Conversation

frazar
Copy link
Contributor

@frazar frazar commented Oct 31, 2020

This PR introduces the support for overriding the path of the fallback settings file (/etc/rustup/settings.toml) by specifying the RUSTUP_OVERRIDE_UNIX_FALLBACK_SETTINGS environment variable.

The fix can be verified by running the following commands

$ # Create an `/etc/rustup/settings.toml` file specifying a default toolchain
$ echo "default_toolchain = 'stable'" | sudo tee /etc/rustup/settings.toml
default_toolchain = 'stable'
$ cargo test update_once # Fails!
$ export RUSTUP_OVERRIDE_UNIX_FALLBACK_SETTINGS=/dev/null 
$ cargo test update_once # Does not fail

Fixes #2456

Issue #2456 also mentions that the env variable should be "set up in the mock test support glue", but I'm not sure what it means..

Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

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

Thank you for trying to fix this issue.

There's a minor snafu in your code which will be easy for you to fix, however the important reason for wanting this feature is so that our test code doesn't use /etc/rustup/settings.toml when testing rustup. In here: https://github.com/rust-lang/rustup/blob/master/tests/mock/clitools.rs#L410 you will see how we set up the environment to run test code, at minimum setting the environment variable to something we know won't exist might be a good starting point. However this could also be an opportunity to introduce the ability to test the fallback code.

src/config.rs Outdated Show resolved Hide resolved
@kinnison
Copy link
Contributor

kinnison commented Nov 1, 2020

While it pains me to say, you didn't format the code or test it before submitting that. There's a little more work you need to do, likely involving a bit of shifting around of the PathBuf::from and friends before it'll compile.

Also, have you given any thought to a way we might test this?

@frazar
Copy link
Contributor Author

frazar commented Nov 1, 2020

Sorry, I didn't mean to actually push this! It's still a work in progress, I'll update the PR title accordingly.

Thanks for your advices!

@frazar frazar changed the title Add env variable to override the fallback settings file path WIP Add env variable to override the fallback settings file path Nov 1, 2020
@frazar frazar changed the title WIP Add env variable to override the fallback settings file path Add env variable to override the fallback settings file path Nov 3, 2020
@frazar
Copy link
Contributor Author

frazar commented Nov 3, 2020

I think this PR is ready for a second review!

The unit test setup was updated to set the RUSTUP_OVERRIDE_UNIX_FALLBACK_SETTINGS variable with a bogus value, as suggested. This should make sure that the actual fallback settings file does not interefere with testing.

Also, a unit test was added for Unix plarforms where the fallback settings mechanism is tested.

Let me know if further changes are required e.g. squashing the commits.

Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

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

I think this looks good. @rbtcollins since this touches the process/env stuff I'd like you to just run your eye over it; but I think it's right.

@kinnison
Copy link
Contributor

Content-wise it looks good. If @rbtcollins is okay with it, then I'd like it rebased into a neater commit if you don't mind.

@frazar
Copy link
Contributor Author

frazar commented Nov 15, 2020

Commits squashed! Let me know if anything else needs fixing

@kinnison
Copy link
Contributor

I'm going to assume Robert is too busy right now, and so since I think it's good I'll merge it, thank you.

@kinnison kinnison merged commit fbccbc3 into rust-lang:master Nov 17, 2020
@frazar frazar deleted the fix-2456 branch November 17, 2020 10:16
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.

Insufficient test isolation for /etc/rustup/settings.toml files
2 participants