Any PR on a package is a chance to fix and improve. I’m not reviewing only the lines changed by the PR.
As for the hardcoded phases missing runHooks, I suggest to add it whenever I see it.
1 Like
Please do not perceive it as a personal attack. There are at least @peterhoeg, @jonringer and @vyp doing the same - adding runHook to own PRs or pitching other to do it in their PR (svaba: init at 1.1.0 by scalavision · Pull Request #72326 · NixOS/nixpkgs · GitHub etc).
If there is a real problem, let’s articulate and solve it
Adding runHook in irrelevant PRs is not a solution just because you guys will need centuries to process whole nixpkgs doing what you are doing. Meanwhile it will be half-fixed.
Adding runHook treewide by a bot is not a solution too, because it will add 200.000 lines.
Perhaps override and overrideDerivation are to be fixed if you are not happy with them?
With any outcome your recent insertions will remain as a garbage, so please stop
2 Likes
Like many expression practices in nixpkgs, these things are convention with no set standard. I was told to add the hooks in a PR by someone else a long time ago, and never really saw a down-side other than future-proofing.
EDIT: The only use case where I saw the hooks being necessary is if you were creating a buildXXXXPackage function, and was meant to be used as a wrapper around mkDerviation, so those hooks make a lot of sense. But for most packages, I agree, it will likely be just visual noise.
2 Likes
Maybe it would be nice to get some consensus on when runHook is appropriate, then formalize that in the manual or related documents. Until then, I think this is largely personal preference.
EDIT: It’s nice to have the contribution guidelines documented because then it’s a source of truth i can use if someone isn’t adhering to it. We have no analogous source of truth on runHook conventions.
2 Likes
I see no reason not to include the runHooks. If you omit them, that just leaves a landmine waiting for a future maintainer who tries to add a package setup hook and doesn’t understand why it doesn’t work, or worse yet, accidentally breaks a package setup hook (such as the default one that compresses manpages) and doesn’t even realize it.
Long-term, we should probably fix it so the standard way of customizing a phase doesn’t disable pre/post phase hooks. Until such time as we have a standardized mechanism, I fully support adding the pre/post runHooks in any PR that touches the derivation.
3 Likes
Thanks for the mention, personal attack not perceived.
As for my review, the author already had the runHooks there, I was only suggesting a typo fix on a runHook line:
https://github.com/NixOS/nixpkgs/pull/72326#discussion_r340961730
I did actually ask on irc about the runHook’s though because you’re right, I don’t normally see runHooks when setting a phase so I was curious about it:
https://logs.nix.samueldr.com/nixos/2019-10-31#2734125
04:57 <xd1le> is it normal to set installPhase and have runHook preInstall at the start and runHook postInstall at the end?
04:58 <xd1le> or is there a 'better' way?
05:00 <xd1le> because I seem to remember that setting phases explicitly was not recommended but can't find that suggestion
05:03 <clever> xd1le: if you do explicitely set a phase, the pre/post hooks will cease to run (enless you add the runHook's back in yourself)
05:05 <xd1le> ok so I guess if installPhase needs to be set, wrapping it with runHook's is good
05:05 <xd1le> clever: thanks
I understand that irc is not a conclusive source of information, I’m just clearing up any questions on my intention. I don’t really have a strong opinion on this, I’m happy to suggest whatever the idiomatic nixpkgs is. As you said, override and overrideDerivation can be used to override installPhase as necessary and I agree so I was also wondering about why the runHook’s were there. I didn’t have a good reason to remove them either even after asking on irc, so I just suggested on the typo fix, not on adding the runHooks itself.
How did you come up with this number? The whole nixpkgs repo mentions those phases about 6k times:
% grep -r buildPhase . | wc -l
958
% grep -r installPhase . | wc -l
3088
% grep -r checkPhase . | wc -l
1249
% grep -r unpackPhase . | wc -l
360
% grep -r fixupPhase . | wc -l
113
So in the very worst case we would have to add no more than 24k lines. And even if it’s 200k lines, I don’t see a big issue with doing that. I do agree with the rest of your point that adding those manually is just a major waste of time.
I calculated when saw people working on inserting runHook for the first time. It was few days ago, and probably the value was remembered incorrectly (or did it by multiplication of extra 16 lines for each 10k+ derivation).
ok, let it be ~20.000, not ~200.000
Could we articulate the problem with override* and then fix it in a generic way, by altering override* or by introducing a new set of override* functions to address the problem (which I fail to see, but I accept that it could be there).
Can we please stop focusing on override? I think that’s a red herring. Forgetting the hooks is a problem even for non-overridden packages because of the potential to silently break existing package setup hooks or to confuse future maintainers when adding hooks down the road.
Besides, the problem wouldn’t even be fixed at the override level, it would be fixed by changing the mechanism by which we override the default phases such that it doesn’t suppress the hooks.
Hmm. Any concrete examples or tickets ?
I checked last night, nearly all of the default hooks are on the fixupOutputs phase which seems rather unlikely for someone to override. There is one on postUnpack though (set-source-date-epoch-to-latest.sh), which means it’s being skipped for all of the various packages that override unpackPhase because in a quick spot-check right now I’m seeing a lot of unpackPhase overrides that don’t run the hooks.
There are other opt-in hooks too, but I’m not particularly familiar with what phases they’re attached to.
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