-
-
Notifications
You must be signed in to change notification settings - Fork 467
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
{ids,checks}: update for new builder UID/GID values #1069
Conversation
cc @cole-h too – I didn’t bother doing any special handling for the temporary Sequoia‐only values the Determinate Systems installer used for a bit and so it will probably recommend those users run the upstream migration script, which I think should just work, even though it’s not strictly required? |
Includes the Sequoia installer fixes.
Checking for the Sequoia stuff won’t work properly if a system is still in this old state. Best to be loud about it to deal with any straggler systems that haven’t yet dealt with this issue.
3313a12
to
1554d8b
Compare
Rebased to pick up the test fixes in #1068 so that hopefully CI will be all green. |
1554d8b
to
88b97aa
Compare
Rearranged the checks to fail with a more useful message if there are no build users. |
- name: Install nix corresponding to latest stable channel | ||
uses: cachix/install-nix-action@v23 | ||
uses: DeterminateSystems/nix-installer-action@main | ||
with: | ||
install_url: https://releases.nixos.org/nix/nix-2.13.6/install | ||
nix-package-url: https://releases.nixos.org/nix/nix-2.18.5/nix-2.18.5-x86_64-darwin.tar.xz |
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.
I think it would be better to have a job for each installer rather than replacing the installer in this job
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.
We have no choice until the Nix team release a 2.18 with the fix, per the comment. This is just a hack taking advantage of the fact that the DetSys installer already deployed the fix and can install multiple Nix versions.
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.
Currently we're still running the tests on macOS 12, so adding a second job seems fine to me, even if the Nix installer doesn't support the Sequoia UID migration yet
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.
The test will fail because this PR means it will print the message informing that the migration script has to be run and error out. Which is the correct behaviour to ensure that users are prepared for the Sequoia update, but doesn't test what we want.
Note that the upgrade breaks Nix entirely if no migration was done and the user will not even be able to activate nix-darwin to see the message. That's why it's urgent that people are informed and run the migration script before upgrading to Sequoia.
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.
How about in a different job running the official Nix installer then the migration script in CI and then try to do installation again? If this is too much extra work, we can leave it out
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.
That could work, but it seemed more fiddly than just changing a few lines until the patched installer was out. If you'd prefer that I can try to implement it tomorrow, but I'll be busy for most of the day and I'm worried about how many users we'll be able to get this change in front of before Sequoia drops. I lean towards iterating on non-essential things after merge.
@@ -4,7 +4,7 @@ on: | |||
push: | |||
|
|||
env: | |||
CURRENT_STABLE_CHANNEL: nixpkgs-23.11-darwin | |||
CURRENT_STABLE_CHANNEL: nixpkgs-24.05-darwin |
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.
The manual should be updated to 24.05 as well
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.
It already was, I think.
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.
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.
Ah, I thought you meant the README. I can handle this but would prefer to do it in a separate PR given the urgency of this one.
- name: Install nix corresponding to latest stable channel | ||
uses: cachix/install-nix-action@v23 | ||
uses: DeterminateSystems/nix-installer-action@main |
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.
uses: DeterminateSystems/nix-installer-action@main | |
uses: DeterminateSystems/nix-installer-action@v14 |
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.
They recommend deploying the action directly from main
in their documentation. I can adjust this if you think it's important, though I expect we're only talking a matter of days before a fixed 2.18 installer we can use is released and I'm not at a computer right now so it'll have to wait until tomorrow.
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.
FWIW the action downloads an unversioned installer anyway so there's no meaningful pinning to be done here.
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.
I'm fine with these changes being put in another PR, I think we will be able to pin both the nix-installer binary used and the Nix version installed using source-url
and nix-package-url
, the goal for me is that we have both installers being tested in the CI eventually
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.
The upstream installer only supports installing one version of Nix, AFAIK. Again this is just a temporary hack for a few days to ensure that we can test 2.18 at all. If we tried to use the upstream installer we could not activate the system. This was the only way I could make the tests not go red because of this PR.
- name: Install nix corresponding to latest stable channel | ||
uses: cachix/install-nix-action@v23 | ||
uses: DeterminateSystems/nix-installer-action@main |
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.
uses: DeterminateSystems/nix-installer-action@main | |
uses: DeterminateSystems/nix-installer-action@v14 |
# Sequoia UID/GID changes have not yet been backported to the | ||
# official installer for that version. | ||
- name: Install nix corresponding to latest stable channel | ||
uses: DeterminateSystems/nix-installer-action@main |
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.
uses: DeterminateSystems/nix-installer-action@main | |
uses: DeterminateSystems/nix-installer-action@v14 |
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.
LGTM
Tested this locally and the checks work 👍
Even though I don't have nix.configureBuildUsers set[1], the increment communicates a known level of compatibility with the future macOS Sequoia. I ran Nix's Sequoia migration script[2] manually prior to any OS update for good measure. $ curl -L https://raw.githubusercontent.com/NixOS/nix/f2e7e996/scripts/sequoia-nixbld-user-migration.sh | bash Attempting to migrate _nixbld users. Step 1: move existing _nixbld users out of the destination UID range. Password: Temporarily moved _nixbld1 from uid 301 -> 31000 Temporarily moved _nixbld2 from uid 302 -> 31001 Temporarily moved _nixbld3 from uid 303 -> 31002 Temporarily moved _nixbld4 from uid 304 -> 31003 Temporarily moved _nixbld5 from uid 305 -> 31004 Temporarily moved _nixbld6 from uid 306 -> 31005 Temporarily moved _nixbld7 from uid 307 -> 31006 Temporarily moved _nixbld8 from uid 308 -> 31007 Temporarily moved _nixbld9 from uid 309 -> 31008 Temporarily moved _nixbld10 from uid 310 -> 31009 Temporarily moved _nixbld11 from uid 311 -> 31010 Temporarily moved _nixbld12 from uid 312 -> 31011 Temporarily moved _nixbld13 from uid 313 -> 31012 Temporarily moved _nixbld14 from uid 314 -> 31013 Temporarily moved _nixbld15 from uid 315 -> 31014 Temporarily moved _nixbld16 from uid 316 -> 31015 Temporarily moved _nixbld17 from uid 317 -> 31016 Temporarily moved _nixbld18 from uid 318 -> 31017 Temporarily moved _nixbld19 from uid 319 -> 31018 Temporarily moved _nixbld20 from uid 320 -> 31019 Temporarily moved _nixbld21 from uid 321 -> 31020 Temporarily moved _nixbld22 from uid 322 -> 31021 Temporarily moved _nixbld23 from uid 323 -> 31022 Temporarily moved _nixbld24 from uid 324 -> 31023 Temporarily moved _nixbld25 from uid 325 -> 31024 Temporarily moved _nixbld26 from uid 326 -> 31025 Temporarily moved _nixbld27 from uid 327 -> 31026 Temporarily moved _nixbld28 from uid 328 -> 31027 Temporarily moved _nixbld29 from uid 329 -> 31028 Temporarily moved _nixbld30 from uid 330 -> 31029 Temporarily moved _nixbld31 from uid 331 -> 31030 Temporarily moved _nixbld32 from uid 332 -> 31031 Step 2: re-create missing early _nixbld# users. Step 3: relocate remaining _nixbld# UIDs to 351+ _nixbld1 migrated to uid: 351 _nixbld2 migrated to uid: 352 _nixbld3 migrated to uid: 353 _nixbld4 migrated to uid: 354 _nixbld5 migrated to uid: 355 _nixbld6 migrated to uid: 356 _nixbld7 migrated to uid: 357 _nixbld8 migrated to uid: 358 _nixbld9 migrated to uid: 359 _nixbld10 migrated to uid: 360 _nixbld11 migrated to uid: 361 _nixbld12 migrated to uid: 362 _nixbld13 migrated to uid: 363 _nixbld14 migrated to uid: 364 _nixbld15 migrated to uid: 365 _nixbld16 migrated to uid: 366 _nixbld17 migrated to uid: 367 _nixbld18 migrated to uid: 368 _nixbld19 migrated to uid: 369 _nixbld20 migrated to uid: 370 _nixbld21 migrated to uid: 371 _nixbld22 migrated to uid: 372 _nixbld23 migrated to uid: 373 _nixbld24 migrated to uid: 374 _nixbld25 migrated to uid: 375 _nixbld26 migrated to uid: 376 _nixbld27 migrated to uid: 377 _nixbld28 migrated to uid: 378 _nixbld29 migrated to uid: 379 _nixbld30 migrated to uid: 380 _nixbld31 migrated to uid: 381 _nixbld32 migrated to uid: 382 Migrated 32 users. If you want to double-check, try: dscl . list /Users UniqueID | grep _nixbld | sort -n -k2 [1]: LnL7/nix-darwin#1069 [2]: NixOS/nix#11075
Hi @emilazy , after a error:
… while evaluating the attribute 'value'
at /nix/store/nh98b9i03r9mq43bclkxvbk3n346jhqr-source/lib/modules.nix:821:9:
820| in warnDeprecation opt //
821| { value = addErrorContext "while evaluating the option `${showOption loc}':" value;
| ^
822| inherit (res.defsFinal') highestPrio;
… while calling the 'addErrorContext' builtin
at /nix/store/nh98b9i03r9mq43bclkxvbk3n346jhqr-source/lib/modules.nix:821:17:
820| in warnDeprecation opt //
821| { value = addErrorContext "while evaluating the option `${showOption loc}':" value;
| ^
822| inherit (res.defsFinal') highestPrio;
(stack trace truncated; use '--show-trace' to show the full trace)
error:
Failed assertions:
- The `system.stateVersion` option is not defined in your
nix-darwin configuration. The value is used to conditionalize
backwards‐incompatible changes in default settings. You should
usually set this once when installing nix-darwin on a new system
and then never change it (at least without reading all the relevant
entries in the changelog using `darwin-rebuild changelog`).
You can use the current value for new installations as follows:
system.stateVersion = 5; $ nix-info -m
- system: `"aarch64-darwin"`
- host os: `Darwin 23.6.0, macOS 14.7`
- version: `nix-env (Nix) 2.21.1`
- nixpkgs: `/Users/user/.nix-defexpr/channels/nixpkgs` $ nix config check
[FAIL] Multiple versions of nix found in PATH:
/nix/store/ip0chiw6dkpz161mkkjvdsp8gb16wmf4-nix-2.21.1/bin
/nix/store/ma0p24nzjcylflyn4bz4wj8kf28a7ida-nix-2.18.5/bin
[PASS] All profiles are gcroots.
[PASS] Client protocol matches store protocol.
[INFO] You are trusted by store uri: local inputs = {
nixpkgs.url = "github:nixos/nixpkgs/nixpkgs-unstable";
home-manager = {
url = "github:nix-community/home-manager";
inputs.nixpkgs.follows = "nixpkgs";
};
darwin = {
url = "github:LnL7/nix-darwin/master";
inputs.nixpkgs.follows = "nixpkgs";
};
}; homeConfigurations.home.stateVersion = "24.05"; |
See:
_nixbld{1,2,3,4}
with system users causingdarwin-rebuild
to fail #970sequoia
subcommand that can migrate build users to the new 351+ UID range DeterminateSystems/nix-installer#1143This is fairly subtle stuff, but also time‐critical; Apple has given us an unexpected gift by announcing that Sequoia will release on Monday, making it the earliest macOS release in over a decade. Currently, Nix users upgrading to Sequoia get a broken installation that outputs a useless error message, and users who have already migrated their users can’t activate their configuration if they manage Nix users with nix-darwin.
I wanted to do a “proper” migration based on our existing user management functionality, but we simply don’t have the time. The best thing we can do for users to reduce the support burden and reputation hit of everything breaking is to get the upstream migration script in front of them ASAP. Therefore, this adds support for the new values, and adds checks when the UIDs/GID aren’t as expected that tell the user what they can do to remediate. I want to get this merged within 24 hours if possible.
I had to do some ugly work to the CI to get it to pass, as Nix have not yet released fixed installers for versions other than 2.24. But it does pass, and I have verified that all the main code paths here seem to behave as expected.
Sorry for neglecting nix-darwin recently. Now that I’m slightly less busy with Nixpkgs work I hope to do more soon, but this is really urgent so I dropped everything to get it out.
Thanks to @mjm for helping test this.
cc @Enzime @Samasaur1 @malob @LnL7 for code review
cc @abathur for feedback on error message wording
Closes: #970