Making runHook work consistently

Uncategorized
1

I recently stumbled on an a package for which.overrideAttrs { postInstall = "..." }didn’t work, and after some investigation I figured that package has explicit installPhase without runHook boilerplate.

That got me thinking. Sure, that is not the only package with this problem, and indeed: 900 files missing runHook for installPhase, and more for other phases, so making one pull request a time is going to take a long time.

So, I propose two alternative solutions:

Both would mean a lot of work to polish and finalize, so I’d rather first discuss any concerns and preferences the community has. I personally strongly favor patching stdenv approach to avoid having humans to do machine’s job.

The only existing discussion on this topic seems to agree that some across-the-board solution would be nice (without definitive answer which one it should be), that fixing one package a time is better than nothing.

2

I have a lint for that but it would be nice to fix it once and for all.

The canonical issue: Fix the problem of forgotten `runHook {pre,post}*` calls · Issue #175605 · NixOS/nixpkgs · GitHub

Note that if we start invoking hooks outside phases, we will want to avoid running explicit runHooks in the overridden *Phases since hooks are not guaranteed to be idempotent.

3 Likes
3

I opened a treewide long ago exactly about missing runHook in installPhase, but it has stayed in draft still :sweat_smile:

1 Like
4

Sure, we will need sed '/runHook/ d', more or less. My vision is PR of three parts:

  1. Patch stdenv to run hooks
  2. Patch-out explicit runHook tree-wide
  3. Patch-out runHook from nixpkgs manual

Thanks for the issue link

5

You need to consider out-of-tree derivations, too. Whatever you do needs to work whether the runHooks are there or not, for a fair while.

1 Like
6

Something like

if installPhase !~ "runHook preInstall" then
   runHook preInstall
fi

I’ll need to research how to do it with pure bash, but it should be doable.

7

Okay, I think stdenv: run pre/post hooks automatically by KAction · Pull Request #570194 · NixOS/nixpkgs · GitHub now covers all points discussed so far.

  • Now it has protection against duplicate runHook This makes it unnecessary to do tree-wide removal of explicit runHook right now (although I plan to do it in subsequent PR)
  • Updated the documentation to stop requiring explicit runHook(we had several places where it was re-iterated)

Looking for a reviewer/committer.

1 Like