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.

Sign up for GitHub

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

Jump to bottom

nixos/system: Support pre-activated images #237040

Merged
merged 8 commits into from
Jul 8, 2023
Merged

nixos/system: Support pre-activated images #237040

merged 8 commits into from
Jul 8, 2023

Conversation

roberth
Copy link
Member

@roberth roberth commented Jun 10, 2023 •
edited
Loading

Description of changes

Mostly refactoring, except for the last commit which introduces the feature.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@roberth roberth requested a review from dasJ as a code owner June 10, 2023 13:39
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jun 10, 2023
@roberth
Copy link
Member Author

roberth commented Jun 10, 2023 •
edited
Loading

The test that tests the feature:

@ofborg test switchTest

... and some other tests

@ofborg test simple
@ofborg test nixos-rebuild-specialisations

@roberth roberth changed the title Flexible activation nixos: Small refactor towards more flexible activation Jun 10, 2023
RaitoBezarius
RaitoBezarius reviewed Jun 10, 2023
View reviewed changes
nixos/modules/system/activation/activatable-system.nix Outdated
mkdir $out/bin
substituteAll ${./switch-to-configuration.pl} $out/bin/switch-to-configuration
chmod +x $out/bin/switch-to-configuration
${optionalString (pkgs.stdenv.hostPlatform == pkgs.stdenv.buildPlatform) ''
Copy link
Member

Choose a reason for hiding this comment

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

Don't you only need to have perl available on the hostPlatform ?

Copy link
Member

Choose a reason for hiding this comment

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

(or on host and build?)

Copy link
Member Author

Choose a reason for hiding this comment

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

@perl@ is substituted in switch-to-configuration.pl.
substituteAll obscures that. Would be nice to be explicit about the variables.

Copy link
Member

Choose a reason for hiding this comment

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

Apologies, my comment was about whether you needed to guard on same host platform and build platform to run the Perl check and I was wondering as Perl is like just checking a script if it could run beyond that condition.

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose you could bring a separate perl for that purpose, but I doubt that it'd be worth the effort.

Copy link
Member

Choose a reason for hiding this comment

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

That's fair

@roberth
Copy link
Member Author

roberth commented Jun 10, 2023 •
edited
Loading

This has uncovered some previously broken tests, but works with my fixes applied:

Don't know how to fix (but not a blocker):

roberth added 3 commits June 10, 2023 19:15
@roberth
nixos: Extract module for activation script inclusion into toplevel
61d43de 
Allows omission of this functionality through disabledModules, e.g.
for image building.
@roberth
nixos: Move installBootLoader to activation script modules
a16986f
@roberth
nixos/activatable-system: Make substitutions explicit
193f4fe 
This helps with understanding the code.
We might make this not depend on environment variables later.
systemBuilderArgs is a form of global state, which isn't helpful.
This was referenced Jun 10, 2023
Closed
Open
@roberth roberth force-pushed the flexible-activation branch from 53c2f97 to 2e234a6 Compare June 21, 2023 15:38
roberth added 5 commits June 28, 2023 12:42
@roberth
nixos/activatable-system: Make system builder commands env independent
990b72f 
This way it will be easier to reuse in a different context, such as
a separate build of the activation script by itself (TBD).
@roberth
nixos/activatable-system: Move legacy variables to top-level
7891c8c
@roberth
nixos/top-level: Remove unused builder variables
9edad17 
These variables were previously used by the activation script
build commands, but are now embedded into those commands for
to improve reusability for an upcoming addition.
@roberth
nixos/tests/switch-test.nix: Fix warnings
8966419
@roberth
nixos: Add system.activatable flag for images that are pre-activated
772d607
@roberth roberth force-pushed the flexible-activation branch from 2e234a6 to 772d607 Compare June 28, 2023 12:07
@roberth roberth changed the title nixos: Small refactor towards more flexible activation nixos: Support pre-activated images Jun 28, 2023
@roberth roberth changed the title nixos: Support pre-activated images nixos/system: Support pre-activated images Jun 28, 2023
@roberth
Copy link
Member Author

roberth commented Jun 28, 2023

I wasn't happy about the lack of validation or test, so I did the thing, not just refactor prep anymore. See last commit.

@RaitoBezarius
Copy link
Member

RaitoBezarius commented Jun 28, 2023 via email

@roberth
Copy link
Member Author

roberth commented Jun 28, 2023

👍 No hurry

RaitoBezarius
RaitoBezarius approved these changes Jul 8, 2023
View reviewed changes
Copy link
Member

@RaitoBezarius RaitoBezarius left a comment

Choose a reason for hiding this comment

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

Awesome, this looks good to me. (first read)
I will take it for a walk later on actual systems.

@RaitoBezarius RaitoBezarius requested a review from nikstur July 8, 2023 13:52
@roberth roberth merged commit 3fd4ac8 into NixOS:master Jul 8, 2023
@roberth
Copy link
Member Author

roberth commented Jul 8, 2023

Thanks!

@tomeon tomeon mentioned this pull request Dec 16, 2023
Open
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaitoBezarius RaitoBezarius RaitoBezarius approved these changes

@dasJ dasJ Awaiting requested review from dasJ

@nikstur nikstur Awaiting requested review from nikstur

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@roberth @RaitoBezarius