postgresql.withPackages.pg_config: fix paths#407920
postgresql.withPackages.pg_config: fix paths#407920wolfgangwalther wants to merge 3 commits intoNixOS:stagingfrom
Conversation
wolfgangwalther
force-pushed
the
postgresql-with-pg-config
branch
from
735daab to
0734b85
Compare
wolfgangwalther
changed the title
github-actions
bot
added
10.rebuild-darwin: 11-100
wolfgangwalther
force-pushed
the
postgresql-with-pg-config
branch
2 times, most recently
from
862d249 to
bb5cd4d
Compare
wolfgangwalther
force-pushed
the
postgresql-with-pg-config
branch
from
bb5cd4d to
aea8669
Compare
Since #404027 is now merged, we'll need to figure out the build failure here. @yrashk could you have a look at this? To me, this seems like the build system might be at fault. |
|
@wolfgangwalther I'll try to build this branch and gather some more logs for @yrashk |
|
So, I can fix this omnigres build failure with: pathsToLink = [
"/"
"/bin"
+ "/share/postgresql/timezonesets"
+ "/share/postgresql/tsearch_data"
];Thus, the reason for the failure seems to be that Omnigres' build system assume that it can just copy SHAREDIR ( So what upstream should really be doing is:
(and avoid copying directories) Or alternatively, it should treat symlinks correctly... although the code looks like it already intends to do so: |
I wonder if it makes sense putting these symlinks in the |
Isn't it simpler to just continue using the CMake flags mentioned above? |
Ma27
left a comment
Ma27
left a comment
There was a problem hiding this comment.
Implementation looks reasonable code-wise.
Do we know more about the potential blast-radius, i.e. other build-systems that fail funnily when getting a symlink instead of a directory from pg_config? Omnigres seems to be one instance already and I'm a little afraid that there are more (considering all the fun we had with pg_config in the past months).
And if that's the case I do wonder if two cmake flags aren't the cheaper and more sustainable option for us.
That also means, if omnigres is an outlier, I'm absolutely in favor.
Not really. Omnigres is the only build system that relies on a PostgreSQL with extensions. Every other extension we know of builds with a
IIRC, they will then fail the same way. The two things are unrelated, I think: CMake needs After having thought about this for a while, I think we can just add to |
wolfgangwalther
force-pushed
the
postgresql-with-pg-config
branch
from
aea8669 to
18f29ae
Compare
Did that. This makes Omnigres build. I still think that their build system should do better there, though. |
|
Hm, omnigres on darwin is still broken with this now: |
Maybe file a bug upstream first? I think I'd prefer to have them look at this before we add package-specific hacks into our builders. |
I think @yrashk is aware, according to his reaction to #407920 (comment). But I could also file an issue, if that's preferred, @yrashk? |
There was a problem hiding this comment.
One optional suggestion.
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.
That being said, if a bit more testing was successful, I'd prefer to merge it rather quick because that's still the kind of change I'd like to see on unstable for a while instead of pushing this right before 25.11.
EDIT: right, this doesn't even rebuild most extensions.
OK yeah rebuilt a bunch of things with @wolfgangwalther feel free to apply my suggestion if you agree and merge. |
Before we can merge, we'll need to figure out the build failure in #407920 (comment), because even with the |
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!
wolfgangwalther
force-pushed
the
postgresql-with-pg-config
branch
from
18f29ae to
ef5af85
Compare
|
The problem is that Before this PR, the wrong pg_config returns the right PGXS path: After this PR, the right pg_config returns the wrong PGXS path: (this just doesn't exist) I wonder why the build system doesn't fail much earlier. |
|
I am able to fix the build failure by passing This fixes PGXS and also eventually the omnigres build - but unfortunately, omnigres is now confused again about where to find plpython3. To fix that, I need to revert the removal of the cmake flags. So while the end-result is a much more correct |
|
There was a problem hiding this comment.
I thought these were supposed to disappear?
EDIT: OK, I missed the part from your previosu comment.
But I must ask: why bother? It looks like we end up with the exact same state as before, but with package-specific hacks in our builder. At this stage I wonder if it's preferable to let upstream figure out how to fix their build-system and then we can apply the correctness-fix, but without the pathsToLink-hacks.
Right now this is effectively dead code from the beginning and if omnigres stays that way, it's always kinda undefined if we can kill it in the future or not (which is exactly what I don't want for e.g. the pathsToLInk stuff).
There was a problem hiding this comment.
Well, at this stage I can't really tell whether I had to reintroduce this because omnigres' build system is at fault - or because I'm still doing something wrong with this correctness fix. The fact that python3Packages.pgvector breaks with something similar is an indication, that I might still be missing something.
So, my goal is to find the problem there, eventually, and then see what remains. Can we then remove the cmake flags? Maybe even the pathsToLink? I can't tell right now, but I won't abandon the PR, yet :)
There was a problem hiding this comment.
I think I owe you an apology: to me, the previous comments read as if the plan was to go with both the CMake flags and the changes to the postgresql build-system (+finding a fix for pgvector) which I do think is a bad idea for the reasons outlined above.
I do agree that doing this fix is valid, but I'd like to only see it in nixpkgs if the fix is actualyl solving the problem (this is especially true for the pathsToLink-part).
If that was the plan all along, then go ahead ;)
There was a problem hiding this comment.
Longer term, the need for Omnigres' build environment to do the copying will go away – Postgres 18 seemingly solves the problems of shipping extensions outside of the hard-coded path. However, we still support Postgres 14 and it will take a few years until 18 will be the minimum supported version.
I think for us, a shorter term good solution would be to split building of extensions and their in-source-tree use (for tests and live experiments).
Building Omnigres extensions does not require a copy of the directory. Testing does – but only in the sense that we did not want to pollute the build directory with built artifacts.
Now I wonder if we can reverse on this decision and move the problem into a different place. We can install extensions into the hard-coded directory (returned by pg_config) and add a target to clean it up (perhaps by reinstalling the version of Postgres we prepared).
With respect to Nixpkgs, in this case, as long as the extensions and libdir directories returned by pg_config would be writable, this should work!
There was a problem hiding this comment.
With respect to Nixpkgs, in this case, as long as the extensions and libdir directories returned by pg_config would be writable, this should work!
In Nixpkgs, all paths returned by pg_config will always be read-only. There is no way for a writeable path here.
But we already pass some writeable directories as install paths. Could omnigres do this as an installCheck? Aka install in the proper location and then do the tests with the installed result?
There was a problem hiding this comment.
We do currently take PostgreSQL_TARGET_PACKAGE_LIBRARY_DIR as an override for pg_config --pkglibdir and PostgreSQL_TARGET_EXTENSION_DIR as an override for $(pg_config --sharedir)/extension.
Does this help?
There was a problem hiding this comment.
I know, that's what I mean. We're already using that to set the install location. But I don't think omnigres is using this for more than installation, right?
We currently have two issues with omnigres' build:
- We need to force the SHAREDIR returned by
pg_configto contain proper directories for/share/postgresql/extensions,/share/postgresql/timezonesetsand/share/postgresql/tsearch_data, but would like some of them to be symlinks only. Right now omnigres seems to copy the symlink, and thus ends up with directories into the read-only nix-store. If omnigres' build system would create those directories itself and then copy the files it needs (instead of copying the directories, which are symlinks), then this would go away. - We somehow need to pass
-DPostgreSQL_EXTENSION_DIRand-DPostgreSQL_PACKAGE_LIBRARY_DIReven thoughpg_configshould return the correct paths. We have a similar failure for pgvector, so this might be a problem on our side. We don't know, yet.
To solve the first problem (bad copying), you proposed to get rid of copying entirely and suggested to instead temporarily install into the paths returned by pg_config. That won't work for us. I suggested to install into the TARGET directories you mentioned - but of course, installing temporarily, then testing, then removing, then installing properly... makes little sense. Thus my question, whether you could first install - and then run the tests.
Hope that clears it up?
There was a problem hiding this comment.
We somehow need to pass
-DPostgreSQL_EXTENSION_DIRand-DPostgreSQL_PACKAGE_LIBRARY_DIReven thoughpg_configshould return the correct paths. We have a similar failure for pgvector, so this might be a problem on our side. We don't know, yet.
Yeah, this was a problem with my change, that should be fixed in the latest push here. I can build both pgvector and omnigres now, without needing the two cmake flags above.
But the copying of symlinks problem still persists, ofc.
wolfgangwalther
force-pushed
the
postgresql-with-pg-config
branch
from
4aa8b35 to
7490876
Compare
github-actions
bot
added
10.rebuild-darwin: 501+
|
So my intermediate step of passing the dev output on made things worse. But I have the correct fix now: PGXS just must be stored during the build-process, which also makes a lot more sense than having this specific knowledge in the pg_config script. Running nixpkgs-review now, but I think that should be all green without the cmake flags (which was the original goal!). We still need the So right now, this PR improves correctness for everyone, removes one workaround for omnigres and adds another for it. So at least a net positive now. Edit: OK, nixpkgs-review won't work with those rebuilds. But I can rebuild the same packages as before. |
|
Builds look fine, so far, but we are back to omnigres failing on darwin as mentioned in #407920 (comment). I thought I had fixed it temporarily, but that wasn't the case because my "propagate dev output" change essentially just reverted everything I did previously. So still need to investigate why omnigres can suddenly not find symbols on darwin. |
|
The following are my steps taken while debugging this, but I found and pushed a fix. I'll still leave this here for the curious. The PostgreSQL failure for MacOS is exactly the same as in omnigres/omnigres#618. We get this: Before this PR, we get this: The only difference is in the path to I think this is related to this line: I remember having seen this a while ago elsewhere. The problem is, that nixpkgs/pkgs/servers/sql/postgresql/generic.nix Lines 468 to 474 in d935de4 IIUC, the Patching the bundle-loader line to point at the real binary instead of the wrapper fixes it. |
|
The only inconsistency remaining is now the |
|
|
Hmm... odd. My force-push and nixpkgs-ci closing the PR temporarily happened at the same time - and now the PR can't be re-opened anymore. Will open a new PR, targeting staging due to the rebuilds. |
4 participants
Previously,
pg_configwould report the paths of the underlyingpostgresqlderivation and not the paths of thebuildEnvthatpostgresql.withPackagescreates.That's a problem when users of
pg_configuse it to find PostgreSQL'ssharedir, in which they'd like to find the extensions added viawithPackages. Those are only linked into the createdbuildEnv, but not available in thepostgresqlderivation.By providing our own
nix-support/pg_config.envfile, we can swap out those paths. We also do the same for the -man output, because this output is linked intobuildEnvas well. Other paths, which are not available in thebuildEnvenvironment, will still link to the originalpostgresqlderivation. Win-win!This came up in #404027, where it's currently required to pass the following
cmakeFlags:Even though
pg_configis made available, the two paths still need to be specified explicitly, because they point to the wrong location.Note: This is a fix, that should allow omnigres to only require the first line about pg_config. However, it currently still breaks the otherwise succeeding build in that PR with, what I suspect, an upstream bug in their cmake files. I still think we should do this, because it's the correct thing to do.
(review with whitespace disabled, most of that is just indentation)
Things done
./result/bin/)Add a 👍 reaction to pull requests you find important.