The motivation explained by them is very questionable (“support for overriding derivation”), which in my opinion is not needed, but even if I am wrong and it has some usefulness - I wonder, why it is not added treewide, but using a kind of a guerrilla tactics in a irrelevant PRs and in random derivation?
runHook isn’t necessary to support overriding derivations
But even if I am wrong here, I see the choice of altered derivations as very strange (are these derivations need better or urgent support than the rest of nixpkgs?) and the tactic of adding hooks (in irrelevant PRs) are strange too.
While I think the real solution is that stdenv and convention should support phases in a way that means overriding them doesn’t disable hooks (I recall seeing discussions regarding this at some point?), that isn’t the case today. The inclusion/addition of an explicit phase without including runHook disables a feature that stdenv normally provides. Whether or not overrides using them are “questionable”, it feels like a bug that hooks are a feature that just sometimes don’t work because a phase uses custom logic and someone forgot to include runHook. Until a better solution exists, it seems appropriate to include them in custom phases.
I see the choice of altered derivations as very strange (are these derivations need better or urgent support than the rest of nixpkgs?) and the tactic of adding hooks (in irrelevant PRs) are strange too
This is generally the result of two behaviours:
Some people believe they should be included in all derivations, and is a bug when they’re omitted or forgotten.
It’s common practice when making a PR to also clean up some inconsistencies or bugs found within, even if they’re not explicitly necessary or related to the changes at hand. Many people are lazy and don’t want to make a new PR for what they think are trivial fixes they noticed when doing something else.
These people may also believe this should be done tree-wide, but don’t feel up to the effort of pushing that through, so they fix the ones they come across instead. Possibly they also think the effort would be put to better use by designing a universal solution.
Thanks for making this thread and I agree re the random PRs bit.
I typically just follow through with whatever recommendations I get unless I really feel I know better, but the way these have been recommended has just seemed a little off to me.
If it’s a random PR to a derivation that adds this, then whatever, but tacking it onto e.g. simple version bumps (like this PR where I first saw it) just seems odd. Purely in terms of relevance, at least.
Maybe when I’m refactoring the entire expression or any related phases, sure, but a version bump?
That being said, this topic does interest me and I’m curious to know what the best solution is.
Of course it is. Adding the hooks in the last commit and then immediately merge the PR preventing people even looking at it - how can I call it else?
“Hooliganism” would be better?
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.
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
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.
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.
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.
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:
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.
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.
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.
