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/nixos-option: don't abort in case of evaluation errors #43147
Conversation
@@ -87,7 +92,6 @@ EOF | |||
' <<EOF | |||
$result | |||
EOF | |||
return 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, that shouldn't be part of the patch :)
This ensures that after evalNix
the script doesn't abort. If e.g. the evaluation of the current value of an option fails the script shouldn't stop, but should print additional properties of the option instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the error still be propagated somehow? E.g. with an $error variable, which, when set, will return 1 at the very end, and intermediate failures will set it.
When running e.g. `nixos-option boot.kernelPackages` I get an output like this on the current unstable channel (18.09pre144959.be1461fc0ab): ``` $ nixos-option boot.kernelPackages Value: *exit 1* ``` This is fairly counter-intuitive as I have no clue what might went wrong. `strace` delivers an output like this: ``` read(3, "error: Package \342\200\230cryptodev-linu"..., 128) = 128 read(3, "ux/cryptodev/default.nix:22 is m"..., 128) = 128 read(3, "lowBroken = true; }\nin configura"..., 128) = 128 read(3, "you can add\n { allowBroken = tr"..., 128) = 128 read(3, "n)\n", 128) = 3 read(3, "", 128) = 0 ``` `nixos-option` evaluates the system config using `nix-instantiate` which might break when the evaluation fails (e.g. due to broken or unfree packages that are prohibited to evaluate by default). The script aborts due to the shebang `@shell@ -e`. In order to ensure that no unexpected behavior occurs due to removing `-e` from the interpreter the easiest way to work around this was to wrap `nix-instantiate` in `evalNix()` with a `set +e`. The function checks the success of the evaluation with `$?` in the end. Additionally `evalNix` shouldn't break, if one evaluation (e.g. the values that contain a package set by default) to return additional information like a description. With the change `nixos-option boot.kernelPackages` delivers the following output for me: ``` Value: error: Package ‘cryptodev-linux-1.9-4.14.52’ in /nix/store/47z2s8cwppymmgzw6n7pbcashikyk5jk-nixos/nixos/pkgs/os-specific/linux/cryptodev/default.nix:22 is marked as broken, refusing to evaluate. Default: { __unfix__ = <LAMBDA>; acpi_call = <CODE>; amdgpu-pro = <CODE>; ati_drivers_x11 = <CODE>; batman_adv = <CODE>; bbswitch = <CODE>; bcc = <CODE>; beegfs-module = <CODE>; blcr = <CODE>; broadcom_sta = <CODE>; callPackage = <CODE>; cpupower = <CODE>; cryptodev = <CODE>; dpdk = <CODE>; e1000e = <CODE>; ena = <CODE>; evdi = <CODE>; exfat-nofuse = <CODE>; extend = <CODE>; facetimehd = <CODE>; fusionio-vsl = <CODE>; hyperv-daemons = <CODE>; ixgbevf = <CODE>; jool = <CODE>; kernel = <CODE>; lttng-modules = <CODE>; mba6x_bl = <CODE>; mwprocapture = <CODE>; mxu11x0 = <CODE>; ndiswrapper = <CODE>; netatop = <CODE>; nvidiaPackages = <CODE>; nvidia_x11 = <CODE>; nvidia_x11_beta = <CODE>; nvidia_x11_legacy304 = <CODE>; nvidia_x11_legacy340 = <CODE>; nvidiabl = <CODE>; odp-dpdk = <CODE>; openafs = <CODE>; openafs_1_8 = <CODE>; perf = <CODE>; phc-intel = <CODE>; pktgen = <CODE>; ply = <CODE>; prl-tools = <CODE>; recurseForDerivations = true; rtl8192eu = <CODE>; rtl8723bs = <CODE>; rtl8812au = <CODE>; rtl8814au = <CODE>; rtlwifi_new = <CODE>; sch_cake = <CODE>; spl = <CODE>; splLegacyCrypto = <CODE>; splStable = <CODE>; splUnstable = <CODE>; stdenv = <CODE>; sysdig = <CODE>; systemtap = <CODE>; tbs = <CODE>; tmon = <CODE>; tp_smapi = <CODE>; usbip = <CODE>; v4l2loopback = <CODE>; v86d = <CODE>; vhba = <CODE>; virtualbox = <CODE>; virtualboxGuestAdditions = <CODE>; wireguard = <CODE>; x86_energy_perf_policy = <CODE>; zfs = <CODE>; zfsLegacyCrypto = <CODE>; zfsStable = <CODE>; zfsUnstable = <CODE>; } Example: { _type = "literalExample"; text = "pkgs.linuxPackages_2_6_25"; } Description: "This option allows you to override the Linux kernel used by\nNixOS. Since things like external kernel module packages are\ntied to the kernel you're using, it also overrides those.\nThis option is a function that takes Nixpkgs as an argument\n(as a convenience), and returns an attribute set containing at\nthe very least an attribute <varname>kernel</varname>.\nAdditional attributes may be needed depending on your\nconfiguration. For instance, if you use the NVIDIA X driver,\nthen it also needs to contain an attribute\n<varname>nvidia_x11</varname>.\n" Declared by: "/nix/var/nix/profiles/per-user/root/channels/nixos/nixpkgs/nixos/modules/system/boot/kernel.nix" Defined by: "/home/ma27/Projects/nixos-config/system/boot.nix" ```
4a01139
to
6f72b63
Compare
@infinisil with my patch the evaluation is resumed, if one value fails to evaluate, the other's can still be returned. You're absolutely right though, when one evaluation breaks, I shouldn't get a zero exit code. Now Is this ok for you now? :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, I sure do like to nitpick though :)
@nbp would you mind having a look at this? :) |
Motivation for this change
When running e.g.
nixos-option boot.kernelPackages
I get an outputlike this on the current unstable channel (18.09pre144959.be1461fc0ab):
This is fairly counter-intuitive as I have no clue what might went
wrong.
strace
delivers an output like this:nixos-option
evaluates the system config usingnix-instantiate
whichmight break when the evaluation fails (e.g. due to broken or unfree
packages that are prohibited to evaluate by default). The script aborts
due to the shebang
@shell@ -e
.In order to ensure that no unexpected
behavior occurs due to removing
-e
from the interpreter the easiestway to work around this was to wrap
nix-instantiate
inevalNix()
with a
set +e
. The function checks the success of the evaluation with$?
in the end. AdditionallyevalNix
shouldn't break, if oneevaluation (e.g. the values that contain a package set by default) to
return additional information like a description.
With the change
nixos-option boot.kernelPackages
delivers thefollowing output for me:
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)nix path-info -S
before and after)