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

nix-profile: fix both profile links detection #9590

Merged
merged 3 commits into from
Jun 3, 2024
Merged

Conversation

wh0
Copy link
Contributor

@wh0 wh0 commented Dec 11, 2023

Motivation

In #7925, using "new nix link exists" instead of "old nix link doesn't exist" mistakenly made it impossible to trigger the "oh no, both links exist" logic. This restores that.

Context

What I gathered from the discussion in March, pushing harder to use the new link is not a priority. But the logic is here, so I want to let it work.

Priorities

Add 👍 to pull requests you find important.

@wh0 wh0 requested a review from edolstra as a code owner December 11, 2023 03:25
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

Could you add a test case?

scripts/nix-profile-daemon.sh.in Outdated Show resolved Hide resolved
@wh0
Copy link
Contributor Author

wh0 commented Dec 12, 2023

what invokes the tests?

@edolstra
Copy link
Member

@wh0 Something like:

nix build .#hydraJobs.installerTests.ubuntu-22-04.x86_64-linux.{install-default,install-force-daemon,install-force-no-daemon}

@roberth roberth marked this pull request as draft December 22, 2023 15:50
@wh0
Copy link
Contributor Author

wh0 commented Dec 29, 2023

come to think of it, Nix has already changed to using the new link when both exist, and no one has complained. I'm going to rework this to maintain that logic, only reinstating the both-links-exist diagnostic and updating the wording accordingly to reflect this new behavior.

analysis of the timeline:

  1. nixos 23.05 ships with nix 2.13
  2. nix 2.14 comes out with support for the new link (Follow XDG Base Directory standard #5588). I believe it creates the new link on new installs, from the if ! [ -e "$NIX_LINK" ]; then NIX_LINK="$NIX_LINK_NEW" logic. there is also a diagnostic that detects when both links are present, and it uses the old link in this case.
  3. nix 2.14.1 comes out with a change back to using the old link for new installs, if [ -e "$NIX_LINK_NEW" ]; then NIX_LINK="$NIX_LINK_NEW". however, this changes the logic to use the new link when both links are present.
  4. nixos 23.11 ships with nix 2.18, I think

updated goals:

  • create the old link for new installs where neither link initially exists
  • when both links exist, display the diagnostic message
  • when both links exist, use the new link

@wh0
Copy link
Contributor Author

wh0 commented Dec 29, 2023

@wh0 Something like:

nix build .#hydraJobs.installerTests.ubuntu-22-04.x86_64-linux.{install-default,install-force-daemon,install-force-no-daemon}

wow is that going to make me recompile the whole project each time I make a change to the test script? I don't think I'm going to be able to do this until a few more iterations of moore's law

@wh0 wh0 marked this pull request as ready for review December 29, 2023 07:19
@wh0
Copy link
Contributor Author

wh0 commented Dec 30, 2023

oh phew, it recompiles nix for each of the install tests, but I don't have to rebuild it each time I change a test.

added an installer test case with both links present

Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

This looks good now. Thank you, and sorry for the delay.

@roberth roberth merged commit de5050f into NixOS:master Jun 3, 2024
8 checks passed
@wh0 wh0 deleted the patch-1 branch June 3, 2024 14:58
@wh0
Copy link
Contributor Author

wh0 commented Jun 3, 2024

no worries, thanks for merging!

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-06-03-nix-team-meeting-minutes-149/46582/1

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