-
Notifications
You must be signed in to change notification settings - Fork 52
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
Make nix.conf changes deterministic #620
Conversation
if defaults_conf_setting.0.trim() == *existing_field | ||
&& defaults_conf_setting.1.trim() != existing_value.trim() | ||
{ | ||
tracing::warn!("Found existing `/etc/nix/nix.conf` setting `{existing_field} = {existing_value}` which will override a default setting from the `nix-installer`, consider unsetting it. For settings like `experimental-features` you can use an `extra-*` prefix to append to the defaults") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
This isn't necessarily true if they used an include
statement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True but I didn't really want to implement full tracking on where we include various settings from. If you think it's important, let's do it, otherwise I just wanted to keep the code simpler. It's my impression most people will see an error like this and go "Oh, that's an old install" and simply blow away the /etc/nix
folder.
I did a full VM test run and it passed, so that gives me some confidence this won't cause breakage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
Looks like we're gonna revert this and try something different. We saw an elevated rate of failures resulting from this ticket and reflecting some members of the team preferred the old merge method. |
This reverts commit 05571a4.
Description
Removes
nix-config-parser
.Uses an
!include ./defaults.conf
to include some defaults file which we control precisely in both content and order.Only manipulates the first line which is not a comment in
nix.conf
, or creates the file.Does a bit of rudimentary scanning of the file for possible conflicts, and warn if so.
Note: More testing to be done, though manual testing indicates it works well-ish.
Should address #562
Checklist
cargo fmt
nix build
nix flake check
Validating with
install.determinate.systems
If a maintainer has added the
upload to s3
label to this PR, it will become available for installation viainstall.determinate.systems
: