nixos/nixpkgs: add option to specify Nixpkgs source path #373201

rnhmjoj · 2025-01-12T14:30:28Z

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

Things done

Add a 👍 reaction to pull requests you find important.

thiagokokada · 2025-01-12T16:07:42Z

pkgs/by-name/ni/nixos-rebuild-ng/src/tests/test_models.py

+@patch(get_qualified_name(nr.process.subprocess.run), autospec=True)
+def test_build_attr_from_arg(mock_run: Any, tmp_path: Path) -> None:
+    nixpkgs_path = tmp_path / "nixpkgs"
+    mock_run.side_effect = [
+        # with config.nixpkgs.source
+        subprocess.CompletedProcess([], 0, f"\"{nixpkgs_path}\"\n"),
+        # without config.nixpkgs.source
+        subprocess.CompletedProcess([], 1, ""),
+    ]
+    assert m.BuildAttr.from_arg(None, None) == m.BuildAttr(str(nixpkgs_path / "nixos"), None)
    assert m.BuildAttr.from_arg(None, None) == m.BuildAttr("<nixpkgs/nixos>", None)


I think it is better to do a scoped patch here instead:

Suggested change

@patch(get_qualified_name(nr.process.subprocess.run), autospec=True)

def test_build_attr_from_arg(mock_run: Any, tmp_path: Path) -> None:

nixpkgs_path = tmp_path / "nixpkgs"

mock_run.side_effect = [

# with config.nixpkgs.source

subprocess.CompletedProcess([], 0, f"\"{nixpkgs_path}\"\n"),

# without config.nixpkgs.source

subprocess.CompletedProcess([], 1, ""),

]

assert m.BuildAttr.from_arg(None, None) == m.BuildAttr(str(nixpkgs_path / "nixos"), None)

assert m.BuildAttr.from_arg(None, None) == m.BuildAttr("<nixpkgs/nixos>", None)

def test_build_attr_from_arg(tmp_path: Path) -> None:

nixpkgs_path = tmp_path / "nixpkgs"

with patch(get_qualified_name(nr.process.subprocess.run), autospec=True, return_value=subprocess.CompletedProcess([], 0, f"\"{nixpkgs_path}\"\n")):

assert m.BuildAttr.from_arg(None, None) == m.BuildAttr(str(nixpkgs_path / "nixos"), None)

with patch(get_qualified_name(nr.process.subprocess.run), autospec=True, return_value=subprocess.CompletedProcess([], 1, "")):

assert m.BuildAttr.from_arg(None, None) == m.BuildAttr("<nixpkgs/nixos>", None)

Done, thank you!

thiagokokada

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.

amozeo

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.

amozeo · 2025-01-12T17:09:13Z

nixos/modules/misc/nixpkgs.nix

@@ -157,6 +157,30 @@ in
      '';
    };

+    source = lib.mkOption {
+      type = lib.types.str;


current type allows to point to a path outside of the nix store.
I don't think this was intended to allow out of store paths or any string here. Consider using lib.types.derivation, which checks if the path, or string, is a store path.

amozeo · 2025-01-12T17:10:45Z

nixos/modules/misc/nixpkgs.nix

@@ -157,6 +157,30 @@ in
      '';
    };

+    source = lib.mkOption {
+      type = lib.types.str;
+      defaultText = "<nixpkgs>";


it is not sufficiently described for/if flakes are used. I would remove defaultText.

Uhm, I'm actually not sure what happens if you use flakes. I guess it will be completely ignored.

amozeo · 2025-01-12T17:16:35Z

nixos/modules/misc/nixpkgs.nix

+      description = ''
+        If set, the path to the Nixpkgs source that will be used by the
+        `nixos-rebuild` tool to build the NixOS system, otherwise the default
+        is to use the `<nixpkgs>` channel.


it is not sufficiently described for/if flakes are used. Consider adding what the default is, when flakes are used.

Suggested change

is to use the `<nixpkgs>` channel.

is to use the nixpkgs NIX_PATH, or the flake input from where you're

using 'lib.nixosSystem'.

amozeo · 2025-01-12T17:29:20Z

nixos/modules/misc/nixpkgs.nix

+    # Link the Nixpkgs source to the system closure
+    system.extraSystemBuilderCmds = lib.mkIf (
+      config.nixpkgs ? source
+    ) "ln -s ${config.nixpkgs.source} $out/nixpkgs";
+
+    # Set nixpkgs in NIX_PATH if nix-channel is disabled
+    nix.nixPath = lib.optional (
+      config.nixpkgs ? source && !config.nix.channel.enable
+    ) "nixpkgs=/run/current-system/nixpkgs";


I don't think it is necessary to link nixpkgs to system derivation. You can directly use config.nixpkgs.source in config.nix.nixPath option definition.

What if I don't want to have nixpkgs in nixPath? there is no enable/disable option for that.

config.nixpkgs ? source is not how you check if option is set or not, use options.nixpkgs.source.isDefined

Suggested change

# Link the Nixpkgs source to the system closure

system.extraSystemBuilderCmds = lib.mkIf (

config.nixpkgs ? source

) "ln -s ${config.nixpkgs.source} $out/nixpkgs";

# Set nixpkgs in NIX_PATH if nix-channel is disabled

nix.nixPath = lib.optional (

config.nixpkgs ? source && !config.nix.channel.enable

) "nixpkgs=/run/current-system/nixpkgs";

# Set nixpkgs in NIX_PATH if nix-channel is disabled

nix.nixPath = lib.optional (

options.nixpkgs.source.isDefined && !config.nix.channel.enable

) "nixpkgs=${config.nixpkgs.source}";

amozeo · 2025-01-12T17:42:25Z

pkgs/os-specific/linux/nixos-rebuild/nixos-rebuild.sh

@@ -489,7 +489,7 @@ if [[ -z $_NIXOS_REBUILD_REEXEC && -n $canRun && -z $fast ]]; then
        p=$(runCmd nix-build --no-out-link $buildFile -A "${attr:+$attr.}config.system.build.nixos-rebuild" "${extraBuildFlags[@]}")
        SHOULD_REEXEC=1
    elif [[ -z $flake ]]; then
-        if p=$(runCmd nix-build --no-out-link --expr 'with import <nixpkgs/nixos> {}; config.system.build.nixos-rebuild' "${extraBuildFlags[@]}"); then
+        if p=$(runCmd nix-build --no-out-link --expr 'with import "$nixpkgsPath/nixos" {}; config.system.build.nixos-rebuild' "${extraBuildFlags[@]}"); then


$nixpkgsPath used here is set from an environment variable, because function that set that variable is ran after this line.

this is also true for $nixpkgsPath in line 579, 580, 582 (after changes).

amozeo · 2025-01-12T17:44:01Z

pkgs/os-specific/linux/nixos-rebuild/nixos-rebuild.sh

@@ -604,6 +621,8 @@ getVersion() {
 }


+getNixpkgsPath


this function should be ran at least before the first use of $nixpkgsPath. Consider moving this and the function itself at least before the first use in line 492

amozeo · 2025-01-12T17:49:24Z

pkgs/os-specific/linux/nixos-rebuild/nixos-rebuild.sh

+    # Get NixOS configuration
+    NIXOS_CONFIG=${NIXOS_CONFIG:-$(runCmd nix-instantiate --find-file nixos-config)}
+    if [[ -d $NIXOS_CONFIG ]]; then
+        NIXOS_CONFIG=$NIXOS_CONFIG/default.nix
+    fi


I don't think it is necessary to move that code from line 517 (before changes) here.
What is the purpose of moving it?

amozeo · 2025-01-12T19:52:51Z

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 · 2025-01-12T21:15:48Z

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 added 2 commits January 12, 2025 15:20

nixos/nixpkgs: add option to specify Nixpkgs source path

389ac0a

nixos-rebuild: use nixpkgs.source over <nixpkgs> if set

a54f8a9

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 bot requested a review from thiagokokada January 12, 2025 14:32

github-actions bot added 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Jan 12, 2025

rnhmjoj requested review from RossComputerGuy, infinisil and amozeo January 12, 2025 14:42

thiagokokada reviewed Jan 12, 2025

View reviewed changes

amozeo suggested changes Jan 12, 2025

View reviewed changes

rnhmjoj added 2 commits January 12, 2025 22:04

nixos-rebuild-ng: use nixpkgs.source over <nixpkgs> if set

4020524

doc/release-notes: mention nixpkgs.source

e2076cf

rnhmjoj force-pushed the pr-nixpkgs-source branch from bd5f609 to e2076cf Compare January 12, 2025 21:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/nixpkgs: add option to specify Nixpkgs source path #373201

nixos/nixpkgs: add option to specify Nixpkgs source path #373201

rnhmjoj commented Jan 12, 2025

thiagokokada Jan 12, 2025 •

edited

Loading

rnhmjoj Jan 12, 2025

thiagokokada left a comment

amozeo left a comment

amozeo Jan 12, 2025

amozeo Jan 12, 2025

rnhmjoj Jan 12, 2025

amozeo Jan 12, 2025

amozeo Jan 12, 2025

amozeo Jan 12, 2025

amozeo Jan 12, 2025

amozeo Jan 12, 2025

amozeo commented Jan 12, 2025 •

edited

Loading

rnhmjoj commented Jan 12, 2025

	is to use the `<nixpkgs>` channel.
	is to use the nixpkgs NIX_PATH, or the flake input from where you're
	using 'lib.nixosSystem'.

nixos/nixpkgs: add option to specify Nixpkgs source path #373201

Are you sure you want to change the base?

nixos/nixpkgs: add option to specify Nixpkgs source path #373201

Conversation

rnhmjoj commented Jan 12, 2025

Things done

thiagokokada Jan 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thiagokokada left a comment

Choose a reason for hiding this comment

amozeo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amozeo commented Jan 12, 2025 • edited Loading

rnhmjoj commented Jan 12, 2025

thiagokokada Jan 12, 2025 •

edited

Loading

amozeo commented Jan 12, 2025 •

edited

Loading