Can I see a single example?
> rg 'unpackPhase ='
pkgs/misc/foldingathome/default.nix
11: unpackPhase = "tar xvzf $src";
Ok, undestood.
Let’s count it as the second (atfer override*) issue the mass-insertion of runHooks might solve.
Although, I would prefer to write
unpackPhase = "tar xvzf $src";
with the meaning to override post-unpack fixing of timestamps, and
unpackPhase = "tar xvzf $src; updateSourceDateEpoch";
when the timestamps are to be fixed
Perhaps
unpackPhase = "runHook preUnpack; tar xvzf $src; runHook postUnpack";
is more readable/maintainable for the majority of the people.
The way the phase is expected to be written today is
unpackPhase = ''
runHook preUnpack
tar xvzf $src
runHook postUnpack
'';
I’d really like us to move to a world where just writing unpackPhase = "tar xvzf $src" doesn’t disable the pre/post hooks and you have to instead write something like skipUnpackPhaseHooks = true to skip the hooks.
7 Likes
I see that some of the hooks in nixpkgs/pkgs/build-support/setup-hooks/ create own phases, so they won’t be disabled in phase override.
So set-source-date-epoch-to-latest.sh could be written in that way if you want it to stay on unpackPhase override. It might be a simpler solution than taking care in every derivation.
How? The closest I can see to what you’re describing is there’s a few specific spots that hooks can insert phases, such as prePhases, preInstallPhases, etc. but those don’t exist in between every phase, and in particular there’s no such spot in between unpackPhase and patchPhase.
Also any derivation that explicitly sets phases (of which there are a bunch) disables this feature too. So for example if we did have such a postUnpackPhases extension point and set-source-date-epoch-to-latest.sh used it, then the following would break that despite running unpackPhase:
pkgs/applications/misc/multibootusb/default.nix
32: phases = [ "unpackPhase" "installPhase" ];
2 Likes
Here is an example of someone expecting to use a preConfigure hook and being surprised by an hardcoded configurePhase.
Similarly, buildRustCrate used to ignore those hooks for its custom install phase, until someone needed to use postInstall.
Our nixpkgs manual states this for every pre- and post- hooks:
Hook executed at the start/end of the xyz phase.
… and not:
Hook sometimes executed at the start/end of the xyz phase.
I believe there needs to be a consistent behaviour and that pre- and post- hooks should always be available, regardless of how a package is built (hardcoding phases or not, custom build functions or not, etc). Because of PoLA, and because it’s just better UX.
Similar to @lilyball I’m all for a way to hardcode phases that doesn’t lose these hooks. AFAICT this is not a trivial thing. Until then, I’m still convinced any hardcoded phase should start with runHook prePhase and end with runHook postPhase.
4 Likes
Yes, build* functions definetively need runHook, I fixed buildPerlModule recently for the same reason.
About the derivations which overwrite *Phases, if you open a PR which will add all 20000 runHooks, I will say nothing against it (although I hope that discussing the PR will produce a more optimal solution).
What I do not like is adding dozen runHook at random places in irrelevant PRs, there should be either 0 or all 20000, anything in between is inconsistent garbage which one cannot rely on
1 Like
So that’s what this whole rant was really about. 
Do we have a policy stating that an improvement must be made treewide or not made at all? I don’t remember reading something in that vein, but maybe there is.
Treewide PRs also means huge code churn and plenty of merge conflicts for existing PRs. It’s my understanding that some “lesser” issues can also be addressed when opportunity arise during review of usual PRs like version updates.
All at once or little by little, in the end it’s the same amount of occurences that would need fixing. So your dislike is irrational.
I may end up crafting the treewide PR that you demand, but it’s not my priority. Nor was it anyone else’s until now, apparently.
Note that the PR above added only postInstall to buildRustCrate, but not preInstall. Similarly, your PR focused on buildPerlModule and left out other custom build functions that may be flawed in the same way (I don’t know if there is – haven’t checked). I still call that valuable contributions. In your parlance, I guess you say irrelevant random garbage.
I do not think adding runHook to installCheckPhase as an improvement, it is a no-op change which could be justified only as a small step (1/20000) in the tree-wide improvement (“it is no-op but the same as in every other derivation for consistency”)
Same change in unpackPhase of foldingathome (the example above) could be seen as a small fix.
But no-op changes… sorry, I cannot see them as small improvements without the context of the final goal of making runHooks consistent. And that conflicts with the tactics, which requires too many years to get things into consistent state by making so small contributions. Then better not to do it at all in order to have the things in the consistent unfixed state.
Well, there are other tree-wide improvements going in small steps, like name → pname migration, whose final goal is still under discussion and it would be nice to get it clean (although my mass-change PR has been accepted I still feel uncomfortable that the final goal of how pnames are to be mapped to upstream/repology names is still not defined). Other tree-wide refactorings which got into a half-fixed state are not good examples to follow
It is probably good time to introduce the policy, because it gets ridiculous that people tend to break consistency for the sake of consistency.
1 Like
Introducing a policy like this is a great way to ensure nothing ever gets better, because treewide changes like this have a very high barrier to actually doing.
1 Like
I’ve reopened [RFC 0032] Phase running changes for better nix-shell use by dezgeg · Pull Request #32 · NixOS/rfcs · GitHub to fix this generically!
If @dezgeg doesn’t want to carry it forwards as author and someone else here does feel free to speak up! Otherwise I’ll take over on thursday when we have a RFCSC meeting anyway.
5 Likes
This runHook flashmob has pretty good virality
New PR, new author, same pattern: refactoring introduces runHook’s in unrelated PR
Yea, I still see it as, “probably more correct, and may be useful. But in most cases, unneeded”.
The original maintainer may have had use cases where the hooks where necessary. Seeing as dune is a buildtool for OCaml packages, this may very well be the case