darwin.moltenvk: fix the eval on linux#400739
Conversation
Without the change the eval fails as:
$ nix eval --impure --expr 'with import ./. { system = "x86_64-linux"; }; darwin.moltenvk.NIX_CFLAGS_COMPILE'
error:
… in the condition of the assert statement
at pkgs/stdenv/generic/make-derivation.nix:716:11:
715| n: v:
716| assert assertMsg (isString v || isBool v || isInt v || isDerivation v)
| ^
717| "The `env` attribute set can only contain derivation, string, boolean or integer attributes. The `${n}` attribute is of type ${builtins.typeOf v}.";
… in the left operand of the OR (||) operator
at lib/asserts.nix:39:31:
38| # TODO(Profpatsch): add tests that check stderr
39| assertMsg = pred: msg: pred || builtins.throw msg;
| ^
40|
(stack trace truncated; use '--show-trace' to show the full, detailed trace)
error: expected a set but found null: null
reckenrode
left a comment
There was a problem hiding this comment.
Ah, right. MoltenVK is Darwin-only, but it still gets eval’d on other platforms. The fix LGTM.
|
Shouldn't the |
I think it depends on the attributes. Today and some are not: |
|
Right, makes sense. Is the worry that this will break Hydra or some other automated tooling that can’t catch the error? It seems impractical to ensure that attributes of a derivation can reliably be evaluated on unsupported |
|
I would say the nowadays tooling is too good at skipping errors :) Attributes disappear all the time (especially tests) due to simple mistakes that break evaluation. I'm running https://trofi.github.io/posts/309-listing-all-nixpkgs-packages.html time to time to make sure most attributes evaluate without uncatchable exceptions (and to be able to build reverse dependency lists with a bit more confidence that they don't skip too much). The post accumulated about 100 past PRs over 1.5 years. Those show sometimes real (my favourite) sometimes benign (this case) errors. In If this kind of fixes is too much to bear I can skip |
|
Fair enough! I don’t object to the principle here, but I am more uncertain about how we can make it work in practice. For instance, #400427 is going to introduce more of this, because So I’m not sure what the best solution here is – I assume merging that PR is going to break your tooling, which I think is interesting and valuable, but it seems like when it comes to platform differences we approach a situation where we say On the other hand, if very few things in Nixpkgs cause this to begin with, then maybe we’re doing something wrong and should stop making it worse! But I wonder how much of that is just that Darwin generally has to support the kinds of things Linux packages expect by necessity, and there are relatively few Darwin‐only packages… |
|
FWIW, I’ve merged #400427 because of the upcoming freeze, but I’m open to revising the specific approach based on these concerns if we can figure out what makes the most sense. |
|
I saw no new |
jopejoe1
left a comment
There was a problem hiding this comment.
Fixing eval issues is always nice, feel free to ping me on any other prs fixing eval issues.
Without the change the eval fails as:
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.