make-derivation.nix: lib.warn if drv.__spliced missing #263082

ghost · 2023-10-24T07:06:05Z

This should wait until after 23.11, but I'd like to try to merge it fairly promptly thereafter.

Right now when an unspliced package appears as a dependency, we just silently do the wrong thing, cross our fingers, and hope for the best. This is the reason why troubleshooting cross compilation is such a headache.

This PR adds a lib.warn for this situation when cross compiling. So we still do the wrong thing, but at least we tell the user that something is probably wrong. The warning will also cause CI to fail. I fixed about a dozen of these failures. The list-of-shame provides an escape hatch for urgent situations.

The list-of-shame currently has (about) two dozen of these splicing failures; these are ignored in order to get CI to pass. The idea is to get a check like this in place to prevent even more of these bugs from occurring while we work to eliminate the ones that are already there. Since lib.warn causes CI failures this will prevent people from introducing any more of these bugs (or else force them to add their stinky deeds to the list of shame).

CI passes
No mass rebuild (14 packages rebuilt, due to fixing splice failures)

ghost · 2023-10-24T07:08:28Z

Ping @Artturin @alyssais

Proof that we are not splicing Rust hooks: check out this PR, then run:

nix-instantiate . -A pkgsCross.aarch64-multiplatform.fd

in the output you'll see:

trace: warning: derivation fd:
                     unspliced dependency /nix/store/99jm62fd7xh1yzjys0z3r70d1vvra51r-cargo-build-hook.sh
                       build=x86_64-unknown-linux-gnu
                       target=aarch64-unknown-linux-gnu

trace: warning: derivation fd:
                     unspliced dependency /nix/store/84aap5fr5s6yblga5w6fbx71j21l591a-cargo-check-hook.sh
                       build=x86_64-unknown-linux-gnu
                       target=aarch64-unknown-linux-gnu

trace: warning: derivation fd:
                     unspliced dependency /nix/store/0796vgahxgg8n7w1gr2pdb9sd3kibk13-cargo-install-hook.sh
                       build=x86_64-unknown-linux-gnu
                       target=aarch64-unknown-linux-gnu

trace: warning: derivation fd:
                     unspliced dependency /nix/store/qppx46087vy5xx8j3jajbzbqsjhvcp2b-cargo-setup-hook.sh
                       build=x86_64-unknown-linux-gnu
                       target=aarch64-unknown-linux-gnu

Artturin · 2023-10-24T07:12:46Z

Is similar to #204387

ghost · 2023-10-24T07:21:51Z

Is similar to #204387

Well, I think they're sort of orthogonal (and both are a good idea). It looks like checkExecutableOnBuild always succeeds when the __spliced attribute is missing even if it should be there -- whereas the point of this PR is to emit a warning in exactly that situation. Currently it takes way too long to figure out if a problem is caused by splicing not happening.

You should merge yours first since mine isn't done and is easier to rebase.

Artturin · 2023-10-24T07:40:36Z

Befofe merge there should be a way to fix the warning.

We should find out a way to splice or work around the lack of splicing when a function that returns derivation(s) is used (without adding it to be reachable from top-level).

Maybe a callPackage like func which internally calls with the various sets callPackage's, but a issue would still be what the user passes if it was not already spliced (eg overrideAttrs was used).

always succeeds when the __spliced attribute is missing even if it should be there

Explicitly used sets like buildPackages won't have a __spliced in packages

ghost · 2023-10-24T23:37:09Z

We should find out a way to splice or work around the lack of splicing when a function that returns derivation(s) is used (without adding it to be reachable from top-level).

The way to do that is to have all derivations be spliced, always, all the time. Then we don't have to worry about it.

In other words, do the splicing in mkDerivation.

pkgs/development/compilers/gcc/common/dependencies.nix

ghost · 2023-10-25T05:23:36Z

Explicitly used sets like buildPackages won't have a __spliced in packages

I think that's the point. Those should never be used in depsFooBar.

They should only be used when you need to interpolate into a string, like "${buildPlatform.lndir} ..."

pkgs/top-level/all-packages.nix

Artturin · 2023-10-25T05:35:46Z

pkgs/stdenv/generic/make-derivation.nix

+          # shame on you, llvm!
+          "llvm"
+          "compiler-rt"
+          "libcxx"
+          "libcxxabi"


the llvm sets use the normal makeScope and that's why there's no __spliced's in the sets own callPackage

I'm little by little working on making the sets better #253671
once I dedupe more stuff it'll be much easier to dedupe and improve the llvm/*/default.nix'es

Nice list name.

pkgs/stdenv/generic/make-derivation.nix

SuperSandro2000 · 2023-10-25T22:24:06Z

pkgs/development/libraries/cracklib/default.nix

@@ -15,7 +16,7 @@ stdenv.mkDerivation rec {
    hash = "sha256-yosEmjwtOyIloejRXWE3mOvHSOOVA4jtomlN5Qe6YCA=";
  };

-  nativeBuildInputs = lib.optional (stdenv.hostPlatform != stdenv.buildPlatform) buildPackages.cracklib;
+  nativeBuildInputs = lib.optional (stdenv.hostPlatform != stdenv.buildPlatform) cracklib;


I am trying to get more fit in cross: Shouldn't this use canExecute?

If this is what you mean, then yes, that would be an improvement (let's put it in a separate PR though -- one thing at a time).

diff --git a/pkgs/development/libraries/cracklib/default.nix b/pkgs/development/libraries/cracklib/default.nix index ba5d96a95182..defdb2965173 100644 --- a/pkgs/development/libraries/cracklib/default.nix +++ b/pkgs/development/libraries/cracklib/default.nix @@ -15,10 +15,10 @@ stdenv.mkDerivation rec { hash = "sha256-yosEmjwtOyIloejRXWE3mOvHSOOVA4jtomlN5Qe6YCA="; }; - nativeBuildInputs = lib.optional (stdenv.hostPlatform != stdenv.buildPlatform) buildPackages.cracklib; + nativeBuildInputs = lib.optional (!stdenv.buildPlatform.canExecute stdenv.hostPlatform) buildPackages.cracklib; buildInputs = [ zlib gettext ]; - postPatch = lib.optionalString (stdenv.hostPlatform == stdenv.buildPlatform) '' + postPatch = lib.optionalString (stdenv.buildPlatform.canExecute stdenv.hostPlatform) '' chmod +x util/cracklib-format patchShebangs util

This goes against your comment below about using buildPackages in nativeBuildInputs.

This goes against your comment below about using buildPackages in nativeBuildInputs.

Indeed! Nice catch. If this PR were merged CI would have caught that.

The patch above should, in addition to the changes it makes, also delete the buildPackages. before cracklib.

Fixes warnings with NixOS#263082 applied Update pkgs/development/libraries/ncurses/default.nix Co-authored-by: Artturi <Artturin@artturin.com> ncurses: Explicitly use buildPackages for --with-build-cc stdenv.cc worked for me because my nixos was configured to use boot.binfmt.emulatedSystems.

In order to avoid setting off the unspliced-dependency check, we need to make sure that the targetPackages.stdenv.cc.bintools used by gcc gets spliced. In order for it to be spliced, it needs a top-level entry in all-packages.nix. Therefore, this commit adds one. This also causes us to stop abusing targetPackages to access the linker, which is a good thing.

This commit adds a `lib.warn` check to stdenv.mkDerivation, causing it to emit a warning if a dependency which ought to be spliced in fact is not.

Co-authored-by: Artturi <Artturin@artturin.com>

…ldInputs

…s in buildInputs" This reverts commit 0899b6b.

ghost · 2023-11-17T11:07:22Z

Fixed merge conflict.

kjeremy · 2023-11-28T14:27:03Z

@amjoseph-nixpkgs can we merge this soon?

kjeremy · 2023-12-07T19:12:03Z

@amjoseph-nixpkgs What do you think about merging this so that it forces the issue of fixing packages for cross? I would personally like it so that I can hunt down the cross issues in all of my out of tree packages.

kjeremy · 2024-01-08T13:50:26Z

@amjoseph-nixpkgs 23.11 branched off a while ago. Can we merge this?

kjeremy · 2024-01-23T12:51:22Z

@amjoseph-nixpkgs why close this? I found this useful when trying to figure out why some of my cross pkgs were broken.

katexochen · 2024-01-23T19:53:53Z

@kjeremy I think this PR was closed in in the light of #50105 (comment). While not necessary, Adam chose to closed some of their open PRs (which is of cause okay if they like to do so).

github-actions bot added the 6.topic: stdenv Standard environment label Oct 24, 2023

This was referenced Oct 24, 2023

rustPlatform.cargoSetupHook: fix platform check #260068

Merged

makeRustPlatform prevents cargo*Hook from being spliced #263019

Open

ghost changed the title ~~[WIP] make-derivation.nix: lib.warn if drv.__splice is missing when expected~~ [WIP] make-derivation.nix: lib.warn if drv.__splice is missing Oct 24, 2023

ghost changed the title ~~[WIP] make-derivation.nix: lib.warn if drv.__splice is missing~~ [WIP] make-derivation.nix: lib.warn if drv.__splice missing Oct 24, 2023

Artturin reviewed Oct 25, 2023

View reviewed changes

pkgs/development/compilers/gcc/common/dependencies.nix Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

Artturin reviewed Oct 25, 2023

View reviewed changes

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved

Artturin reviewed Oct 25, 2023

View reviewed changes

This was referenced Oct 25, 2023

top-level/splice.nix: add NIXPKGS_ALWAYS_SPLICE debugging aid #263277

Closed

stdenv: inline and remove actuallySplice (no effect on eval) #263278

Closed

Artturin reviewed Oct 25, 2023

View reviewed changes

pkgs/stdenv/generic/make-derivation.nix Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

ghost changed the title ~~[WIP] make-derivation.nix: lib.warn if drv.__splice missing~~ make-derivation.nix: lib.warn if drv.__splice missing Oct 25, 2023

ghost mentioned this pull request Oct 25, 2023

setuptools-rust-hook: Use correct python for cross #262123

Closed

12 tasks

This comment was marked as outdated.

Sign in to view

SuperSandro2000 reviewed Oct 25, 2023

View reviewed changes

ghost changed the title ~~make-derivation.nix: lib.warn if drv.__splice missing~~ make-derivation.nix: lib.warn if drv.__spliced missing Oct 26, 2023

ofborg bot added 8.has: clean-up 8.has: package (new) labels Oct 26, 2023

ofborg bot requested review from winterqt, vcunat, Mic92 and Ericson2314 October 26, 2023 07:57

kjeremy mentioned this pull request Nov 2, 2023

ncurses: fix splicing #265060

Closed

13 tasks

This was referenced Nov 3, 2023

bash: avoid using unspliced dependency #265162

Closed

ncurses: do not use unspliced dependency #265759

Closed

mesa: fix cross compilation to x86_64 #233628

Merged

kjeremy mentioned this pull request Nov 6, 2023

ncurses: fix splicing #265891

Merged

13 tasks

ghost mentioned this pull request Nov 11, 2023

workflows/check-by-name: print failed command output #261693

Merged

12 tasks

Adam Joseph and others added 11 commits November 17, 2023 03:01

stdenv: lib.warn if drv.__splice is missing when expected

61f0912

This commit adds a `lib.warn` check to stdenv.mkDerivation, causing it to emit a warning if a dependency which ought to be spliced in fact is not.

stdenv: allow unspliced hostTarget dependencies (for now)

23bbc79

stdenv: add list-of-shame

6cae19e

Co-authored-by: Artturi <Artturin@artturin.com>

gcc: use stdenv.cc.bintools instead of targetPackages_bintools in bui…

849df93

…ldInputs

isl: fix splicing failure

a42fd7a

rustc-wasm32: give it a custom pname for stdenv list-of-shame

1b1cd5d

cracklib: do not use buildPackages for self-reference (fails to splice)

6a5f5a9

stdenv: alphabetize list-of-shame

e178e10

stdenv: improve diagnostics for unspliced-dependency check

8891c0a

Revert "gcc: use stdenv.cc.bintools instead of targetPackages_bintool…

ec58a9e

…s in buildInputs" This reverts commit 0899b6b.

Artturin mentioned this pull request Nov 25, 2023

GCC stdenv broken when cross-compiling to same system and depsBuildBuild contains buildPackages.stdenv.cc #265121

Open

ghost closed this Jan 23, 2024

ghost deleted the pr/stdenv/warn-missing-splices branch January 23, 2024 06:50

kjeremy mentioned this pull request Jan 23, 2024

Reopen #263082 "make-derivation.nix: lib.warn if drv.__spliced missing" #283295

Open

13 tasks

Artturin mentioned this pull request Jan 29, 2024

List of @amjoseph-nixpkgs Pull requests before account removal #284796

Open

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make-derivation.nix: lib.warn if drv.__spliced missing #263082

make-derivation.nix: lib.warn if drv.__spliced missing #263082

ghost commented Oct 24, 2023 •

edited by ghost

Loading

ghost commented Oct 24, 2023 •

edited by ghost

Loading

Artturin commented Oct 24, 2023

ghost commented Oct 24, 2023

Artturin commented Oct 24, 2023

ghost commented Oct 24, 2023 •

edited by ghost

Loading

ghost commented Oct 25, 2023

This comment was marked as resolved.

Artturin Oct 25, 2023 •

edited

Loading

This comment was marked as outdated.

This comment was marked as outdated.

SuperSandro2000 Oct 25, 2023

ghost Oct 26, 2023

kjeremy Oct 26, 2023

ghost Nov 2, 2023

ghost commented Nov 17, 2023

kjeremy commented Nov 28, 2023

kjeremy commented Dec 7, 2023

kjeremy commented Jan 8, 2024

kjeremy commented Jan 23, 2024

katexochen commented Jan 23, 2024 •

edited

Loading

make-derivation.nix: lib.warn if drv.__spliced missing #263082

make-derivation.nix: lib.warn if drv.__spliced missing #263082

Conversation

ghost commented Oct 24, 2023 • edited by ghost Loading

ghost commented Oct 24, 2023 • edited by ghost Loading

Artturin commented Oct 24, 2023

ghost commented Oct 24, 2023

Artturin commented Oct 24, 2023

ghost commented Oct 24, 2023 • edited by ghost Loading

ghost commented Oct 25, 2023

This comment was marked as resolved.

Artturin Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

SuperSandro2000 Oct 25, 2023

Choose a reason for hiding this comment

ghost Oct 26, 2023

Choose a reason for hiding this comment

kjeremy Oct 26, 2023

Choose a reason for hiding this comment

ghost Nov 2, 2023

Choose a reason for hiding this comment

ghost commented Nov 17, 2023

kjeremy commented Nov 28, 2023

kjeremy commented Dec 7, 2023

kjeremy commented Jan 8, 2024

kjeremy commented Jan 23, 2024

katexochen commented Jan 23, 2024 • edited Loading

ghost commented Oct 24, 2023 •

edited by ghost

Loading

ghost commented Oct 24, 2023 •

edited by ghost

Loading

ghost commented Oct 24, 2023 •

edited by ghost

Loading

Artturin Oct 25, 2023 •

edited

Loading

katexochen commented Jan 23, 2024 •

edited

Loading