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/nixpkgs: add option to specify Nixpkgs source path #373201

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Jan 12, 2025

A non-breaking and (IMHO) simpler alternative to #333788 for setting the Nixpkgs versions declaratively from configuration.nix.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change (nixos-rebuild, nixos-rebuild-ng)
  • Tested basic functionality of all binary files
  • 25.05 Release Notes (or backporting 24.11 and 25.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.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 8.has: changelog 8.has: module (update) This PR changes an existing module in `nixos/` labels Jan 12, 2025
@nix-owners nix-owners bot requested a review from thiagokokada January 12, 2025 14:32
Copy link
Contributor

@thiagokokada thiagokokada left a comment

Choose a reason for hiding this comment

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

Can't comment too much about the change itself since I use Flakes, but at least the nixos-rebuild-ng changes looks good to me.

Left a comment about the tests mocks but this is not a merge blocker.

Copy link
Contributor

@amozeo amozeo left a comment

Choose a reason for hiding this comment

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

Hey, nixpkgs.source option in nixos is an interesting approach to pin nixpkgs for a given nixos configuration. I found several issues with your implementation of it.

I cannot review changes in nixos-rebuild-ng because I am not familiar with its code.

in nixos-rebuild.sh, there is also one more instance of getting nixpkgs from nix path in line 646 (after changes). I think you forgot about that occurence.

nixos/modules/misc/nixpkgs.nix Outdated Show resolved Hide resolved
nixos/modules/misc/nixpkgs.nix Outdated Show resolved Hide resolved
nixos/modules/misc/nixpkgs.nix Outdated Show resolved Hide resolved
nixos/modules/misc/nixpkgs.nix Outdated Show resolved Hide resolved
pkgs/os-specific/linux/nixos-rebuild/nixos-rebuild.sh Outdated Show resolved Hide resolved
pkgs/os-specific/linux/nixos-rebuild/nixos-rebuild.sh Outdated Show resolved Hide resolved
pkgs/os-specific/linux/nixos-rebuild/nixos-rebuild.sh Outdated Show resolved Hide resolved
@amozeo
Copy link
Contributor

amozeo commented Jan 12, 2025

I just noticed that there is option nixpkgs.flake.sources that tries to hold similar information as the option that you want to add, nixpkgs.source, maybe we want to deprecate it/change it/reuse it? doesn't look like it's used anywhere else in nixos module system except that file where is declared.

@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Jan 12, 2025

Hey, nixpkgs.source option in nixos is an interesting approach to pin nixpkgs for a given nixos configuration.
I found several issues with your implementation of it.

Thank you for reviewing!

in nixos-rebuild.sh, there is also one more instance of getting nixpkgs from nix path in line 646 (after changes). I think you forgot about that occurence.

Yes, I missed it because I was looking only for the angle brackets syntax.

@rnhmjoj rnhmjoj force-pushed the pr-nixpkgs-source branch 4 times, most recently from f4152b0 to 2c1cfe0 Compare January 13, 2025 09:13
@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Jan 13, 2025

I just noticed that there is option nixpkgs.flake.sources that tries to hold similar information

I think we should unify the two options.

@github-actions github-actions bot added the 6.topic: flakes The experimental Nix feature label Jan 13, 2025
@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Jan 13, 2025

I made an attempt to unify flakes and non-flakes options in 0fca667. The flake part is completely untested for now.

Copy link
Contributor

@thiagokokada thiagokokada left a comment

Choose a reason for hiding this comment

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

nixos-rebuild-ng part LGTM, but not tested.

Also didn't review the other changes.

nixos/modules/misc/nixpkgs-pin.nix Outdated Show resolved Hide resolved
pkgs/os-specific/linux/nixos-rebuild/nixos-rebuild.sh Outdated Show resolved Hide resolved
pkgs/os-specific/linux/nixos-rebuild/nixos-rebuild.sh Outdated Show resolved Hide resolved
@amozeo amozeo added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 14, 2025
@rnhmjoj rnhmjoj force-pushed the pr-nixpkgs-source branch 2 times, most recently from 03adbc7 to b233775 Compare January 15, 2025 13:45
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 15, 2025
@rnhmjoj rnhmjoj force-pushed the pr-nixpkgs-source branch 2 times, most recently from b119c71 to 61e4dbe Compare January 19, 2025 09:53
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 19, 2025
This moves the options under `nixpkgs.flake` to `nixpkgs` and
generalises them to work with systems built without flakes.

Specifically, nixpkgs.source now allows to pin the version of nixpkgs
used by nixos-rebuild to build the system, even without flakes.
This option can be used by other NixOS modules to detect whether the
system is being built from a flake.
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 19, 2025
Comment on lines +236 to +242
if nixpkgsPath=$(test -n "$NIXOS_CONFIG" && runCmd nix-instantiate --eval --expr "
(import <nixpkgs/nixos> {
configuration = {
imports = [ $NIXOS_CONFIG ];
_module.check = false;
};
}).config.nixpkgs.source
Copy link
Member

Choose a reason for hiding this comment

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

I'm really not a fan of this. It's fundamentally impossible to make configuration.nix specify Nixpkgs and it be used consistently, you always end up with the same problematic cases, such as:

  • A separate Nixpkgs is needed to "bootstrap" the eval, like the <nixpkgs> here. And this is not reproducible in general.
  • Upgrades across Nixpkgs versions are problematic, because some module might not evaluate with both the older and newer Nixpkgs (the _module.check can't ignore everything)

The real solution is to abandon the idea of "configuration.nix can reproduce the system" and switch to "system.nix/flake.nix/default.nix can reproduce the system", where that entry-point file depends on configuration.nix. That's something that Flakes got right, and it's also the direction I've indicated with #333788. This way there's none of the above problems.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, I kinda think of the non Flakes way as legacy and so large changes specific for it shouldn't be made. Flakes is the direction a lot of things are going and just manages it a lot cleaner.

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 necessarily agree with that. I don't want people to have to buy into Flakes just to have an eval-reproducible system. It's very easy to make something similar work in traditional Nix, it's not a large change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's fundamentally impossible to make configuration.nix specify Nixpkgs and it be used consistently

My initial proposal was to use callPackage (or similar) to fill in the arguments of configuration.nix and extract nixpkgs.source, without using evalModules. This works reliably with the only limitation that nixpkgs.source must not depend on any of the module arguments and be set directly in configuration.nix.

The real solution is to abandon the idea of "configuration.nix can reproduce the system"

Yes, but even then it's a letdown that you can't have a self-contained expression to describe a system reproducibly.
It means you'll never be able to just copy-paste, build and get the same system.

Copy link
Member

@infinisil infinisil Jan 20, 2025

Choose a reason for hiding this comment

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

Nah it's totally possible. With #333788, you can write a /etc/nixos/system.nix as follows:

let
  nixpkgs = fetchTarball { ... };
in
import (nixpkgs + "/nixos") {
  configuration = {
    environment.systemPackages = [ ... ];
  };
  # Or if you want to have the traditional `configuration.nix`:
  # configuration = ./configuration.nix;
}

And nixos-rebuild switch works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: flakes The experimental Nix feature 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants