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

release-cross.nix: fix cross bootstrap tools eval #176901

Merged
merged 2 commits into from
Jun 8, 2022

Conversation

alyssais
Copy link
Member

@alyssais alyssais commented Jun 8, 2022

Description of changes

In #175484, I changed pkgs/stdenv/linux/make-bootstrap-tools.nix
to take a package set instead of system and localSystem arguments, but
I forgot to update make-bootstrap-tools-cross.nix.

Now that we have a package set in make-bootstrap-tools-cross.nix, we
can use pkgsCross to avoid full re-evaluations of Nixpkgs for each
platform.

I'm not sure why release-cross.nix hardcodes the build system to
x86_64-linux, so I've left that as-is.

Fixes: 5643714 ("stdenvBootstrapTools: inherit {cross,local}System")

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Jun 8, 2022
@alyssais alyssais added the 6.topic: cross-compilation Building packages on a different platform than they will be used on label Jun 8, 2022
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 0 This PR does not cause any packages to rebuild labels Jun 8, 2022
@Ericson2314
Copy link
Member

I would rather not use pkgsCross. I don't really like pkgsCross in general -- it is very gross it is part of the package set and not some out of band convenience.

If we want some sort of memoization maybe we can factor that out, e.g. into a new pkgs/top-level/memoized.nix?

@alyssais
Copy link
Member Author

alyssais commented Jun 8, 2022

it is very gross it is part of the package set and not some out of band convenience.

I strongly disagree. You need cross compiled derivations to be accessible inside the package set, because sometimes (not often, but sometimes) you have a package that depends on specifically the static version of another package, or a cross-compiled version.

Additionally, in all-packages.nix there are a bunch of packages using pkgsCross. nixpkgsFun being inaccessible except to privileged attributes inside stage.nix means that doing anything for multiple systems involves needless Nixpkgs re-evaluations, and pkgsCross and friends are the only mitigations.

@Ericson2314
Copy link
Member

There are a few exception, which is why getting rid of pkgsCross is tough, but in general enumerating the platforms to be supported vs being parameteric is not good and I get very nervous about platform enumeration happening for no good reason (like flakes).

To be clear you wouldn't do that, but I wouldn't want people to read the code and get the wrong impression.

Thinking and reading a bit more, In this case, we are making a regular package set for no other reason than to get pkgsCross. But release-lib.nix has it's own memoize Nixpkgs and utilities around that. Can we use that instead?

This has been deprecated for a long time, and it's doubtful it had any
users to start with.  And having an undisablable warning when
enumarating platforms is not good.
@alyssais
Copy link
Member Author

alyssais commented Jun 8, 2022

I've pushed an implementation using release-lib. The supportedSystems = [] hack I had to do is unfortunate — really that should just be an argument to the two functions in release-lib that use it…

In 5643714, I changed pkgs/stdenv/linux/make-bootstrap-tools.nix
to take a package set instead of system and localSystem arguments, but
I forgot to update make-bootstrap-tools-cross.nix.

Fixes: 5643714 ("stdenvBootstrapTools: inherit {cross,local}System")
@ofborg ofborg bot added 10.rebuild-linux: 1-10 and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild labels Jun 8, 2022
Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@Ericson2314 Ericson2314 merged commit 97bdf48 into NixOS:master Jun 8, 2022
@alyssais alyssais deleted the release-cross branch June 8, 2022 17:39
@ghost
Copy link

ghost commented Jun 27, 2022

I'm a bit late to the discussion here, but...

I don't really like pkgsCross in general -- it is very gross it is part of the package set and not some out of band convenience.

Does this dislike extend to pkgs.buildPackages?

I always thought of pkgs: pkgs.pkgsCross._ as the right inverses of pkgs: pkgs.buildPackages.

I get very nervous about platform enumeration happening for no good reason

Manually-coded enumeration bothers me too. Unfortunately because so much software needs Rosetta Stones we can't really avoid having a manual enumeration for each of the four non-vendor components of a multiarch tuple: cpu-and-mode, kernel, libc, abi. Would it still make you nervous if lib.getAttrs (pkgs.pkgsCross) were defined as the product of those lists (possibly filtered to remove nonsense combinations)?

Unfortunately nix doesn't have a high-precedence postfix function application operator. If people have to write (pkgsCross "awesome-platform").myPackage they may resort to torches and pitchforks... so it might be hard to avoid some kind of enumeration since the set of attributes in an attrset must be finite.

@sternenseemann
Copy link
Member

Does this dislike extend to pkgs.buildPackages?

buildPackages is different and it is necessary anyways to bootstrap a cross package set, so it's not like we could remove it.

I always thought of pkgs: pkgs.pkgsCross._ as the right inverses of pkgs: pkgs.buildPackages.

pkgs: pkgs.targetPackages is the correct right inverse (pkgs.buildPackages.targetPackages == pkgs). targetPackages and buildPackages actually traverse the package set chain, whereas pkgsCross just replaces the host (and target) platform of the current package set with a new one (so e.g. pkgsCross.${x}.pkgsCross.${x} == pkgsCross.${x}).

If people have to write (pkgsCross "awesome-platform").myPackage they may resort to torches and pitchforks...

The bigger problem is that this needs to be recomputed everytime it is used. Nix can share eval work for attributes, but doesn't memoize functions.

@ghost
Copy link

ghost commented Jun 28, 2022

pkgs: pkgs.targetPackages is the correct right inverse

I think that's the left inverse of pkgs: pkgs.buildPackages.

so e.g. pkgsCross.${x}.pkgsCross.${x} == pkgsCross.${x}

Oh wow, yes, that is gross. And unexpected.

Nix can share eval work for attributes, but doesn't memoize functions.

Hrm, I thought it did. I guess this changed? From edolstra's thesis, section 4.4:

maximal sharing: if two terms are syntactically equal, then they occupy the same location in memory. ... Maximal sharing is implemented through a hash table. Whenever a new term is created, the term is looked up in the hash table. If the term already exists, the address of the term obtained from the hash table is returned.

I think the LISP people call this "hash consing".

@sternenseemann
Copy link
Member

sternenseemann commented Jul 2, 2022

Hrm, I thought it did. I guess this changed? From edolstra's thesis, section 4.4:

This is only the case for variable bindings, attribute sets etc. Functions behave like this (utilising the fact that valueSize can be (ab)used to observe whether a thunk has been evaluated yet or not):

nix-repl> f = x: x

nix-repl> builtins.valueSize (f {})
270130

nix-repl> f {}
{ }

nix-repl> builtins.valueSize (f {})
270130

nix-repl> x = f {}

nix-repl> x
{ }

nix-repl> builtins.valueSize x
32

I think that's the left inverse of pkgs: pkgs.buildPackages.

It's both: pkgs.buildPackages.targetPackages == pkgs and pkgs.targetPackages.buildPackages == pkgs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: cross-compilation Building packages on a different platform than they will be used on 6.topic: stdenv Standard environment 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants