-
-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
How to handle optional dependencies not available on the given (host) platform? #35906
Comments
In seriousness though, this is yet another excellent reason to peak at the |
There is also an option to have Of course, this will bring us really close to a point where we need a single |
@Ericson2314 I think you misunderstood: I'm not asking how we check if something is supported, I'm asking where we check. If we check at |
@7c6f434c yeah, that's a good way to do this. @Ericson2314 can you fit that into your mkDerivation model? |
Peeking at meta of invalid derivations works already. EDIT: almost all attributes should work lazily, not just |
@shlevy I was trying to answer that, just skipped a lot I guess.
Let's have our cake and eat it too: # in lib
{
optionalDep = dep: if dep.meta.available or false then dep else null;
} # strace v1, shorter
{ stdenv, libunwind }:
stdenv.mkDerivation {
buildInputs = [ (stdenv.lib.optionalDep libunwind) ];
} # strace v2, longer
{ stdenv
, supportUnwinding ? libunwind.meta.availible or false
, libunwind
}:
stdenv.mkDerivation {
buildInputs = stdenv.lib.optional supportUnwinding libunwind;
/* somethingElse = ... supportUnwinding ..; */
} |
That's a fine idea, and it fits in that I don't see affecting the core model at all. As long as we keep doing dependency propagation in bash, We'll need to convert that to basically what we have anyways. |
Cool, sounds good. I'll go with the |
|
See also #34444 for making the meta checks work for cross. |
Is it a good idea to (eventually) handle native-ness in the same way? Or propagation? |
@7c6f434c I'm open to that. I mean I think of deps not in terms of the names but the two axes (see unstable nixpkgs manual), so the flattening that top-level attrs do is unfortunate. The limitations of bash mean that effectively we'll convert away from this stuff in the end so it must be just an easy superficial change. |
@Ericson2314 so I'd have Speaking of limitations of bash... We may want to take advantage of I'm definitely in favor of |
@shlevy errrr not to bikeshed, but I must say I like my CC @NixOS/darwin-maintainers |
(Renamed because I don't see this as a cross-specific issue but general portability one. Reopened waiting on the darwin people's input more than my little bike-shed above.) |
Right, that's better. |
@Ericson2314 I wonder how to treat |
Oops, I guess, the hidden SLNOS agenda behind the sequence of #33057 and #35111 got uncovered :)
We had exactly the same discussion in SLNOS (without reaching a consensus).
I made the two aforementioned PRs to ease SLNOS (and, eventually, nixpkgs) into "optionalBuildInputs" route (yep, I confess, the use of `.available` in `relatedPackages` with which I pushed `.available` through review was an accidental revelation, not the original purpose, the original purpose of `.available` was exactly this issue).
As to the essence of issue, I do want them unsupported packages to be explicit because I hate to debug those optional `null`s. You should try tracing `null` inputs in `stdenv.mkDerivation` and you are going to be surprised by how many overrides with `null`s you already use. It's in hundreds (well, maybe less, as those traces do have lots of duplicates, still `grep -r '= null'` is pretty enormous).
They are especially hard to debug when using a package with a custom `callPackage` or overriding an already overridden package. Anything is better than `null`s. I vote for anything except `null`s. Explicitly filtering with `filter (p: p.meta.available)` is fine for me. To be honest, even "strace v2" from above and simple
```nix
buildInputs = [
...
]
++ optionals stdenv.isLinux [
...
]
++ optionals stdenv.isDarwin [
...
]
```
are fine. But I think most people will steer away from such things as they are too verbose. Hence, I vote for `optionalBuildInputs`. Anything but the `null`s.
To be even more honest, now I'm unsure that even `meta.available` is good enough. It's too permissive with all its options of `config.checkMeta`, `config.checkMetaRecursively` and `config.handleEvalIssue`.
Now I'm experimenting with `meta.supported` that only checks the platforms (I also call `optionalBuildInputs` `platformBuiltInputs`, the name is subject to change, though).
As a side note, `meta.platforms` should be checked (or just autogenerated, or maybe both, maybe with `config.checkMetaPlatforms` or something) against platforms produced by intersecting platforms of derivations from `buildInputs`. But I have not implemented that properly yet.
As another side note, we could have `meta.hostPlatforms` or something, that describes platforms you can build the derivation on by doing the same for `nativeBuildInputs`.
|
Action item tracked in #36226 |
Unfortunately, |
Example:
strace
optionally depends onlibunwind
, which hasn't yet been ported toRISC-V
. Do we want thestrace
build expression to explicitly sayoptional libunwind.supportsTarget libunwind
, or do we want thelibunwind
expression to simply benull
on unsupported systems? Or is there another alternative?@Ericson2314 @bgamari @dtzWill (we need @nixpkgs-cross...)
The text was updated successfully, but these errors were encountered: