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

nixos/top-level: add includeBuildDependencies option #195511

Merged
merged 1 commit into from
Mar 15, 2023

Conversation

lheckemann
Copy link
Member

Description of changes

This option allows adding the build closure of the system to its runtime closure, enabling fully-offline rebuilds (as long as no new packages are added).

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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@lheckemann lheckemann requested a review from dasJ as a code owner October 11, 2022 10:26
@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 Oct 11, 2022
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Oct 11, 2022
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.

I like the idea.

It's a problem I've also encountered and solved independently for nixos/tests/nixops/default.nix, but that solution required virtualisation.additionalPaths; so not immediately transferable to NixOS itself. I'm glad to see a more general and cleaner solution.
If I'm not mistaken, the nixops test could be supported the following instead of my hacky solution:

  deployer.system.extraDependencies = nodes.server.config.system.build.toplevel;
  server.system.includeBuildDependencies = true;

You might also be interested in what I've written down for a more efficient solution in #180529, but let's not work on that in this PR.

nixos/modules/system/activation/top-level.nix Outdated Show resolved Hide resolved
nixos/modules/system/activation/top-level.nix Show resolved Hide resolved
@lheckemann lheckemann force-pushed the include-build-deps branch from 0180978 to abac59a Compare March 4, 2023 01:27
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 4, 2023
@lheckemann lheckemann force-pushed the include-build-deps branch from abac59a to 3ac0784 Compare March 4, 2023 02:17
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 4, 2023
@lheckemann lheckemann force-pushed the include-build-deps branch from 3ac0784 to cd10904 Compare March 4, 2023 02:29
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.

Looks good to me.

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.

Just some doc improvements, because I think we should be more explicit about what happens.

Comment on lines 320 to 323
Whether to include the build closure of the whole system in
its runtime closure. This can be useful for making changes
fully offline, as it results in all sources and patches
required to build the system being included.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Whether to include the build closure of the whole system in
its runtime closure. This can be useful for making changes
fully offline, as it results in all sources and patches
required to build the system being included.
Whether to include the build closure of the whole system in
its runtime closure. This can be useful for making changes
fully offline, as it includes all sources, patches and intermediate
outputs that are required to build all the derivations.
All derivations really means all derivations, from the bootstrapping
tools, to the build tools, to the packages, and all the way up to the
configuration files of the system.

fully offline, as it results in all sources and patches
required to build the system being included.

Note that this increases closure size drastically: a minimal
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Note that this increases closure size drastically: a minimal
This increases the system's size drastically: a minimal

... or do some people scan for "Note" in text? I don't think I do.

Copy link
Member

Choose a reason for hiding this comment

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

(I do, but I am fine either way)

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Note that this increases closure size drastically: a minimal
Note that this increases the system size drastically: a minimal

@roberth
Copy link
Member

roberth commented Mar 5, 2023

deployer.system.extraDependencies = nodes.server.config.system.build.toplevel;
server.system.includeBuildDependencies = true;

This works when enabled on the deployer instead, which makes more sense anyway.

@roberth
Copy link
Member

roberth commented Mar 5, 2023

You may include my nixops test patch, although I'd prefer a simple stand-alone test that doesn't involve a specific deployment tool, but just NixOS.

0001-nixosTests.nixops.unstable.legacyNetwork-Use-system..patch.txt

Refs

@lheckemann lheckemann force-pushed the include-build-deps branch from cd10904 to 44b293b Compare March 8, 2023 16:45
the compilers. This increases the size and build/download time
of the system drastically: a minimal configuration with no
extra services enabled grows from ~670MiB in size to 13.5GiB,
and takes proportionally longer to build.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the build is much slower, is it? Isn't this more about substituting many more dependencies?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're absolutely right. Building the system entirely from scratch without substitution should take pretty much the same amount of time as before.

This option allows adding the build closure of the system to its
runtime closure, enabling fully-offline rebuilds (as long as no new
packages are added).
@roberth roberth merged commit c3b245d into NixOS:master Mar 15, 2023
@roberth
Copy link
Member

roberth commented Mar 15, 2023

Thanks @lheckemann for polishing the implementation. I'll add this to the nixops test so that it's tested at least somewhere.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/declarative-vms-ubuntu-on-top-of-nixos/34592/2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants