stdenv: support __structuredAttrs, opt-in per package #85042
Conversation
At this stage, this doesn't yet really work at all, because a lot of code in stdenv's setup scripts assumes the old API. In the following series of commits, we'll develop support so it's usable.
In some places it's necessary to condition on structured attrs in order to handle both cases correctly. We'll use this variable as a nice explicit way to spell that.
This part of the API is designed so that the old-fashioned shell style works cleanly. So we can get the full benefits of structured attrs without breaking compatibility here.
This covers buildInputs, nativeBuildInputs, depsBuildBuildPropagated, and all their relatives.
We don't disturb the `srcs` variable itself, because that could interact with a postUnpack or other hook. Just let it remain either a string or an array, depending on whether this derivation has structured attrs.
We'll want to use this at a handful of spots in stdenv's scripts
that say things like
configureFlags="--enable-foo --disable-bar $configureFlags"
and can instead say
_prepend configureFlags --enable-foo --disable-bar
This causes these to behave correctly with or without structured attrs enabled.
We'll use this shell function in a number of spots in setup.sh
that use a pattern like this (often with still more items in it):
local flagsArray=(
SHELL=$SHELL
$makeFlags ${makeFlagsArray+"${makeFlagsArray[@]}"}
)
which assumes `makeFlags` is a string to be split, and can
instead say:
local flagsArray=(
SHELL=$SHELL
)
_accumFlagsArray makeFlags makeFlagsArray
in order to behave correctly both with and without structured attrs.
In every case this pattern appears, the local variable is named
`flagsArray`. So we keep this function a little simpler to read
by baking that name into its interface.
Similarly, in a lot of cases there are some input variables that
should be treated as arrays even in the old-API case; and every
such input variable has a name ending in `Array` and vice versa.
So we bake that convention into the interface too, in order to
save the complexity of specifying which to treat which way.
That all only makes sense as long as this function is for
stdenv's internal use. If we find we want to use it more widely
(and take off the leading `_` from the name), it'll be better to
make the interface more explicit and just eat the complexity that
that adds.
This takes care of the handling of buildFlags, checkFlags, installFlags, installTargets, and part of the handling of makeFlags.
This takes care of the handling of installCheckFlags, distFlags, and the remaining use of makeFlags.
This takes care of the handling of stripDebugList, stripDebugFlags, stripAllList, and stripAllFlags.
This one has very few references in the tree -- just here, the manual, and a handful of expressions setting it.
Because the specification for this attr is that the elements are glob patterns, this line where we use it will stay `[*]` (vs. `[@]`) and unquoted even in a fully structured-attrs future.
Specifically, I believe that for the majority of simple packages, enabling it will Just Work, with no further effort. That's based on taking a random sample of simple packages, building with and without, and carefully comparing the build logs. (With a tool to normalize out boring differences.) In the sample, all built successfully, and ~90% had no differences detectable in the build log. For that sample, a "simple package" was defined as one where (a) the expression directly calls `stdenv.mkDerivation`, not another abstraction; and (b) the file is <=30 lines long, which puts it in the smallest 30% or so of such packages.
First demo example! Tested in nix-shell that both `netmask` and `info netmask` seem to work. Also studied the build log before and after (with the output hash normalized away), and no difference was visible there.
ofborg
bot
added
6.topic: stdenv
ofborg
bot
added
10.rebuild-darwin-stdenv
In case they're useful to anyone else, or if only to help make clear just how these tests were done, here's:
I'm pretty happy with the The other essential ingredient in reading these logs was |
|
I can get on board with this sort of thing, but I think it's important we have a bounded transitional period and don't go on supporting both indefinitely. For example I would say if we merge this now, commit to removing the old way by the release 20.09, and if the deadline comes before all the old packages are converted, too bad: keep the deadline and just break those packages. |
Sure. From my perspective at least, supporting both is just a migration strategy — a means to help the migration go faster. I'd love to see it progress to being able to hardwire the |
|
I'd prefer not to do this currently until the work on stdenv has been stabilised. Only yesterday I pushed more changes to that. Also |
alternatively forcing the use of structuredAttrs can be a good way to incite people polish the code (assuming the first merge deals with most of the issues). This may be an unpopular (or misinformed) opinion but I would be fine freezing unstable for a ~ month while solving most issues. |
|
Maybe the RFC could have both the "cold turkey" and "incremental migration with deadline" approaches, and the community could decide. If by some miracle there was consensus with the former, I think that could be a really good exercise in community-wide coordination, not just a faster way to get things done. |
This comment has been minimized.
This comment has been minimized.
stale
bot
added
the
2.status: stale
|
What needs to be done to get this over the finish line? |
stale
bot
removed
the
2.status: stale
That makes sense; stabilize the interface before destabilizing it again. |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/problems-with-python-package/11705/11 |
Has this been done yet? Structured attrs are blocking the introduction of declarative wrappers, which are supposed to solve one of the major pains with packaging graphical applications: #83321 (comment)
We have been waiting for years and packages are currently broken because we can't implement a solution without this. Can we please start to do something? |
|
This is a nix prerequisite before we can merge anything to master: NixOS/nix#4770 Also RFCs for the stdenv changes are necessary But what can definitely be done in the meantime is fix further packages and packaging infrastructure on this branch. |
SuperSandro2000
added
the
2.status: merge conflict
|
I should note though that NixOS/nix#4770 seems to only need a final approval from Eelco :) The other question is whether we can get this backported somehow, otherwise you'd need |
|
|
|
I'm not aware of any blockers but at least the merge conflicts need to be solved |
|
Is an RFC still needed to be able to get this in? I guess we do at least need a plan for when we want to move over to structuredAttrs (since it doesn't make sense to support both ways for an extended period of time). |
|
merge conflicts resolved in #175649 |
Motivation for this change
This PR is inspired by #72074, and is
The key design choice in this PR is that we craft our stdenv changes to make it work both with and without structured attrs enabled. Happily, this only adds a modest amount of complexity relative to supporting only the structured-attrs case.
This PR replaces only a small fraction of the changes in #72074. The majority of the work represented in that PR is about fixing individual packages (lots and lots of them!) to work correctly with structured attrs. My hope is that merging this PR will in turn make it easier to merge many of those changes too.
I'd love to hear feedback from the people who've been authoring and reviewing #72074, including @globin @lheckemann @jtojnar @Ericson2314 @FRidh @teto @Ma27, as well as naturally anyone else interested.
Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)I've tested this PR from two angles:
Clean as is: With this PR as is, not yet turning on structured attrs, I believe it has no effect on how any package is built. (Modulo unknown bugs.)
Works when enabled: For most of the simpler packages in the tree, turning on structured attrs on top of this PR should Just Work. That means no further edits are necessary, and again no effect on how the package gets built.
(More-complex packages will need edits of their own in order to migrate; stdenv: enable __structuredAttrs #72074 includes many examples.)
I've tested each of these on a random sample of packages. To find subtle differences that don't simply cause the build to fail, I compared the build log against baseline.
For comparing build logs, a plain diff shows a lot of noise -- due to the different output path, and to nondeterminism in e.g. the order sources get compiled in. I wrote a little script to normalize these out. In many cases that made the diff between logs completely empty; in the rest, I read through the remaining diff by hand.
The results were (expand here):
Clean as is: I sampled 10 packages using
grep -wl stdenv.mkDerivation | shuf(skipping 1 unfree). I also took the same sample of 25 simpler packages used for the other test, below.35/35 built successfully. 26/35 (5/10 of the broad sample, 21/25 of the simple-package sample) had identical build logs after normalization; and for the other 9/35 I read the complete diffs and determined there was no change. So 35/35 had no change detectable in the build log.
One of these packages was
palemoon, a web browser, with naturally a fairly complex build.Also naturally I rebuilt the whole toolchain at the tip of the PR, and everything seems to work fine. I didn't inspect the build logs for this part.
Works when enabled: I sampled 25 packages from the simpler packages in the tree (direct callers of stdenv.mkDerivation, <= 30 lines long.)
25/25 build successfully with structured attrs.
For 22/25, the build log shows no detectable difference. (Of these 16 had empty diff after normalization; the other 6 were short and easy to read.)
3/25 were more interesting:
gbsplay), the build logs expose a bug in the package expression! I'll send the fix as its own small PR (gbsplay: fix configure flags #85044). After fixing the bug, enabling structured attrs has no effect, as hoped.artha), the log shows an extra "Nothing to be done" line frommake. Harmless in itself, but I plan to investigate in case this is a subtle sign of some bug.closurecompiler), when structured attrs is enabled the calculation ofSOURCE_DATE_EPOCHgets thrown off by the.attrs.shfile. I plan to debug this and fix.I should emphasize that the effects on these 3/25 only appeared when structured attrs was actually enabled. At the tip of the PR, with
__structuredAttrsremaining false, I built these packages as part of the "clean as is" testing above, and confirmed the build logs remained unchanged (after normalization.)In short:
As is, these changes left the build unaffected for 100% of packages I tried, including some complex ones.
On taking the further step of actually enabling structured attrs, the build remained unaffected for about 90% of a sample of relatively-simple packages.