Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discourage/prohibit inheriting from package sets in callPackage #204303

Open
Artturin opened this issue Dec 3, 2022 · 50 comments
Open

Discourage/prohibit inheriting from package sets in callPackage #204303

Artturin opened this issue Dec 3, 2022 · 50 comments
Labels
5. scope: tracking Long-lived issue tracking long-term fixes or multiple sub-problems 6.topic: cross-compilation Building packages on a different platform than they will be used on significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc.

Comments

@Artturin
Copy link
Member

Artturin commented Dec 3, 2022

in all-packages.nix when something is inherited (ex, {inherit (pkgs.darwin.apple_sdk.frameworks) Security;}) in a callPackage invocation the inherited attr will not have __spliced and so won't be cross-compilation compatible.

one way to fix this would be to change the with pkgs; in all-packages to with pkgs.__splicedPackages like tried in #58327
however that will result in other issues like infinite recursion.

$ nix build ".#pkgsCross.aarch64-multiplatform.bash"
error: infinite recursion encountered

other way to work around it would be to to inherit from the __splicedPackages set (ex, {inherit (pkgs.__splicedPackages.darwin.apple_sdk.frameworks) Security;})

the way i prefer would be to just not inherit from package sets

Demo: show-splices.nix

let
  pkgs = import ./. { };
  pkgsCross = pkgs.pkgsCross.aarch64-multiplatform;
  workingSplices = pkgsCross.callPackage
    ({ darwin, xorg }: {
      d = darwin.apple_sdk.frameworks.Security.__spliced;
      x = xorg.libX11.__spliced;
    })
    { };

  notWorkingSplices = pkgsCross.callPackage
    ({ Security, libX11 }: {
      d = Security.__spliced;
      x = libX11.__spliced;
    })
    {
      inherit (pkgsCross.darwin.apple_sdk.frameworks) Security;
      inherit (pkgsCross.xorg) libX11;
    };
in
{
  # for --json to work
  workingSplices =
    removeAttrs workingSplices [ "override" "overrideDerivation" ];
  notWorkingSplices =
    removeAttrs notWorkingSplices [ "override" "overrideDerivation" ];
}

working:

$ NIXPKGS_ALLOW_UNSUPPORTED_SYSTEM=1 nix eval -f show-splices.nix workingSplices --json | jq
{
  "d": {
    "buildBuild": "/nix/store/ls83bixygn2bzqq1w2z66dyccfn61wic-apple-framework-Security",
    "buildHost": "/nix/store/ls83bixygn2bzqq1w2z66dyccfn61wic-apple-framework-Security",
    "buildTarget": "/nix/store/ls83bixygn2bzqq1w2z66dyccfn61wic-apple-framework-Security",
    "hostHost": "/nix/store/vhqjns1zfab6cnv2lbkr6gff1fa8n3kw-apple-framework-Security-11.0.0",
    "hostTarget": "/nix/store/vhqjns1zfab6cnv2lbkr6gff1fa8n3kw-apple-framework-Security-11.0.0"
  },
  "x": {
    "buildBuild": "/nix/store/w3zzhfl4a7xp0xfflz2gawv02y8ba9z8-libX11-1.8.1",
    "buildHost": "/nix/store/w3zzhfl4a7xp0xfflz2gawv02y8ba9z8-libX11-1.8.1",
    "buildTarget": "/nix/store/w3zzhfl4a7xp0xfflz2gawv02y8ba9z8-libX11-1.8.1",
    "hostHost": "/nix/store/gjh7n6myi7miwprjkk4lll8cp1j7kmz8-libX11-aarch64-unknown-linux-gnu-1.8.1",
    "hostTarget": "/nix/store/gjh7n6myi7miwprjkk4lll8cp1j7kmz8-libX11-aarch64-unknown-linux-gnu-1.8.1"
  }
}

broken:

}
$ NIXPKGS_ALLOW_UNSUPPORTED_SYSTEM=1 nix eval -f show-splices.nix notWorkingSplices --json | jq
error: attribute '__spliced' missing

       at /home/artturin/nixgits/my-nixpkgs/show-splices.nix:5:74:

            4|   workingSplices = pkgsCross.callPackage ({ darwin, xorg }: { d = darwin.apple_sdk.frameworks.Security.__spliced; x = xorg.libX11.__spliced;}) { };
            5|   notWorkingSplices = pkgsCross.callPackage ({ Security, libX11 }: { d = Security.__spliced; x = libX11.__spliced;}) { inherit (pkgsCross.darwin.apple_sdk.frameworks) Security; inherit (pkgsCross.xorg) libX11; };
             |                                                                          ^
            6| in
{
  "d": null
}

related issues:
#49526

@Artturin Artturin added the 6.topic: cross-compilation Building packages on a different platform than they will be used on label Dec 3, 2022
@AndersonTorres
Copy link
Member

@Artturin , please, can you (or someone) explain what is the matter around the splicing?

I have seen today some code on XFCE mega-attrset, and I am confused.

Also, I have some packages that use Darwin libs, so I will start to cleanup them.

@Artturin
Copy link
Member Author

Artturin commented Dec 3, 2022

@Artturin , please, can you (or someone) explain what is the matter around the splicing?

splicing adds the __spliced attribute set to the package
mkDerivation then picks __spliced.buildHost for nativeBuildInputs

(map (drv: drv.__spliced.buildHost or drv) (checkDependencyList "nativeBuildInputs" nativeBuildInputs

and __spliced.hostTarget for buildInputs

(map (drv: drv.__spliced.buildHost or drv) (checkDependencyList "nativeBuildInputs" nativeBuildInputs

@sternenseemann
Copy link
Member

I think it is not possible to get rid of it entirely, especially if you consider the API benefit of being able to override individual inputs which is certainly an intention in some of the cases. Also, in sub package sets the solution you propose in #195874 won't work, of course.

@tjni
Copy link
Contributor

tjni commented Dec 3, 2022

There's some magic going on with __spliced that's not obvious to me. Given that, it sounds hard to teach to contributors, so how do we prohibit this pattern?

@Atemu
Copy link
Member

Atemu commented Dec 3, 2022

I concur that, ideally, we do not want to pass sets of packages to packages. Same reason we don't just pass pkgs everywhere and call it a day.

Why are the Darwin frameworks hidden away in a subset anyways? If there's no good reason for it, we might want to consider just adding them to the top-level.

Another thing we could consider is a special attribute where we can simply pass attribute sets from which attrs should be considered as if they were at the top-level but at that point, we might as well use the top-level in the first place?

@AndersonTorres
Copy link
Member

Why are the Darwin frameworks hidden away in a subset anyways?

I don't believe they are hidden, properly speaking. Anyone can access them by the cumbersome inherit mechanism.
The problem to me is the magic behind them and that huge amount of if isDarwin then [ Security OpenGL Metal ... ]

If there's no good reason for it, we might want to consider just adding them to the top-level.

This is a bit personal and preciousness-y, but I do not like the currently huge top-level file and its propensity to scramble the asciibetical ordering.

@Artturin
Copy link
Member Author

Artturin commented Dec 3, 2022

stuff like this should be discouraged/prohibited too

  keybinder = callPackage ../development/libraries/keybinder {
    automake = automake111x;
    lua = lua5_1;
  };
nix-repl> pkgsCross.aarch64-multiplatform.keybinder.nativeBuildInputs
[ ... «derivation /nix/store/yj8vj3gbd78scrj4xjmdx4rm5p87dyb9-automake-aarch64-unknown-linux-gnu-1.11.6.drv» ]

if it has to be done then it should be

  keybinder = callPackage ../development/libraries/keybinder {
    automake = __splicedPackages.automake111x;
    lua = __splicedPackages.lua5_1;
  };

nix-repl> pkgsCross.aarch64-multiplatform.keybinder.nativeBuildInputs
[ ... «derivation /nix/store/c9g2kiyrwv9hq33wa3ckrkz95y4dq1qh-automake-1.11.6.drv» ]

@figsoda
Copy link
Member

figsoda commented Dec 3, 2022

can we fix that by changing (all-packages.nix L13)

with pkgs;

to

with pkgs.__splicedPackages;

?

@Artturin
Copy link
Member Author

Artturin commented Dec 3, 2022

i linked #58327 in the OP

a thing we could try is reorganize all-packages.nix so we don't get infinite recursion

quick POC (added with pkgs.__splicedPackages under wrapBintoolsWith which was giving inf rec)

diff --git a/pkgs/top-level/all-packages.nix b/pkgs/top-level/all-packages.nix
index 5e0e798be20..765ce0b92c0 100644
--- a/pkgs/top-level/all-packages.nix
+++ b/pkgs/top-level/all-packages.nix
@@ -8,7 +8,7 @@
 { lib, noSysDirs, config, overlays }:
 res: pkgs: super:
 
-with pkgs;
+(with pkgs;
 
 {
   # A module system style type tag
@@ -15618,6 +15618,9 @@ with pkgs;
 
   yaml-language-server = nodePackages.yaml-language-server;
 
+}) // (with pkgs.__splicedPackages; {
+
+
   # prolog
   yap = callPackage ../development/compilers/yap { };
 
@@ -38170,4 +38173,4 @@ with pkgs;
   aitrack = libsForQt5.callPackage ../applications/misc/aitrack { };
 
   widevine-cdm = callPackage ../applications/networking/browsers/misc/widevine-cdm.nix { };
-}
+})

a issue with that is the use of // so we won't get errors about duplicate attrs and the RHS will override the LHS

@AndersonTorres
Copy link
Member

#204370

@Atemu
Copy link
Member

Atemu commented Dec 4, 2022

I don't believe they are hidden, properly speaking. Anyone can access them by the cumbersome inherit mechanism.

That's what I mean.

I do not like the currently huge top-level file and its propensity to scramble the asciibetical ordering.

Note that I said "top-level", not "top-level file". I don't care what file they're in but I do want them to be accessible from pkgs so that the auto-discovery mechanism, splicing and all the other quirks of callPackage's "just work" with them.

Alternatively, an extra attr that automagically extends the main package set for the purpose of discovering dependencies for that specific package would also be fine in my eyes. i.e.

  bar = callPackage ../foo/bar {
    extraPackageSets = [ darwin xorg ];
  }
{
  stdenv,
  libX11,
  Security,
  ...
}

...

stuff like this should be discouraged/prohibited too

  keybinder = callPackage ../development/libraries/keybinder {
    automake = automake111x;
    lua = lua5_1;
  };

In cases like this, I prefer to simply reference automake111x and lua5_1 explicitly in the inputs of the package's function. I.e.

{ 
  lib,
  stdenv,
  automake111x,
  lua5_1,
  ...
}:

let
  lua = lua5_1;
  automake = automake111x;
in

stdenv.mkDerivation {
  nativeBuildInputs = [ automake lua ];
}

(Or just in-line without the let.)

IMO, we should have as little logic as possible in all-packages.

@AndersonTorres
Copy link
Member

AndersonTorres commented Dec 4, 2022

It's becoming confusing...

It should be something more, ehr, ergonomical. Yes, I believe we should be more cross-compilaton-aware and we should strive to make it a "first class citizen" in Nix{,pkgs}.
However, should this be engulfed by magical syntax/semantics?

Or maybe there is a terminology we all should learn?

@Atemu
Copy link
Member

Atemu commented Dec 5, 2022

The most ergonomic will always be to not have package sets and have everything under the top-level as I initially suggested.

My alternative suggestion is just something that came to my mind because we might need something like that anyways because we do have huge package sets whose packages other packages depend on and, unlike the smaller ones we've been discussing so far, we probably don't want to dump those into the top-level.
If you can think of a more ergonomic solution to that problem, I'm all ears.

@AndersonTorres
Copy link
Member

AndersonTorres commented Dec 14, 2022

Taking the idea to a ridiculous extreme, all the expressions organized in subdirectories can be "concatenated" in a huge 4GB single file, sqlite-amalgam style.
Indeed, I have refactored some Elisp packages that were written directly onto manual-packages.nix some time ago.

Thinking a bit about it, this makes few to no sense. We have custom code to shelter platforms like FreeBSD and PowerPC, it would be insane to make all of it accessible to the top-level.

Edit: I found the word: uniformization!

@viraptor
Copy link
Contributor

viraptor commented Dec 28, 2022

Why are the Darwin frameworks hidden away in a subset anyways?

The frameworks come from different sdks, so sometimes we want the ones from apple_sdk, sometimes apple_sdk_11_0 (and there will be more).
(On a second read... did you ask why the sdks are under Darwin, not top level?)

@AndersonTorres
Copy link
Member

(On a second read... did you ask why the sdks are under Darwin, not top level?)

In a certain sense, yes.

@ghost
Copy link

ghost commented Apr 20, 2023

when something is inherited (ex, {inherit (pkgs.darwin.apple_sdk.frameworks) Security}) in a callPackage invocation

Note that RFC 140 will deprecate callPackage _ { /* non-empty */ } so this problem might solve itself.

@infinisil
Copy link
Member

@amjoseph-nixpkgs No it won't deprecate that, such invocations will still be valid! It's in scope to look at for the Nixpkgs Architecture Team, but until custom arguments have an adequate replacement of its use cases (which RFC 140 isn't), it shouldn't be deprecated

@AtnNn
Copy link
Contributor

AtnNn commented Jul 6, 2023

Some sort of deep pattern matching in the nix language would help with this

I've experimented with a similar approach that doesn't require changes to the Nix language. In master...AtnNn:nixpkgs:deepcall callPackage looks for parameters that contain ' and "replaces" it with .. So packages can take as arguments llvmPackages'bintools, pkgsBuildHost'nodejs, perlPackages'CGIFast, targetPackages'stdenv'cc, xorg'libX11, python3Packages'docutils, gst_all_1'gstreamer, unixODBCDrivers'mariadb, etc.

@fgaz
Copy link
Member

fgaz commented Jul 6, 2023

@AtnNn very nice, thanks for trying it out!

While yours is a clever solution given the current Nix language limitation, I think in the long run a feature like the one I proposed is necessary to keep everything clean. Nix is a domain specific language after all!

At the moment I'm busy, but I might draft an RFC later this year

@ghost
Copy link

ghost commented Jul 12, 2023

until custom arguments have an adequate replacement of its use cases

What can callPackage _ { /* non-empty */ } do that (callPackage _ { }).override cannot?

Missing arguments are the only thing I can think of, and the easy workaround is to add ? throw "missing".

@AndersonTorres
Copy link
Member

AndersonTorres commented May 29, 2024

Thanks for remembering me about this!
Now I need to rewrite something about getting rid of splicing.

@nixos-discourse
Copy link

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/is-this-the-current-state-of-things/45944/14

@physics-enthusiast
Copy link
Contributor

physics-enthusiast commented Jun 10, 2024

Random suggestion - how about implementing cross via an argument in mkDerivation? Have an offset parameter denoting the current derivation's position in the (build, ..., build, build, host, host, host, ..., host) chain which determines their internal platform triplet. Then dependent mkDerivations can call overrideAttrs on their nativeBuildInputs with an offset one less then their own, and on their depsTargetTarget with an offset one higher. Then, we no longer need to worry about callPackage injecting splices in all the places they're needed. As a bonus, external consumers of Nixpkgs expressions calling mkDerivation can participate in cross as dependencies without having to inject their own __spliced.buildHost and __spliced.hostTarget. I don't have full knowledge of the history of cross and why it is the way it is right now, so maybe there's some massive hidden downside I'm not aware of? More thunks, I guess?

@Atemu
Copy link
Member

Atemu commented Jun 10, 2024

Then dependent mkDerivations can call overrideAttrs on their nativeBuildInputs with an offset one less then their own

I'm not sure how you're imagining this to work with overrideAttrs?

@physics-enthusiast
Copy link
Contributor

physics-enthusiast commented Jun 10, 2024

I'm not sure how you're imagining this to work with overrideAttrs?

We'd add offset to derivationArg in make-derivation.nix, which I presume is how it becomes overrideable via overrideAttrs. To be a bit clearer, I'm proposing we add this lever to all mkDerivation calls everywhere; the universality is what makes it useful. Semantics would remain unchanged since we're still doing the exact same platform shuffling, only in a different place. Thinking about it a little bit more, we might also need a second offset to accomodate the more esoteric combinations like depsHostHost or depsBuildTarget.

Edit: Nevermind, I forgot about all of the variables defined outside the mkDerivation call. I think I see what you were getting at now.

@nixos-discourse
Copy link

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/frustrations-about-splicing/49607/1

@nixos-discourse
Copy link

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/frustrations-about-splicing/49607/7

@reckenrode
Copy link
Contributor

The way forward for Darwin is to get rid of frameworks and provide an SDK in the stdenv. It doesn’t help other package sets, but it should at least get rid of one of the major contributors to this problem.

@reckenrode
Copy link
Contributor

#346043 replaces the individual framework packages with top-level SDK packages for Darwin, which should help reduce some of the Darwin-caused pain here.

@tomodachi94 tomodachi94 added the 5. scope: tracking Long-lived issue tracking long-term fixes or multiple sub-problems label Oct 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5. scope: tracking Long-lived issue tracking long-term fixes or multiple sub-problems 6.topic: cross-compilation Building packages on a different platform than they will be used on significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc.
Projects
None yet
Development

No branches or pull requests