haskell-generic-builder: Use args ? attr for passthru (closes #94246) #94251
Conversation
|
@GrahamcOfBorg build haskellPackages.lens |
| // optionalAttrs (args ? preBuild) { inherit preBuild; } | ||
| // optionalAttrs (args ? postBuild) { inherit postBuild; } | ||
| // optionalAttrs (args ? doBenchmark) { inherit doBenchmark; } | ||
| // optionalAttrs (args ? checkPhase) { inherit checkPhase; } |
There was a problem hiding this comment.
In this PR, it looks like you're changing lines like
// optionalAttrs (checkPhase != "") { inherit checkPhase; }to lines like
// optionalAttrs (args ? checkPhase) { inherit checkPhase; }I think I understand your argument as to why this is a good change, but the logic changes slightly. Instead of comparing checkPhase to the empty string, you're now just checking whether args has a checkPhase field.
Is this going to cause any subtle bugs for users using things like checkPhase is a weird way?
It is hard to say whether this PR is okay, because I don't quite understand all the ramifications of this change.
There was a problem hiding this comment.
My understanding of the code would be the only difference (beyond the increased laziness) is that the new one will pass through explicitly empty strings where the former would eat them (as it couldn't distinguish a passed empty string from a default empty string).
There was a problem hiding this comment.
In that case, it sounds like this change is pretty good.
|
This looks like a nice and reasonable change, but my nix foo is too weak to be sure about this. |
peti
force-pushed
the
haskell-updates
branch
3 times, most recently
from
8a7eb3e
to
ed16234
Compare
|
This somehow got forgotten, sorry. I think the change is reasonable and am willing to merge it. But sadly can you first rebase it and clean the diff up? |
…4246) Comparing to a default value to detect if an argument was provided forces it to at least WHNF, which can cause failure (e.g., if the argument is a string with a quoted path from a broken package).
twhitehead
force-pushed
the
haskell-updates
branch
from
c12caa7
to
612f332
Compare
|
Should be good now (was just issues with other commits in the Haskell branch). |
|
Great, thanks! But now I am a bit worried about the test results. ofBorg is claiming this changes the output hash of quite a lot of packages.. I mean this could be completely harmless but I am a bit worried about it. And would feel much more at ease if we were to know, how those packages changed. It is my understanding that this change is not supposed to change packages, is it? |
|
I have the feeling that nearly all this packages are opengl related. So this might well come down to one single weird package that uses some in-band coding … |
|
It will now pass on empty strings where the prior one would eat them (i.e., if you passed |
|
Okay, I think the culprit is:
|
|
@GrahamcOfBorg build haskellPackages.OpenGLRaw |
|
That would make sense. If I recall the shell scripts that do the actually building, they don't distinguish between not set and set to empty, so it won't change the result, but it will change the hash. Technically you do a quoted attribute name expression on the LHS that returns null if you don't want the attribute to avoid passing in |
|
Thx for the PR! |
Comparing to a default value to detect if an argument was provided forces it to at least WHNF, which can cause failure (e.g., if the argument is a string with a quoted path from a broken package). Replaced with
args ? attras theargsset does not contain defaulted values.Motivation for this change
Closes #94246.
Things done
Besides verifying that accessing the derivation no longer forces any provided phase override strings, I rebuild a random haskell project I had after apply this change. The nix environment produced all the required packages and the program ran, so I presume all is still good.
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)