pkgsMusl.postgresql: fix build#228349
pkgsMusl.postgresql: fix build#228349Ma27 merged 1 commit intoNixOS:stagingfrom yu-re-ka:musl-postgresql
Conversation
ofborg
bot
added
10.rebuild-darwin: 501+
There was a problem hiding this comment.
Since I don't know about darwin, I gotta ask: how does this behave there?
|
You're completely right, this would change the behavior on darwin in an unwanted way. Also looking at the description of this patch more closely, we can do something better than this in Nix: The issue is the However this is more difficult because on Mac OS the locale binary is not actually available in the nix store, but only in the impure environment in /usr/bin/locale :/ I will try to come up with something. |
|
@Ma27 please review the patch I have pushed |
Ma27
left a comment
Ma27
left a comment
There was a problem hiding this comment.
After re-reading the message of https://git.alpinelinux.org/aports/plain/main/postgresql14/dont-use-locale-a-on-musl.patch?id=56999e6d0265ceff5c5239f85fdd33e146f06cb7, LGTM.
(tl;dr and for reference, if locale doesn't exist, it's fine, but on a musl system, locale from musl-locales may be called which breaks things, hence the absolute path which doesn't exist on musl)
| (fetchpatch { | ||
| url = "https://git.alpinelinux.org/aports/plain/main/postgresql14/icu-collations-hack.patch?id=56999e6d0265ceff5c5239f85fdd33e146f06cb7"; | ||
| hash = "sha256-Yb6lMBDqeVP/BLMyIr5rmR6OkaVzo68cV/+cL2LOe/M="; | ||
| }) | ||
|
|
There was a problem hiding this comment.
@yu-re-ka @Ma27 could one of you explain why the "icu-collations-hack.patch" was introduced here?
This PR was supposed to fix pkgsMusl.postgresql building, because it was failing two tests. For this the other patch disable-test-collate.icu.utf8.patch was added. Understood.
But the lengthy text in icu-collations-hack.patch states:
Since full ICU data is very big (30 MiB), we have created a stripped down
variant with only English locale (package icu-data-en, 2.6 MiB). It also
includes a subset of 18 collations that cover hundreds of languages.When the cluster is initialized or
pg_import_system_collations()is
called directly and only icu-data-en (default) is installed, the user
ends up with only und, en and en_GB ICU-based COLLATIONs. The user can
create missing COLLATIONs manually, but this a) is not expected nor
reasonable behaviour, b) it's not easy to find out for which locales
there's a collation available for.
(emphasis mine)
This is very specific to alpine trying to have a very small icu package. I don't think this applies for NixOS/nixpkgs at all.
I tested removing the icu-collations-hack.patch - all 5 versions still build just fine.
There was a problem hiding this comment.
I think you're right. I was quite afraid of adding some but not all collations-related patches, and getting weird runtime behavior regarding the collations, but there was no logical reason for requiring this patch.
There was a problem hiding this comment.
Thanks for confirming! I added a commit remove it in #293996.
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.
/bin/locale doesn't exist on musl and was already effectively disabled in NixOS#228349. However this still leaves the following warning for initdb: performing post-bootstrap initialization ... sh: locale: not found By applying the alpine patch to disable locale -a entirely, this warning will disappear. This will also make one more regression test pass when testing "check-world" instead of "check", only.
/bin/locale doesn't exist on musl and was already effectively disabled in NixOS#228349. However this still leaves the following warning for initdb: performing post-bootstrap initialization ... sh: locale: not found By applying the alpine patch to disable locale -a entirely, this warning will disappear. This will also make one more regression test pass when testing "check-world" instead of "check", only.
4 participants
Description of changes
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)