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:
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,
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.