postgresql.withPackages.pg_config: fix paths#419486
postgresql.withPackages.pg_config: fix paths#419486wolfgangwalther merged 1 commit intoNixOS:stagingfrom
Conversation
Previously, pg_config would report the paths of the underlying postgresql derivation and not the paths of the buildEnv that postgresql.withPackages creates. That's a problem when users of pg_config use it to find PostgreSQL's sharedir, in which they'd like to find the extensions added via withPackages. Those are only linked into the created buildEnv, but not available in the postgresql derivation. By providing our own nix-support/pg_config.env file, we can swap out those paths. We also do the same for the -man output, because this output is linked into buildEnv as well. Other paths, which are not available in the buildEnv environment, will still link to the original postgresql derivation. Win-win!
nixpkgs-ci
bot
added
10.rebuild-linux: 501+
| "/share/postgresql/extension" | ||
| # Unbreaks Omnigres' build system | ||
| "/share/postgresql/timezonesets" | ||
| "/share/postgresql/tsearch_data" |
There was a problem hiding this comment.
Why was this shipped with this mess included now?
There was a problem hiding this comment.
I understood #407920 (review), namely the following, as you being OK with that part:
I guess I'll rebuild the extensions with it now and see if I catch something obvious. But if that's not the case, I'd say that we should ship it: I'm very much in favor of getting rid fo the pathsToLink-hack asap, but I do agree that this is a valid correctness fix.
The remaining discussion in the other PR afterwards was about a different issue, where I temporarily re-introduced the cmake flags, which were intended to be removed in the first place. This was because of a buggy implementation I had temporarily, which I have fixed now. The correctness issue is resolved very well, the only inconsistency is pathToLink which remains - hopefully only temporarily, so. But even if it sticks longer, then the correctness thing is important, so that we're not getting other extensions/packages locked in on the bad behavior as well.
It's literally the same PR as the PR before, so this had plenty of review, yes. |
|
So my initial reaction was plain surprise because it was in my list of tabs with things to review (I kinda monitor GH notifications, so it doesn't mean people pinging me to get prioritize something doesn't get lost entirely). Now, there was an approval in the original PR from me, yes. However this was before the removal of the CMake flags got reverted and I questioned whether we should take this approach because we don't even get rid of the hacks in omnigres. I know that you were working on a follow-up, but I didn't know about the state after that, so in my mind this was back to "in review". I agree that this is absolutely up for interpretation here, but for me the takeaway is that an up-front comment would be nice if a PR had a bit of back-and-forth (and code-cycles after that). Anyways, given that nixpkgs-review is fully green and the change looks good in total, there's nothing else actionable here. Despite this, thanks for this patch in the first place. |
Agreed, will try to do better. |
|
This leaks |
|
Uhm, we have an How exactly are you observing this? |
|
I'm doing |
|
Ah.. once we go |
|
Fix in #422969. |
3 participants
Continuation of #407920, which can't be re-opened anymore, retargeting staging.
I consider the state of this ready-to-go.
Things done
./result/bin/)Add a 👍 reaction to pull requests you find important.