From 80983bbe54cfb69f361e64b8b8cdb5ab79720ec7 Mon Sep 17 00:00:00 2001 From: aszlig Date: Wed, 3 Feb 2016 14:21:45 +0100 Subject: [PATCH 1/4] nixos/udev: Provide a better warning for FHS paths We were trying to find FHS references in all of the rules found in services.udev.packages. Unfortunately we're still fixing up paths in the same derivation where we are checking those references, so for example references to /sbin/modprobe were still printed to be needed to fixup even though they were already fixed at the time. So now we're printing a more helpful warning message which is also conditional (before the warning message was printed regardless of whether there are any rules that need fixup) and is based off the rules that were already fixed up. The new warning message not only contains the build-local rule files but also the original files from other store paths and the FHS path references that were still found. With 8ecd3a5e1db40d308d60b5ccfa40eabccb419be9 reverted, we now get this: /nix/store/...-udev-rules/63-md-raid-arrays.rules (originally from /nix/store/...-mdadm-3.3.4/lib/udev/rules.d/63-md-raid-arrays.rules) contains references to /usr/bin/readlink and /usr/bin/basename. Which is now more accurate to what is not yet fixed and where it's coming from. Signed-off-by: aszlig --- nixos/modules/services/hardware/udev.nix | 27 ++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/nixos/modules/services/hardware/udev.nix b/nixos/modules/services/hardware/udev.nix index 1821fe8c0b8a..dc87c9215e65 100644 --- a/nixos/modules/services/hardware/udev.nix +++ b/nixos/modules/services/hardware/udev.nix @@ -94,10 +94,29 @@ let done echo "OK" - echo "Consider fixing the following udev rules:" - for i in ${toString cfg.packages}; do - grep -l '\(RUN+\|IMPORT{program}\)="\(/usr\)\?/s\?bin' $i/*/udev/rules.d/* || true - done + filesToFixup="$(for i in "$out"/*; do + grep -l '\B\(/usr\)\?/s\?bin' "$i" || : + done)" + + if [ -n "$filesToFixup" ]; then + echo "Consider fixing the following udev rules:" + echo "$filesToFixup" | while read localFile; do + remoteFile="origin unknown" + for i in ${toString cfg.packages}; do + for j in "$i"/*/udev/rules.d/*; do + if [ -e "$out/$(basename "$j")" ]; then + remoteFile="originally from $j" + break 2 + fi + done + done + refs="$( + grep -o '\B\(/usr\)\?/s\?bin/[^ "]\+' "$localFile" \ + | sed -e ':r;N;''${s/\n/ and /;br};s/\n/, /g;br' + )" + echo "$localFile ($remoteFile) contains references to $refs." + done + fi ${optionalString config.networking.usePredictableInterfaceNames '' cp ${./80-net-setup-link.rules} $out/80-net-setup-link.rules From 9f2a61c59cc4e4ce278e6582cb4bdca9c2088755 Mon Sep 17 00:00:00 2001 From: aszlig Date: Wed, 3 Feb 2016 14:45:19 +0100 Subject: [PATCH 2/4] nixos/udev: Add an option to fail on FHS paths So far we were merely printing a warning if there are still references to (/usr)/s?bin, but we actually want to make sure that we fix those paths, especially on updates of packages that come with udev rules. This adds a new option allowImpurePaths, which when set to false will cause the "udev-rules" derivation to fail. I've set this to true by default, to not break existing systems too much and the intention is to set it to false for a few NixOS VM tests. Signed-off-by: aszlig --- nixos/modules/services/hardware/udev.nix | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/nixos/modules/services/hardware/udev.nix b/nixos/modules/services/hardware/udev.nix index dc87c9215e65..74ec335c7509 100644 --- a/nixos/modules/services/hardware/udev.nix +++ b/nixos/modules/services/hardware/udev.nix @@ -116,6 +116,7 @@ let )" echo "$localFile ($remoteFile) contains references to $refs." done + ${optionalString (!cfg.allowImpurePaths) "exit 1"} fi ${optionalString config.networking.usePredictableInterfaceNames '' @@ -231,6 +232,20 @@ in ''; }; + allowImpurePaths = mkOption { + default = true; + example = false; + type = types.bool; + description = '' + If this is disabled, the build will fail whenever one of the + udev rules contains a reference to + /usr/bin, /usr/sbin, + /bin or /sbin. + + By default only a warning is printed during build. + ''; + }; + }; hardware.firmware = mkOption { From 9c13fe6604358e5255457422acbe8e03734f1e44 Mon Sep 17 00:00:00 2001 From: aszlig Date: Wed, 3 Feb 2016 14:49:56 +0100 Subject: [PATCH 3/4] nixos/tests/installer: Fail on impure udev rules With 9f2a61c in place, let's actually use this in the installer tests to make sure we won't shovel FHS paths down the throad of unstable channel users. I've tested this by running all of the installer tests for x86_64-linux and they all succeeded. Signed-off-by: aszlig --- nixos/tests/installer.nix | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/nixos/tests/installer.nix b/nixos/tests/installer.nix index 84fdb027ed85..cbfdad8d76bb 100644 --- a/nixos/tests/installer.nix +++ b/nixos/tests/installer.nix @@ -30,6 +30,8 @@ let hardware.enableAllFirmware = lib.mkForce false; + services.udev.allowImpurePaths = false; + ${replaceChars ["\n"] ["\n "] extraConfig} } ''; @@ -185,6 +187,8 @@ let hardware.enableAllFirmware = mkForce false; + services.udev.allowImpurePaths = false; + # The test cannot access the network, so any packages we # need must be included in the VM. system.extraDependencies = From c10a17a3ebfe460e713ba6b766fbdabd8c8f716c Mon Sep 17 00:00:00 2001 From: aszlig Date: Wed, 3 Feb 2016 16:40:41 +0100 Subject: [PATCH 4/4] nixos/udev: Always fail if rules contain FHS paths Partially reverts the following commits: 9f2a61c59cc4e4ce278e6582cb4bdca9c2088755 9c13fe6604358e5255457422acbe8e03734f1e44 As @edolstra pointed out, it would make more sense to do this by default instead of having that allowImpurePaths option. This of course might break systems which add extra packages to udev, but on the upside it's hard to miss one of these paths now because it won't get buried in the ocean of build output lines. Signed-off-by: aszlig --- nixos/modules/services/hardware/udev.nix | 16 +--------------- nixos/tests/installer.nix | 4 ---- 2 files changed, 1 insertion(+), 19 deletions(-) diff --git a/nixos/modules/services/hardware/udev.nix b/nixos/modules/services/hardware/udev.nix index 74ec335c7509..74200eec4c04 100644 --- a/nixos/modules/services/hardware/udev.nix +++ b/nixos/modules/services/hardware/udev.nix @@ -116,7 +116,7 @@ let )" echo "$localFile ($remoteFile) contains references to $refs." done - ${optionalString (!cfg.allowImpurePaths) "exit 1"} + exit 1 fi ${optionalString config.networking.usePredictableInterfaceNames '' @@ -232,20 +232,6 @@ in ''; }; - allowImpurePaths = mkOption { - default = true; - example = false; - type = types.bool; - description = '' - If this is disabled, the build will fail whenever one of the - udev rules contains a reference to - /usr/bin, /usr/sbin, - /bin or /sbin. - - By default only a warning is printed during build. - ''; - }; - }; hardware.firmware = mkOption { diff --git a/nixos/tests/installer.nix b/nixos/tests/installer.nix index cbfdad8d76bb..84fdb027ed85 100644 --- a/nixos/tests/installer.nix +++ b/nixos/tests/installer.nix @@ -30,8 +30,6 @@ let hardware.enableAllFirmware = lib.mkForce false; - services.udev.allowImpurePaths = false; - ${replaceChars ["\n"] ["\n "] extraConfig} } ''; @@ -187,8 +185,6 @@ let hardware.enableAllFirmware = mkForce false; - services.udev.allowImpurePaths = false; - # The test cannot access the network, so any packages we # need must be included in the VM. system.extraDependencies =