postgresqlPackages: replace custom installPhase with buildPostgresqlExtension helper

[wolfgangwalther]

Description of changes

Most postgres extensions are built with PGXS, postgres' build system for extension. All of them have in common, that they override the installPhase, because the pgxs Makefiles don't work correctly in nixpkgs out of the box. This PR fixes that - by providing the DESTDIR option, which all those Makefiles support. However, this will be added to the full paths which are returned by pg_config and will temporarily result in paths installed as $out/nix/store/..-postgresql-.../<proper-path. To deal with that, a postInstall hook cleans up and moves the files back to $out/ directly without the additional nix/store/....

This works nicely - I was able to build all extension like this without any custom installPhases anymore. Even more, I was able to significantly simplify the postgis derivation with that.

The current approach has two downsides, from what I can tell:

• postInstall doesn't work as expected, because at this time the files are still in $out/nix/store/.../ instead of $out. The postInstall hook registered via setup hook will always run after the postInstall hook in the derivation.
• All packages having postgresql in their build dependencies will get this setup hook and thus the DESTDIR setting automatically, not only the extensions in postgresql/ext. While this is good thing for those which use PGXS - there might some other packages that just happen to have postgresql as a dependency without it's build system. Setting DESTDIR could have unintended effects, if they happen to use that differently.

One way to solve both of this would me to create a wrapper around mkDerivation specifically for postgresql extensions - and then prepend the postInstall hook code to the derivation-provided postInstall code. This would always run first, making postInstall work normally again. This wrapper would have to be used explicitly - and all postgresql extensions would do so. Other packages would not be affected.

Before I go that way, I would like to have some feedback on the general approach here and whether is anything that I might have missed or is not working currently. Since postgis is changed / simplified the most, I'd welcome a review of some of the postgis maintainers: @NixOS/geospatial @MarcWeber

Things done

• Built on platform(s)
  • x86_64-linux (all extensions, each on a supported postgresql version)
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin (all extensions, each on a supported postgresql version)
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[das-g]

One way to solve both of this would me to create a wrapper around mkDerivation specifically for postgresql extensions - and then prepend the postInstall hook code to the derivation-provided postInstall code. This would always run first, making postInstall work normally again. This wrapper would have to be used explicitly - and all postgresql extensions would do so. Other packages would not be affected.

That sounds like the right way to me.

[wolfgangwalther]

One way to solve both of this would me to create a wrapper around mkDerivation specifically for postgresql extensions - and then prepend the postInstall hook code to the derivation-provided postInstall code. This would always run first, making postInstall work normally again. This wrapper would have to be used explicitly - and all postgresql extensions would do so. Other packages would not be affected.

Did that. This works very nicely now. I built all extensions on aarch64-darwin and x86_64-linux - all of them have the same build result as on master, so succeed when they did before, and fail with the same errors when they did before. But overall, we have much less code in ext/ - extensions just mostly "work out of the box" now.

The first three commits had already been merged to master earlier, just putting them here to make my rebasing easier later. The last commit is the one to look at.

[das-g]

Not sure I'm qualified to review this, but here's what questions came up for me:

[wolfgangwalther]

rebased due to merge conflict

[das-g]

Great work! Looks good to me. 👍 (With the already-mentioned caveat that I don't feel very qualified to review this.)

(Consider using one of the more standard-ish spellings of "OK"/"okey".)

[Ma27]

I'm fine with the current state since it makes the situation easier for extension packages, but I don't really like it.

But I wonder, how much effort would it be to patch PGXS to not assume the output of pg_config as base installation directory?

[wolfgangwalther]

I'm fine with the current state since it makes the situation easier for extension packages, but I don't really like it.

Exactly how I'm feeling about this, too. It isn't pretty, but it's better than the status quo.

But I wonder, how much effort would it be to patch PGXS to not assume the output of pg_config as base installation directory?

You can't, really. That's what I started with, patching the PGXS Makefiles that PostgreSQL provides. But each extension includes those Makefiles - and then builds on top of that. So we'd have to suddenly patch the extension's Makefiles, too - if we wanted to make that solution generic, i.e. without knowing those Makefiles in advance, then it's... hard.

There is one approach that I didn't try, yet. We could try to wrap the install command - and redirect everything that this command tries to install into postgresql's output into one of the extension outputs. Kind of the same approach as in here, but one step earlier.

This would only work, though, if all extensions were really using install and not some random cp or so, too. I didn't test that, yet.

[Ma27]

That's what I started with, patching the PGXS Makefiles that PostgreSQL provides

So after taking another look at the Makefiles of some postgresql extensions I see what you mean and I have to agree.

Let's stick to the approach. There are still a few minor things to be done (and perhaps the postgis maintainers @NixOS/geospatial @MarcWeber also want to take a look), then we can ship this.

[Ma27]

@wolfgangwalther I think we've discussed everything in here. Anything I can assist you with here to get it done?

[wolfgangwalther]

@wolfgangwalther I think we've discussed everything in here. Anything I can assist you with here to get it done?

I'm currently travelling and will look into this and other PRs end of next week again.

[wolfgangwalther]

Rebased and addressed comments. Still building everything locally, will take a while.

[wolfgangwalther]

Still building everything locally, will take a while.

All looks good from my side.

[Ma27]

Built a lot of stuff today, looking good!
Would like to take a closer look at the diff tomorrow to see if I missed something (too tired now, sorry). Then I'd merge.

[wolfgangwalther]

Rebased to resolve conflicts.

[wolfgangwalther]

Hm, ofborg failure seems unrelated to my changes. I will try another rebase on staging.