Explicit runHook in derivation

Recently I have seen some PRs made by different people which besides the main goal of PR insert void runHook here and there

For example https://github.com/NixOS/nixpkgs/pull/67572#pullrequestreview-310159482

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?

Is there a Halloween flashmob running?

1 Like

More PRs can be found at https://github.com/NixOS/nixpkgs/search?o=desc&q=runHook&s=updated&type=Issues
There could also be direct commits without PRs

Which are you saying?:

  1. Supporting overriding derivations is questionable
  2. runHook isn’t necessary to support overriding derivations
  3. runHook doesn’t work to override derivations
  4. runHook won’t work unless it’s applied consistently, and it isn’t, so don’t add it
  1. 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.

Hello @volth

First things first : the term of “guerilla tactics” is inappropriate. Think longer about your choice of words if you really have a point to raise. :wink:

In the PR you linked , you misread my explanation. I’m not saying “support for overriding derivation”.

This ofBorg issue about linting some common mistakes is relevant :

  • When hardcoded phases are used but they lack runHook

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:

  1. Some people believe they should be included in all derivations, and is a bug when they’re omitted or forgotten.
  2. 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.

3 Likes

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.

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 (https://github.com/NixOS/nixpkgs/pull/72326#pullrequestreview-309650877 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.

This RFC is potentially relevant: https://github.com/NixOS/rfcs/pull/32

2 Likes

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.