-
-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
stdenv: add stdenv.isAvailable pkg
#245322
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it looks like you touched that code so you might know, why is assertValidity
having both attrs
and meta
parameters? What's the difference between attrs.meta
and meta
?
Oh man was this frustrating.
So for example, The reason why This is mainly because We could probably make this into a lot less of a footgun by arranging things so that the attrnames of |
pkgs/stdenv/generic/default.nix
Outdated
isAvailable = pkg: | ||
lib.meta.checkAvailability buildPlatform pkg.meta.availability.build && | ||
lib.meta.checkAvailability hostPlatform pkg.meta.availability.host && | ||
lib.meta.checkAvailability targetPlatform pkg.meta.availability.target; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why this is desirable.
So far we have been trying to deprecate the various stdenv.is*
in favor of the more accurate stdenv.*Platform.is
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note the &&
in the code above.
I don't understand why this is desirable.
Because availability isn't a function of just the hostPlatform
(or just the buildPlatform
, or just the targetPlatform
). You need all three in order to decide.
Perhaps a more-perfect solution would be to have stdenv.platform.{build,host,target}
and then add stdenv.platform.isAvailable
. That would place isAvailable
the smallest attrset containing all the needed information. @Artturin, would this resolve your concern?
@@ -232,6 +232,8 @@ else let | |||
|
|||
outputs = outputs'; | |||
|
|||
referencesOnBuild = nativeBuildInputs ++ propagatedNativeBuildInputs; | |||
referencesOnHost = buildInputs ++ propagatedBuildInputs; | |||
references = nativeBuildInputs ++ buildInputs |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pr has big performance impacts too (current ofborg eval time is ~11 minutes)
Percentages are more useful here: ~5% on all but one metric (and that one metric at +10% is not an allocation metric).
That said, there are still lots of optimization opportunities. If performance is the only remaining obstacle I will implement those. I believe the overhead can be brought down to ~1% but I don't want to put in that effort if it's going to be wasted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1c68dedefcb5ac3c2b2757fc076a1f9850c4c1a1 should improve this. Let's see what OfBorg says.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest eval report, at 4141be0. It's basically neutral; a mixed bag of under-5% changes (about half faster, half slower). The only thing that stands out is the +12% on nrLookups
, but attrset lookups are fast and allocate no memory -- this metric is far less important than most of the others.
Squashed. |
It turns out that most of the eval overhead is due to people making wacky choices for In practice, So I'm going to leave |
This commit adds `stdenv.isAvailable pkg`, which checks `meta` availability not only for the hostPlatform, but also for the `buildPlatform` and `targetPlatform` using `lib.meta.checkAvailability`. Our current meta.{platforms,badPlatforms} scheme isn't capable of expressing constraints that apply to anything other than the hostPlatform. For example: - LLVM can't be used with targetPlatform.isMips64n32, but GCC can The clumsy way of working around this is to have `buildRustPackage` inject `meta.badPlatforms` values into the packages it creates, but that covers only Rust (not other LLVM-based compilers) and only those which use `buildRustPackage`. Really, it's the wrong layer at which to express these things. This commit adds `meta.availability.{build,host,target}.{bad,only}` which are computed automatically based on a package's dependencies. This is done in a backwards-compatible way for the hostPlatform: `meta.availability.host` will incorporate `meta.{platforms,badPlatforms}` from both the package as well as all of its `buildInputs`. The constraints are cumulative, so deep recursion (like checkMetaRecursively) is not needed; each package's `meta.availability` only needs to look at that package's immediate inputs. This commit also causes check-meta.nix to utilize the information in meta.badBuildPlatforms and meta.badTargetPlatforms. In particular, the following test commands now behave correctly: # XFAIL: golang can't build *on* MIPS, but it can cross compile to it nix-instantiate . -A pkgsCross.mipsel-linux-gnu.go # SUCCESS: golang can't build *on* MIPS, but it can cross compile to it nix-instantiate . -A pkgsCross.mipsel-linux-gnu.dnscrypt-proxy2 # XFAIL: golang can't target Mips64n32 nix-instantiate . -A pkgsCross.mips64el-linux-gnuabin32.dnscrypt-proxy2 # XFAIL: rust can't target Mips64n32 nix-instantiate . -A pkgsCross.mips64el-linux-gnuabin32.tiny
We can't build a `go` compiler on MIPS due to lack of bootstrap binaries. However, go is in fact capable of emitting code for MIPS, so we can cross compile to a MIPS target (useful for embedded routers!).
Squashed. |
Latest eval report, at 4141be0. It's basically neutral; a mixed bag of under-5% changes (about half faster, half slower). The only thing that stands out is the +12% on That But the The far more important metric is |
Next push will be an experiment with having hashing replace sorting, to see which wins. |
The PR is still in draft, what is missing before being ready for review? |
Why did you closed it? |
#50105 (comment) somebody else will have to pick up the work probably |
Adam has closed many of his PRs. I don't think that was necessary, and I hope he's doing well. |
I'm fine, Robert. Thanks for your concern. |
Motivation
The current
meta.availableOn
has two problems:buildInputs
are nothostPlatform
. For example, a package is "available" even when:hostPlatform
buildPlatform
hostPlatform
binaries but!buildPlatform.canExecute hostPlatform
This PR fixes both problems.
Specific examples of multi-platform constraints
buildPlatform.canExecute hostPlatform
, and we have no clean way to express this. Often (xdg-open
,p11-kit
, and others) these packages are optional dependencies, and the depending package needs to detect their unavailability so they can be omitted rather than simply failing the build.targetPlatform.isMips64n32
, but GCC cango
is missing bootstrap binaries for a few platforms yet can generate code for those platforms, so we can cross-compile to these platforms but can't build a native compiler for them.ghc
is missing usable bootstrap binaries forpowerpc64*
, although it can emit code for (and even compile itself for) that platform.Description of changes
This commit adds
meta.availability.{build,host,target}.{bad,only}
, which are computed automatically, and updatesmeta.availableOn
to use these attributes.For the hostPlatform this is done in a backwards-compatible way:
meta.availability.host
incorporatesmeta.{platforms,badPlatforms}
from the package as well asmeta.availability.host.{bad,only}
from itsbuildInputs
. Because the constraints are cumulative at each package, deep recursion (like checkMetaRecursively) is not needed; each package'smeta.availability
only looks at that package's immediate inputs.For the buildPlatform and targetPlatform two additional package-provided
meta
attributes are needed:meta.badBuildPlatforms
andmeta.badTargetPlatforms
. The latter is needed only for compiler-like packages.badBuildPlatforms
is useful; I don't have an example for it yet. Howevermeta.availability.build.bad
is definitely useful when we have a compiler written in its own language that can generate code for a platform but upstream doesn't provide "bootstrap binaries" for that platform. Two real-life examples: GHC on powerpc64le and golang on all flavors of MIPS.This PR also adds a boolean,
meta.requiresBuildCanExecuteHost
, to signal a very common cross-compilation problem: build processes that assume the build platform can execute host platform binaries.This PR also adds
stdenv.isAvailable pkg
which checks availability constraints for all three platforms (build, host, and target) as well asmeta.requiresBuildCanExecuteHost -> buildPlatform.canExecute hostPlatform
. This should be preferred overmeta.availableOn platform pkg
.Concrete example
One use-case for this is being able to automatically compile all eligible packages using the
mips64n32
ABI (which cuts memory usage dramatically for pointer-intensive data structures) and build onmips64n64
only those few packages which don't supportn32
(usually because they're written in Rust or useboost-context
).The clumsy way of working around this would be to have
buildRustPackage
injectmeta.badPlatforms
values into the packages it creates, but that covers only Rust (not other LLVM-based compilers) and even then covers only those which usebuildRustPackage
. Really, it's the wrong layer at which to express these things.meta.broken
isn't helpful here either. It is shallow, and its "deep" versionmeta.available
is based onthrow
, so it can't be used to do anything other than to abort eval (see example below). This is whymeta.availableOn
doesn't (and can't ever) understandmeta.broken
.Includes
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)