postgresql: more cleanup #293996
postgresql: more cleanup #293996
Conversation
github-actions
bot
added
6.topic: nixos
|
For now, just one request: please don't @ me in commit messages (feel free to reference my GH handle or my name, but without @). The reason is that too many people out there have interesting rebase workflows in their forks that cause notifications in my inbox whenever they push something (had this issue a few times already) and it was always pretty annoying. |
Uh, thanks for the hint. Yeah, that's bad. I found one reference to your handle, I removed the at (not pushed, yet). I also changed a |
wolfgangwalther
force-pushed
the
postgresql-cleanup
branch
from
d74ecd8 to
10eb052
Compare
|
Rebased after dropping the "split autotools.nix" commit in the other PR. |
|
Just to make sure it doesn't get lost, from #292993 (comment): would you mind trying out if adding 2da789a to .git-blame-ignore-revs helps to retain a bit of the history of the previous file? No idea if that works out, but maybe worth a try? |
| broken = jitSupport && (stdenv.hostPlatform != stdenv.buildPlatform); | ||
| broken = (jitSupport && stdenv.hostPlatform != stdenv.buildPlatform) | ||
| # Allmost all tests fail FATAL errors for v12 and v13 | ||
| || (jitSupport && stdenv.hostPlatform.isMusl && olderThan "14"); |
There was a problem hiding this comment.
After marking pkgsMusl.postgresql_12_jit and pkgsMusl.postgresql_13_jit as broken here (which they currently are), I realized that those builds were fixed in #294504 / d5ab5216debe155f216e255d28921038fb482325. I don't really know why, though.
wolfgangwalther
force-pushed
the
postgresql-cleanup
branch
from
69dd675 to
4969231
Compare
|
I don't expect the base branch to change much anymore - and even if it does, it would all be non-rebuilding refactors anyway. So I suggest you could already start reviewing the changes in here @Ma27? I think those are in good shape, too. I just added one more commit to deal with the failing regression tests on musl in a better way. The meson based build is working fine now, but I haven't put that up in a PR, yet, because there's already enough of them open, all containing the same many unmerged commits. |
wolfgangwalther
force-pushed
the
postgresql-cleanup
branch
2 times, most recently
from
d3ea5c3 to
80084d2
Compare
|
Added a commit to remove |
|
I think this needs another rebase. |
wolfgangwalther
force-pushed
the
postgresql-cleanup
branch
from
80084d2 to
fe2dc26
Compare
Done. Will rebase again after the merged changes have made their way into |
wolfgangwalther
force-pushed
the
postgresql-cleanup
branch
from
fe2dc26 to
76f2c10
Compare
Done. |
wegank
added
12.approvals: 1
wolfgangwalther
force-pushed
the
postgresql-cleanup
branch
from
c759e1d to
148282a
Compare
|
Just a rebase to resolve a conflict in |
wegank
removed
the
12.approvals: 1
Previously, it was not possible to run tests on an overridden derivation, because
the derivation under test was always pulled from pkgs.
With this change, the following will return the same test:
postgresql_jit.tests
and
(postgresql.override { jitSupport = true; }).tests
This is for consistency between the top-level attribute and the respective package name, which makes it easier to track derivations.
More cosmetic than anything else. The description in findstring.patch was duplicated, postgresql-9.6.1 references confusing. Timestamps are not needed, that's what git blame is for.
Trying to understand what each patch does made me come up with some more descriptive names: - Renaming the two disable-xxx patches to a common names makes it immediately clear that one replaces the other depending on version number. - findstring was really not descriptive at all. - hardcode-pgxs-path will be extended with more paths for split outputs in a later commit. Renaming here already to allow git to better track renames. Finally replacing HARDCODED_PGXS_PATH with $out/lib in the last patch, makes it easier to understand what the end result will look like when reading the patch.
This was introduced in 65fd8f3 around 20 years ago as LANG=en_US, later changed to LC_ALL=en_US and LC_ALL=C to avoid bash warnings. This might have been relevant to run initdb during the check phase, but the PostgreSQL docs [1] say this today: For implementation reasons, setting LC_ALL does not work for this purpose Therefore it can be removed safely. [1]: https://www.postgresql.org/docs/current/regress-run.html#REGRESS-RUN-LOCALE
This seems to have been done better by now. There are no references kept and the build does not fail when removing it.
This was introduced in NixOS#44083 to fix cross building, where xml2-config wouldn't run on the host platform. This was fixed upstream two years later [1], so that from v13 on pkg-config is used before xml2-config is. Once v12 is EOL, we can remove this entirely. [1]: postgres/postgres@0bc8ceb
This was disabled to fix NixOS#51093. The related discussion upstream revealed that this has been fixed in [1] for v12+. Since v12 is our oldest supported version right now, we should be able to safely enable parallel building on darwin again. Furthermore, it seems that this kind of issue was also fixed in NixOS#51221 and NixOS#51408 for the general case. [1]: postgres/postgres@826eff5
Those currently don't build on master, not sure whether they ever built correctly in the first place.
The enableJIT = true case was fixed in NixOS#221851 or e2fb651 respectively. However this did not take the case into consideration, when doing this: services.postgresql = { enable = true; enableJIT = false; package = pkgs.postgresql_15_jit; }; If enableJIT is treated as the source of truth, then this should indeed cause JIT to be disabled, which this commit does.
This was introduced in NixOS#228349, but doesn't seem necessary to build for pkgsMusl. In fact the patch itself seems to be very specific about a stripped down icu package in Alpine Linux, which does not apply to nixpkgs.
We don't have postgresql < 12 in nixpkgs anymore.
This was fixed upstream as a result of this discussion: https://www.postgresql.org/message-id/flat/ef1b47df-0808-4bd7-b08c-5153d5d75f4c%40technowledgy.de
wolfgangwalther
force-pushed
the
postgresql-cleanup
branch
from
148282a to
81201ce
Compare
|
Rebased to fix the previously failing format check (#301268). |
|
Looking good. Would like to run a few tests on my own with it before approving and merging. Btw cc @yu-re-ka would you mind taking a look at the musl-related patches? Nice work! |
We already had a discussion about the icu related patches here: #228349 (comment) |
|
Apologies for the unnecessary ping then! I thought there was discussion around that, but I didn't find it in this thread, sorry. |
|
Thanks a lot for your work and your patience! |
Description of changes
TLDR: This is a continuation of #292993 including those changes which will cause rebuilding the postgresql derivations. Only cleanup, no functional changes.
While working on building PostgreSQL with meson as mentioned in #292993, I had to carefully look at every line in the old builder to see if and how to take this to the meson build. Some of the special cases seem to be not needed anymore, so this PR removes them. Trying to understand the purpose of each of the custom patches, I also cleaned those up along the way with some common formatting and improved naming. Hopefully, this would not only help my own understanding, but other's, too. Finally this PR also contains one more change that is related to the refactor of
thisin the other PR - here, I removethisAttr, too, and make sure that.testswill always work correctly, even when overriding the derivation.This merges into the branch of #292993, so please ignore the first 13 or so commits (all starting with "refactor ...") - those are discussed in the other PR.I still split this into a second PR, because the other changes are a pure refactor without causing rebuilds - while those changes there will cause a full rebuild of all postgresql derivations. Thus, I also targeted staging instead of master with this PR here.Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Maintainers: @thoughtpolice @danbst @globin @marsam @ivan @Ma27
Add a 👍 reaction to pull requests you find important.