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: Disable nix-channel #242098

Merged
merged 5 commits into from
Jul 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 47 additions & 9 deletions nixos/modules/config/nix-channel.nix
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
{ config, lib, ... }:
let
inherit (lib)
mkDefault
mkIf
mkOption
stringAfter
Expand All @@ -21,13 +22,42 @@ in
{
options = {
nix = {
channel = {
enable = mkOption {
description = lib.mdDoc ''
Whether the `nix-channel` command and state files are made available on the machine.

The following files are initialized when enabled:
- `/nix/var/nix/profiles/per-user/root/channels`
- `/root/.nix-channels`
- `$HOME/.nix-defexpr/channels` (on login)

Disabling this option will not remove the state files from the system.
'';
type = types.bool;
default = true;
};
};

nixPath = mkOption {
type = types.listOf types.str;
default = [
"nixpkgs=/nix/var/nix/profiles/per-user/root/channels/nixos"
"nixos-config=/etc/nixos/configuration.nix"
"/nix/var/nix/profiles/per-user/root/channels"
];
default =
if cfg.channel.enable
then [
"nixpkgs=/nix/var/nix/profiles/per-user/root/channels/nixos"
"nixos-config=/etc/nixos/configuration.nix"
"/nix/var/nix/profiles/per-user/root/channels"
]
else [];
Comment on lines +44 to +51
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
default =
if cfg.channel.enable
then [
"nixpkgs=/nix/var/nix/profiles/per-user/root/channels/nixos"
"nixos-config=/etc/nixos/configuration.nix"
"/nix/var/nix/profiles/per-user/root/channels"
]
else [];
default = lib.optionals cfg.channel.enable [
"nixpkgs=/nix/var/nix/profiles/per-user/root/channels/nixos"
"nixos-config=/etc/nixos/configuration.nix"
"/nix/var/nix/profiles/per-user/root/channels"
];

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not exactly equal to the documented default. Let's keep them in sync, and see other comment.

Copy link
Member

Choose a reason for hiding this comment

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

What's the difference?

defaultText = ''
if nix.channel.enable
then [
"nixpkgs=/nix/var/nix/profiles/per-user/root/channels/nixos"
"nixos-config=/etc/nixos/configuration.nix"
"/nix/var/nix/profiles/per-user/root/channels"
]
else [];
Comment on lines +53 to +59
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
if nix.channel.enable
then [
"nixpkgs=/nix/var/nix/profiles/per-user/root/channels/nixos"
"nixos-config=/etc/nixos/configuration.nix"
"/nix/var/nix/profiles/per-user/root/channels"
]
else [];
lib.optionals cfg.channel.enable [
"nixpkgs=/nix/var/nix/profiles/per-user/root/channels/nixos"
"nixos-config=/etc/nixos/configuration.nix"
"/nix/var/nix/profiles/per-user/root/channels"
];

Copy link
Member Author

Choose a reason for hiding this comment

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

This is harder to understand for someone who doesn't contribute to nixpkgs or other expressions regularly. optionals is idiosyncratic to Nixpkgs lib, whereas everyone understand if then else.

Copy link
Member

Choose a reason for hiding this comment

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

we had multiple treewide replacement for this already, so I don't think we should add new candidates for this

Copy link
Member Author

Choose a reason for hiding this comment

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

This documentation should not require someone to know the optionals function.

'';
description = lib.mdDoc ''
The default Nix expression search path, used by the Nix
evaluator to look up paths enclosed in angle brackets
Expand All @@ -49,22 +79,30 @@ in
config = mkIf cfg.enable {

environment.extraInit =
''
mkIf cfg.channel.enable ''
if [ -e "$HOME/.nix-defexpr/channels" ]; then
export NIX_PATH="$HOME/.nix-defexpr/channels''${NIX_PATH:+:$NIX_PATH}"
fi
'';

environment.extraSetup = mkIf (!cfg.channel.enable) ''
rm $out/bin/nix-channel
'';

# NIX_PATH has a non-empty default according to Nix docs, so we don't unset
# it when empty.
environment.sessionVariables = {
NIX_PATH = cfg.nixPath;
};

system.activationScripts.nix-channel = stringAfter [ "etc" "users" ]
''
nix.settings.nix-path = mkIf (! cfg.channel.enable) (mkDefault "");
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 we want to set this to empty string here because that clears the default nix-path setting of nix which breaks many programs like nix-tree or comma which rely on it to find some nixpkgs which in my case points to a flake.

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 so. The point of this is one really doesn't want to use a populated NIX_PATH. If other programs can't cope with that, they should be fixed.

One can always set nix.settings.nix-path elsewhere than this module instead.

Copy link
Member

Choose a reason for hiding this comment

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

By default nix-path is left empty which means it default to NIX_PATH env. When we explicitly set it to empty, that is no longer the case and things completely break for many tools which makes this option rather useless.

 ➜ nix show-config | rg nix-path
nix-path = nixpkgs=/nix/store/x6ly3m0dylzsbrgz0sx937xxcyswr2yp-source nixos=/nix/store/x6ly3m0dylzsbrgz0sx937xxcyswr2yp-source nixos-config=/etc/nixos/configuration.nix

 ➜ (unset NIX_PATH; nix show-config | rg nix-path)
nix-path =

 ➜ (unset NIX_PATH; nix --option nix-path "" show-config | rg nix-path)
nix-path =

 ➜ nix --option nix-path "" show-config | rg nix-path
nix-path =

Copy link
Member

Choose a reason for hiding this comment

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

That seems like the behaviour that someone ripping out a formerly-mandatory system component to avoid non-flakes things would want, given that properly flakes-aware tools like nixos-rebuild(8) won't look at the Nix path when used correctly.

Copy link
Member

Choose a reason for hiding this comment

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

hmmm, yeah, that sounds about right. Maybe I misunderstood the feature a bit.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so. The point of this is one really doesn't want to use a populated NIX_PATH

Kind of debatable, IMO. Feels like you're assuming that disabling channels automatically implies that:

  • The person is using flakes
  • The person wants to use a PURE flake workflow everywhere.

The reality is that:

  • nixpkgs is kind of special
  • You can use --impure with flakes
  • Some people may want a more "it just works" sort of thing with certain types of commands/ and /or the ability to build their system against a flake and then have all other commands use the same version of nixpkgs that their system was built against, or maybe still allow the NIX_PATH variable to take effect.

Copy link
Contributor

Choose a reason for hiding this comment

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

That seems like the behaviour that someone ripping out a formerly-mandatory system component to avoid non-flakes things would want, given that properly flakes-aware tools like nixos-rebuild(8) won't look at the Nix path when used correctly.

This is a weird position that doesn't make sense.

You can only use NIX_PATH at all, if you pass the --impure flag to these commands. If you dont, you'll get:

error: cannot look up '<nixpkgs>' in pure evaluation mode (use '--impure' to override)

If you're explicitly opting IN by passing the --impure flag, this behavior makes absolutely no sense.

Really, the problem is not setting nix-path to the empty string, the problem is NixOS/nix#8890 and NixOS/nix#8890

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also encountered the problem where I don't use channels but put aliases in NIX_PATH for commands. This setting disables NIX_PATH envvar entirely, which is not what I want. I'm kinda thinking we should revert this line.


system.activationScripts.nix-channel = mkIf cfg.channel.enable
(stringAfter [ "etc" "users" ] ''
# Subscribe the root user to the NixOS channel by default.
if [ ! -e "/root/.nix-channels" ]; then
echo "${config.system.defaultChannel} nixos" > "/root/.nix-channels"
fi
'';
'');
};
}
109 changes: 104 additions & 5 deletions nixos/tests/installer.nix
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,20 @@ let

# The configuration to install.
makeConfig = { bootLoader, grubDevice, grubIdentifier, grubUseEfi
, extraConfig, forceGrubReinstallCount ? 0
, extraConfig, forceGrubReinstallCount ? 0, flake ? false
}:
pkgs.writeText "configuration.nix" ''
{ config, lib, pkgs, modulesPath, ... }:

{ imports =
[ ./hardware-configuration.nix
<nixpkgs/nixos/modules/testing/test-instrumentation.nix>
${if flake
then "" # Still included, but via installer/flake.nix
else "<nixpkgs/nixos/modules/testing/test-instrumentation.nix>"}
SuperSandro2000 marked this conversation as resolved.
Show resolved Hide resolved
];

networking.hostName = "thatworked";

documentation.enable = false;

# To ensure that we can rebuild the grub configuration on the nixos-rebuild
Expand Down Expand Up @@ -67,7 +71,7 @@ let
# partitions and filesystems.
testScriptFun = { bootLoader, createPartitions, grubDevice, grubUseEfi
, grubIdentifier, preBootCommands, postBootCommands, extraConfig
, testSpecialisationConfig
, testSpecialisationConfig, testFlakeSwitch
}:
let iface = "virtio";
isEfi = bootLoader == "systemd-boot" || (bootLoader == "grub" && grubUseEfi);
Expand All @@ -86,9 +90,14 @@ let

qemu_flags = {"qemuFlags": assemble_qemu_flags()}

import os

image_dir = machine.state_dir
disk_image = os.path.join(image_dir, "machine.qcow2")

hd_flags = {
"hdaInterface": "${iface}",
"hda": "vm-state-machine/machine.qcow2",
"hda": disk_image,
}
${optionalString isEfi ''
hd_flags.update(
Expand Down Expand Up @@ -232,6 +241,11 @@ let
machine = create_machine_named("boot-after-rebuild-switch")
${preBootCommands}
machine.wait_for_unit("network.target")

# Sanity check, is it the configuration.nix we generated?
hostname = machine.succeed("hostname").strip()
assert hostname == "thatworked"

${postBootCommands}
machine.shutdown()

Expand Down Expand Up @@ -270,6 +284,84 @@ let
with subtest("We should find a file named /etc/gitconfig"):
machine.succeed("test -e /etc/gitconfig")

${postBootCommands}
machine.shutdown()
''
+ optionalString testFlakeSwitch ''
${preBootCommands}
machine.start()

with subtest("Configure system with flake"):
# TODO: evaluate as user?
machine.succeed("""
mkdir /root/my-config
mv /etc/nixos/hardware-configuration.nix /root/my-config/
mv /etc/nixos/secret /root/my-config/
rm /etc/nixos/configuration.nix
""")
machine.copy_from_host_via_shell(
"${makeConfig {
inherit bootLoader grubDevice grubIdentifier grubUseEfi extraConfig;
forceGrubReinstallCount = 1;
flake = true;
}}",
"/root/my-config/configuration.nix",
)
machine.copy_from_host_via_shell(
"${./installer/flake.nix}",
"/root/my-config/flake.nix",
)
machine.succeed("""
# for some reason the image does not have `pkgs.path`, so
# we use readlink to find a Nixpkgs source.
pkgs=$(readlink -f /nix/var/nix/profiles/per-user/root/channels)/nixos
if ! [[ -e $pkgs/pkgs/top-level/default.nix ]]; then
echo 1>&2 "$pkgs does not seem to be a nixpkgs source. Please fix the test so that pkgs points to a nixpkgs source.";
exit 1;
fi
sed -e s^@nixpkgs@^$pkgs^ -i /root/my-config/flake.nix
""")

with subtest("Switch to flake based config"):
machine.succeed("nixos-rebuild switch --flake /root/my-config#xyz")

${postBootCommands}
machine.shutdown()

${preBootCommands}
machine.start()

machine.wait_for_unit("multi-user.target")

with subtest("nix-channel command is not available anymore"):
machine.succeed("! which nix-channel")

# Note that the channel profile is still present on disk, but configured
# not to be used.
with subtest("builtins.nixPath is now empty"):
machine.succeed("""
[[ "[ ]" == "$(nix-instantiate builtins.nixPath --eval --expr)" ]]
""")

with subtest("<nixpkgs> does not resolve"):
machine.succeed("""
! nix-instantiate '<nixpkgs>' --eval --expr
""")

with subtest("Evaluate flake config in fresh env without nix-channel"):
machine.succeed("nixos-rebuild switch --flake /root/my-config#xyz")

with subtest("Evaluate flake config in fresh env without channel profiles"):
machine.succeed("""
(
exec 1>&2
rm -v /root/.nix-channels
rm -vrf ~/.nix-defexpr
rm -vrf /nix/var/nix/profiles/per-user/root/channels*
)
""")
machine.succeed("nixos-rebuild switch --flake /root/my-config#xyz")

${postBootCommands}
machine.shutdown()
'';
Expand All @@ -282,6 +374,7 @@ let
, grubDevice ? "/dev/vda", grubIdentifier ? "uuid", grubUseEfi ? false
, enableOCR ? false, meta ? {}
, testSpecialisationConfig ? false
, testFlakeSwitch ? false
}:
makeTest {
inherit enableOCR;
Expand Down Expand Up @@ -387,7 +480,7 @@ let
testScript = testScriptFun {
inherit bootLoader createPartitions preBootCommands postBootCommands
grubDevice grubIdentifier grubUseEfi extraConfig
testSpecialisationConfig;
testSpecialisationConfig testFlakeSwitch;
};
};

Expand Down Expand Up @@ -439,6 +532,10 @@ let
'';
};

simple-test-config-flake = simple-test-config // {
testFlakeSwitch = true;
};

simple-uefi-grub-config = {
createPartitions = ''
machine.succeed(
Expand Down Expand Up @@ -493,6 +590,8 @@ in {
# one big filesystem partition.
simple = makeInstallerTest "simple" simple-test-config;

switchToFlake = makeInstallerTest "switch-to-flake" simple-test-config-flake;

# Test cloned configurations with the simple grub configuration
simpleSpecialised = makeInstallerTest "simpleSpecialised" (simple-test-config // specialisation-test-extraconfig);

Expand Down
20 changes: 20 additions & 0 deletions nixos/tests/installer/flake.nix
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# This file gets copied into the installation

{
# To keep things simple, we'll use an absolute path dependency here.
inputs.nixpkgs.url = "@nixpkgs@";

outputs = { nixpkgs, ... }: {

nixosConfigurations.xyz = nixpkgs.lib.nixosSystem {
modules = [
./configuration.nix
( nixpkgs + "/nixos/modules/testing/test-instrumentation.nix" )
{
# We don't need nix-channel anymore
nix.channel.enable = false;
}
];
};
};
}