Skip to content

Commit

Permalink
release: forbid use of lib.fileset in Nixpkgs
Browse files Browse the repository at this point in the history
Due to Nix bug <NixOS/nix#11503>,
`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):

* <#340046>
* <#352491>
* <#369459>

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.

(cherry picked from commit 8725e46)
  • Loading branch information
emilazy committed Jan 1, 2025
1 parent 800b3b5 commit 4fc70a4
Show file tree
Hide file tree
Showing 9 changed files with 54 additions and 13 deletions.
23 changes: 22 additions & 1 deletion pkgs/top-level/default.nix
Original file line number Diff line number Diff line change
Expand Up @@ -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 <https://github.com/NixOS/nix/issues/11503> is fixed.
__allowFileset ? true

, # List of overlays layers used to extend Nixpkgs.
overlays ? []

Expand All @@ -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 <https://github.com/NixOS/nix/issues/11503>. 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;

Expand Down
5 changes: 4 additions & 1 deletion pkgs/top-level/release-cross.nix
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions pkgs/top-level/release-cuda.nix
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ in
"${variant}Support" = true;
inHydra = true;
};

__allowFileset = false;
},
...
}@args:
Expand Down
5 changes: 4 additions & 1 deletion pkgs/top-level/release-lib.nix
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions pkgs/top-level/release-outpaths.nix
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ let

inHydra = true;
};

__allowFileset = false;
};
};
recurseIntoAttrs = attrs: attrs // { recurseForDerivations = true; };
Expand Down
2 changes: 2 additions & 0 deletions pkgs/top-level/release-python.nix
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
allowUnfree = false;
inHydra = true;
};

__allowFileset = false;
},
}:

Expand Down
2 changes: 2 additions & 0 deletions pkgs/top-level/release-small.nix
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
allowUnfree = false;
inHydra = true;
};

__allowFileset = false;
},
}:

Expand Down
2 changes: 2 additions & 0 deletions pkgs/top-level/release-unfree-redistributable.nix
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@
cudaSupport = true;
inHydra = true;
};

__allowFileset = false;
},
# We only build the full package set on infrequently releasing channels.
full ? false,
Expand Down
24 changes: 14 additions & 10 deletions pkgs/top-level/release.nix
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 4fc70a4

Please sign in to comment.