From 8725e466ef2bcc5be69106c16dbf69b9ef989273 Mon Sep 17 00:00:00 2001 From: Emily Date: Mon, 30 Dec 2024 15:36:05 +0000 Subject: [PATCH] release: forbid use of `lib.fileset` in Nixpkgs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Due to Nix bug , `builtins.filterSource` and chroot stores interact in a confusing and broken way that breaks `lib.fileset`. This means that uses of the API inside Nixpkgs keep breaking the NixOS installer, blocking the channel. The resulting error messages are inscrutable (they look like “the installer test is trying to download `curl`…?” and eventually bottom out in a derivation that has the wrong `outPath` because of the chroot store causing an incorrect `lib.fileset` result). Whenever this happens, someone (well, in practice K900 or I) has to bisect the change that introduced it and remove the use of `lib.fileset`. This has happened at least three times in the past four months (I believe I might actually be missing one here, but these are the ones I remember and could easily dig up): * * * The options I see here are: 1. Forbid use of `lib.fileset` within Nixpkgs until the Nix bug is fixed. This is the approach taken here. External users of Nixpkgs can continue to use the API as normal, but using it from within something that affects any release jobset `outPath`s will cause an evaluation failure with a hopefully‐helpful error message. 2. Forbid `lib.fileset` and also all of the other library APIs that use `builtins.filterSource`. I’m happy to do this, but so far none of those have broken the installer, so I decided to start small and worry about the others if they end up causing a problem in practice. 3. Forbid `builtins.filterSource` directly. This is hard and would require more invasive `builtins.scopedImport` crimes to do at evaluation time. I think this would realistically have to be done in something like nixpkgs-vet instead and I didn’t have much luck shoehorning a check like this into that codebase when I tried. 4. Fix the Nix bug. This would be great! But also it doesn’t seem to be happening any time soon, it seems difficult to fix in a way that doesn’t subtly break compatibility with the previous semantics, and arguably the fix would need backporting all the way back to 2.3 given our minimum version policy. 5. Do nothing; have people continue to innocuously use `lib.fileset` throughout Nixpkgs, breaking the installer whenever one of them sneaks in to that closure, causing the channel to be blocked and requiring expensive bisections to narrow down the inscrutable test failure to the package using `lib.fileset`, which then needs moving back off it. This sucks for the people who keep having to track it down, holds back important channel bumps, and the criteria for when it’s okay to use `lib.fileset` are not realistically possible to teach to all contributors. I'd be happy to work on (2) as an alternative; (3) would be difficult and seems like overkill, (4) is not really something I trust myself to do and wouldn’t address the immediate problem, and (5) isn’t sustainable. I think that the current approach here is the best trade‐off for now, as `lib.fileset` seems to be the only prominent user of the `builtins.filterSource` API that works with full store paths, exposing it to the Nix bug. It’s unfortunate to lose the nice API, but since we can’t rely on it to produce correct results and the channels keep getting blocked as a result, I don’t think we really have an alternative right now. --- pkgs/top-level/default.nix | 23 +++++++++++++++++- pkgs/top-level/release-cross.nix | 5 +++- pkgs/top-level/release-cuda.nix | 2 ++ pkgs/top-level/release-lib.nix | 5 +++- pkgs/top-level/release-outpaths.nix | 2 ++ pkgs/top-level/release-python.nix | 2 ++ pkgs/top-level/release-small.nix | 2 ++ .../release-unfree-redistributable.nix | 2 ++ pkgs/top-level/release.nix | 24 +++++++++++-------- 9 files changed, 54 insertions(+), 13 deletions(-) diff --git a/pkgs/top-level/default.nix b/pkgs/top-level/default.nix index 5c224802d5bf..fd443ac773ff 100644 --- a/pkgs/top-level/default.nix +++ b/pkgs/top-level/default.nix @@ -27,6 +27,10 @@ , # Allow a configuration attribute set to be passed in as an argument. config ? {} +, # Temporary hack to let Nixpkgs forbid internal use of `lib.fileset` + # until is fixed. + __allowFileset ? true + , # List of overlays layers used to extend Nixpkgs. overlays ? [] @@ -47,7 +51,24 @@ let # Rename the function arguments crossSystem0 = crossSystem; in let - lib = import ../../lib; + pristineLib = import ../../lib; + + lib = + if __allowFileset then + pristineLib + else + pristineLib.extend (_: _: { + fileset = abort '' + + The use of `lib.fileset` is currently forbidden in Nixpkgs due to the + upstream Nix bug . This + causes difficult‐to‐debug errors when combined with chroot stores, + such as in the NixOS installer. + + For packages that require source to be vendored inside Nixpkgs, + please use a subdirectory of the package instead. + ''; + }); inherit (lib) throwIfNot; diff --git a/pkgs/top-level/release-cross.nix b/pkgs/top-level/release-cross.nix index 1ce8f8f709e2..492a7503a03d 100644 --- a/pkgs/top-level/release-cross.nix +++ b/pkgs/top-level/release-cross.nix @@ -17,7 +17,10 @@ , # Strip most of attributes when evaluating to spare memory usage scrubJobs ? true , # Attributes passed to nixpkgs. Don't build packages marked as unfree. - nixpkgsArgs ? { config = { allowUnfree = false; inHydra = true; }; } + nixpkgsArgs ? { + config = { allowUnfree = false; inHydra = true; }; + __allowFileset = false; + } }: let diff --git a/pkgs/top-level/release-cuda.nix b/pkgs/top-level/release-cuda.nix index 945f0e6d2954..a19bc6f8ecaa 100644 --- a/pkgs/top-level/release-cuda.nix +++ b/pkgs/top-level/release-cuda.nix @@ -43,6 +43,8 @@ in "${variant}Support" = true; inHydra = true; }; + + __allowFileset = false; }, ... }@args: diff --git a/pkgs/top-level/release-lib.nix b/pkgs/top-level/release-lib.nix index c7581c47d341..a2821386ce3e 100644 --- a/pkgs/top-level/release-lib.nix +++ b/pkgs/top-level/release-lib.nix @@ -3,7 +3,10 @@ , packageSet ? (import ../..) , scrubJobs ? true , # Attributes passed to nixpkgs. Don't build packages marked as unfree. - nixpkgsArgs ? { config = { allowUnfree = false; inHydra = true; }; } + nixpkgsArgs ? { + config = { allowUnfree = false; inHydra = true; }; + __allowFileset = false; + } }: let diff --git a/pkgs/top-level/release-outpaths.nix b/pkgs/top-level/release-outpaths.nix index 0b70ea631244..e6caabec04e9 100644 --- a/pkgs/top-level/release-outpaths.nix +++ b/pkgs/top-level/release-outpaths.nix @@ -57,6 +57,8 @@ let inHydra = true; }; + + __allowFileset = false; }; }; recurseIntoAttrs = attrs: attrs // { recurseForDerivations = true; }; diff --git a/pkgs/top-level/release-python.nix b/pkgs/top-level/release-python.nix index a6222acaad53..161a6ddb68c3 100644 --- a/pkgs/top-level/release-python.nix +++ b/pkgs/top-level/release-python.nix @@ -15,6 +15,8 @@ allowUnfree = false; inHydra = true; }; + + __allowFileset = false; }, }: diff --git a/pkgs/top-level/release-small.nix b/pkgs/top-level/release-small.nix index 42cf6e79b732..511cef6a0d8a 100644 --- a/pkgs/top-level/release-small.nix +++ b/pkgs/top-level/release-small.nix @@ -19,6 +19,8 @@ allowUnfree = false; inHydra = true; }; + + __allowFileset = false; }, }: diff --git a/pkgs/top-level/release-unfree-redistributable.nix b/pkgs/top-level/release-unfree-redistributable.nix index 96384e9fb020..45940c4d02cc 100644 --- a/pkgs/top-level/release-unfree-redistributable.nix +++ b/pkgs/top-level/release-unfree-redistributable.nix @@ -37,6 +37,8 @@ cudaSupport = true; inHydra = true; }; + + __allowFileset = false; }, # We only build the full package set on infrequently releasing channels. full ? false, diff --git a/pkgs/top-level/release.nix b/pkgs/top-level/release.nix index 0140f0bd9d6a..78bdf1491125 100644 --- a/pkgs/top-level/release.nix +++ b/pkgs/top-level/release.nix @@ -28,16 +28,20 @@ # Strip most of attributes when evaluating to spare memory usage , scrubJobs ? true # Attributes passed to nixpkgs. Don't build packages marked as unfree. -, nixpkgsArgs ? { config = { - allowUnfree = false; - inHydra = true; - # Exceptional unsafe packages that we still build and distribute, - # so users choosing to allow don't have to rebuild them every time. - permittedInsecurePackages = [ - "olm-3.2.16" # see PR #347899 - "kanidm_1_3-1.3.3" - ]; - }; } +, nixpkgsArgs ? { + config = { + allowUnfree = false; + inHydra = true; + # Exceptional unsafe packages that we still build and distribute, + # so users choosing to allow don't have to rebuild them every time. + permittedInsecurePackages = [ + "olm-3.2.16" # see PR #347899 + "kanidm_1_3-1.3.3" + ]; + }; + + __allowFileset = false; + } # This flag, if set to true, will inhibit the use of `mapTestOn` # and `release-lib.packagePlatforms`. Generally, it causes the