auto-patchelf: avoid filesets #352491
Merged
auto-patchelf: avoid filesets #352491
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The use of filesets triggers the same Nix evaluation bug as in e4b8a0a, which broke the installer tests again.
|
Thanks to @lilyinstarlight for putting in way too much effort to track this down. @ofborg test installer.simpleUefiSystemdBoot |
|
I'm 95% sure this cannot be worked around on the fileset library level without compromising correctness :( |
|
If we can’t work around it in Nixpkgs then I think we really do have to just ban the use of filesets in‐tree for now, preferably enforced by CI. Any package that uses filesets and ends up being in the closure of the systems set up by the installer image wreaks havoc and it’s painful to track down the failures. Eval breaking under chroot stores is a pretty big correctness problem too, after all 😅 |
|
Might this be caused by NixOS/nix#11503 ? |
|
That indeed looks eminently plausible. |
13 tasks
emilazy
added a commit
to emilazy/nixpkgs
that referenced
this pull request
Dec 30, 2024
* <NixOS#340046> * <NixOS#352491> * <NixOS#369459>
emilazy
added a commit
to emilazy/nixpkgs
that referenced
this pull request
Dec 30, 2024
* <NixOS#340046> * <NixOS#352491> * <NixOS#369459>
emilazy
added a commit
to emilazy/nixpkgs
that referenced
this pull request
Dec 30, 2024
* <NixOS#340046> * <NixOS#352491> * <NixOS#369459>
emilazy
added a commit
to emilazy/nixpkgs
that referenced
this pull request
Dec 30, 2024
* <NixOS#340046> * <NixOS#352491> * <NixOS#369459>
emilazy
added a commit
to emilazy/nixpkgs
that referenced
this pull request
Dec 30, 2024
* <NixOS#340046> * <NixOS#352491> * <NixOS#369459>
emilazy
added a commit
to emilazy/nixpkgs
that referenced
this pull request
Dec 30, 2024
* <NixOS#340046> * <NixOS#352491> * <NixOS#369459>
emilazy
added a commit
to emilazy/nixpkgs
that referenced
this pull request
Dec 30, 2024
* <NixOS#340046> * <NixOS#352491> * <NixOS#369459>
emilazy
added a commit
to emilazy/nixpkgs
that referenced
this pull request
Dec 30, 2024
* <NixOS#340046> * <NixOS#352491> * <NixOS#369459>
emilazy
added a commit
to emilazy/nixpkgs
that referenced
this pull request
Dec 30, 2024
* <NixOS#340046> * <NixOS#352491> * <NixOS#369459>
emilazy
added a commit
to emilazy/nixpkgs
that referenced
this pull request
Dec 30, 2024
* <NixOS#340046> * <NixOS#352491> * <NixOS#369459>
emilazy
added a commit
to emilazy/nixpkgs
that referenced
this pull request
Dec 30, 2024
* <NixOS#340046> * <NixOS#352491> * <NixOS#369459>
emilazy
added a commit
to emilazy/nixpkgs
that referenced
this pull request
Dec 31, 2024
* <NixOS#340046> * <NixOS#352491> * <NixOS#369459>
emilazy
added a commit
to emilazy/nixpkgs
that referenced
this pull request
Dec 31, 2024
* <NixOS#340046> * <NixOS#352491> * <NixOS#369459>
emilazy
added a commit
to emilazy/nixpkgs
that referenced
this pull request
Dec 31, 2024
* <hxxps://github.com/NixOS/pull/340046> * <hxxps://github.com/NixOS/pull/352491> * <hxxps://github.com/NixOS/pull/369459>
emilazy
added a commit
to emilazy/nixpkgs
that referenced
this pull request
Dec 31, 2024
* <hxxps://github.com/NixOS/pull/340046> * <hxxps://github.com/NixOS/pull/352491> * <hxxps://github.com/NixOS/pull/369459>
emilazy
added a commit
to emilazy/nixpkgs
that referenced
this pull request
Dec 31, 2024
* <hxxps://github.com/NixOS/pull/340046> * <hxxps://github.com/NixOS/pull/352491> * <hxxps://github.com/NixOS/pull/369459>
emilazy
added a commit
to emilazy/nixpkgs
that referenced
this pull request
Dec 31, 2024
* <hxxps://github.com/NixOS/pull/340046> * <hxxps://github.com/NixOS/pull/352491> * <hxxps://github.com/NixOS/pull/369459>
emilazy
added a commit
to emilazy/nixpkgs
that referenced
this pull request
Dec 31, 2024
* <hxxps://github.com/NixOS/pull/340046> * <hxxps://github.com/NixOS/pull/352491> * <hxxps://github.com/NixOS/pull/369459>
emilazy
added a commit
to emilazy/nixpkgs
that referenced
this pull request
Dec 31, 2024
* <hxxps://github.com/NixOS/pull/340046> * <hxxps://github.com/NixOS/pull/352491> * <hxxps://github.com/NixOS/pull/369459>
emilazy
added a commit
to emilazy/nixpkgs
that referenced
this pull request
Dec 31, 2024
* <hxxps://github.com/NixOS/pull/340046> * <hxxps://github.com/NixOS/pull/352491> * <hxxps://github.com/NixOS/pull/369459>
emilazy
added a commit
to emilazy/nixpkgs
that referenced
this pull request
Dec 31, 2024
* <hxxps://github.com/NixOS/pull/340046> * <hxxps://github.com/NixOS/pull/352491> * <hxxps://github.com/NixOS/pull/369459>
emilazy
added a commit
to emilazy/nixpkgs
that referenced
this pull request
Dec 31, 2024
* <hxxps://github.com/NixOS/pull/340046> * <hxxps://github.com/NixOS/pull/352491> * <hxxps://github.com/NixOS/pull/369459>
emilazy
added a commit
to emilazy/nixpkgs
that referenced
this pull request
Dec 31, 2024
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): * <htxps://github.com/NixOS/pull/340046> * <NixOS#352491> * <NixOS#369459> The options I see here are: 1. Forbid use of `lib.fileset` within Nixpkgs. 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.
emilazy
added a commit
to emilazy/nixpkgs
that referenced
this pull request
Dec 31, 2024
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): * <NixOS#340046> * <NixOS#352491> * <NixOS#369459> The options I see here are: 1. Forbid use of `lib.fileset` within Nixpkgs. 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.
emilazy
added a commit
to emilazy/nixpkgs
that referenced
this pull request
Dec 31, 2024
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): * <NixOS#340046> * <NixOS#352491> * <NixOS#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.
13 tasks
emilazy
added a commit
that referenced
this pull request
Jan 1, 2025
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)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The use of filesets triggers the same Nix evaluation bug as in e4b8a0a, which broke the installer tests again.
See this Matrix message for the reproducer last time this happened with
switch-to-configuration-ng. It’s some interaction betweenNIX_PATHand chroot stores.I genuinely think we need to ban use of filesets in Nixpkgs until the filesets implementation works around the bug or all affected Nix versions get fixes (we do claim compatibility down to 2.3, after all). It has been fiendishly difficult to track down this issue both times now and it’s blocked the
staging-nextmerge for a couple days.Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.