Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkgsStatic.postgresql: fix build #345289

Merged
merged 3 commits into from
Dec 1, 2024
Merged

Conversation

wolfgangwalther
Copy link
Contributor

The underlying problem was fixed as a side-effect of 7797728, for reasons unknown to me. In the current state, it's enough to disable a few breaking dependencies to make the build pass.

Note, that this builds the full package, including backend. However, the backend is not working, yet: Loading shared modules, which PostgreSQL heavily depends is still broken. Further, all binaries in the default output, even client binaries such as psql, are currently dynamically linked against libpq.so. While the current autoconf based build system doesn't support changing this, this might be possible in the future with meson.

However, not all is bad: Fixing the build allows using the static libpq.a library, which is probably the one thing that most users want from pkgsStatic.postgresql anyway.

Resolves #191920

Note: I also experimented with a meson-based static build, only to then notice that autoconf works with the basics as well. Making the switch to meson eventually should make things better, though: I was able to build a working backend, too, at least enough to run basic test suites for postgresql drivers with this. We can't fully switch to meson as a build system for 16 or 17, yet, because JIT will not work correctly. The meson build system doesn't build the required bitcode files, yet.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • 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.

Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thank you!

However, this will need to target staging.

@alyssais do you want to take a look? Would wait a few days with merging.

@wolfgangwalther
Copy link
Contributor Author

However, this will need to target staging.

I hope that we don't need any regular rebuilds and only affect the static build, that's why I targeted master. If my latest fix doesn't do that, yet, I will look at the eval diff - could only be some whitespace in the script or so.

@wolfgangwalther wolfgangwalther marked this pull request as draft September 29, 2024 18:26
@wolfgangwalther wolfgangwalther changed the base branch from master to staging September 29, 2024 18:27
@alyssais alyssais added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 29, 2024
@alyssais
Copy link
Member

Would be good to understand what's wrong with the broken dependencies. Are they not compatible with being built statically in some fundamental way? If so, that should be marked in badPlatforms (see e.g. systemd), and then we should use lib.meta.availableOn rather than using an explicit isStatic check, so that platform info for those dependents can have a single source of truth.

@wolfgangwalther
Copy link
Contributor Author

wolfgangwalther commented Sep 29, 2024

Would be good to understand what's wrong with the broken dependencies. Are they not compatible with being built statically in some fundamental way?

They both build fine in pkgsStatic, but PostgreSQL in combination doesn't.

(Edit: I will dig out the error messages and add them as comments or so, let me see)

@wolfgangwalther wolfgangwalther marked this pull request as ready for review September 29, 2024 18:56
@wolfgangwalther
Copy link
Contributor Author

Tried to build with each of (icu, linux-pam) and put the error messages into a comment. I also tried some variations of NIX_LDFLAGS listing the various libs in both package's pkg-config files (of course, both are missing Libs.private and Requires.private ...), but wasn't able to fix it that way.

@wolfgangwalther
Copy link
Contributor Author

However, this will need to target staging.

Done.

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 29, 2024
Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good enough for me now.

But if the server part is inherently broken on static builds, would it make sense to entirely skip the build? Haven't checkout how good this is doable though.

@wolfgangwalther
Copy link
Contributor Author

But if the server part is inherently broken on static builds, would it make sense to entirely skip the build? Haven't checkout how good this is doable though.

I think it would be more invasive to skip the build than I'd want to go at this stage. I just tried, but one thing leads to the next and it will result in quite a few isStatic conditions all over the file.

I'd rather move towards a separate libpq package eventually (see #61580 (comment)). I'd still do the fix as proposed here, with the intention to eventually make the static postgresql backend actually work.

@ofborg ofborg bot requested a review from Ma27 September 29, 2024 20:35
# /nix/store/3s55icpsbc36sgn7sa8q3qq4z6al6rlr-linux-pam-static-x86_64-unknown-linux-musl-1.6.1/lib/libpam.a(pam_audit.o):
# in function `pam_modutil_audit_write':(.text+0x571):
# undefined reference to `audit_close'
++ lib.optionals (stdenv'.hostPlatform.isLinux && !stdenv'.hostPlatform.isStatic) [ "--with-pam" ]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably needs the following in its .pc file (upstream), if you're interested in fixing it:

Requires.private: audit

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given it's autotools, something like this: rockdaboot/libpsl#246

Copy link
Contributor Author

@wolfgangwalther wolfgangwalther Sep 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have a look, but I'm not sure how useful that would be. I just looked and pkgsStatic.linux-pam contains only static libraries. But AFAICT, PAM is built around the idea of loading those authentication modules as shared libraries at run-time, depending on config. You'd probably have to link whatever authentication module you're going to use statically into the binary as well, but I'm not sure whether PAM would actually make use of that and how you'd configure it in that case.

Maybe it's a "linux-pam inherently doesn't make sense in pkgsStatic" as you earlier asked for, despite it being built fine on it's own.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 weeks ago, upstream merged a PR to build with meson. meson generally generates better .pc files. So it might be worth trying that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that should do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A new version of linux-pam was released, which only supports meson. I wasn't able to make that work, yet, so couldn't try whether that would improve this static build. Something for another day.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Work at all or work with postgres?

Copy link
Contributor Author

@wolfgangwalther wolfgangwalther Nov 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't make linux-pam build with meson, so was not able to update linux-pam itself in the first place. I never got to testing with postgres.

Copy link
Contributor Author

@wolfgangwalther wolfgangwalther Nov 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't make linux-pam build with meson, so was not able to update linux-pam itself in the first place.

I can push my intermediate state of that, if you're interested.

Comment on lines 139 to 143
# Building with icu in pkgsStatic gives tons of "undefined reference" errors like this:
# /nix/store/452lkaak37d3mzzn3p9ak7aa3wzhdqaj-icu4c-74.2-x86_64-unknown-linux-musl/lib/libicuuc.a(chariter.ao):
# (.data.rel.ro._ZTIN6icu_7417CharacterIteratorE[_ZTIN6icu_7417CharacterIteratorE]+0x0):
# undefined reference to `vtable for __cxxabiv1::__si_class_type_info'
(lib.withFeature (!stdenv'.hostPlatform.isStatic) "icu")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For icu, there has been a PR merged a week ago which adds some Requires.private and Libs.private to their pkg config files.

So maybe once this is available to us, this will be possible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth backporting now so we remember. (But doesn't have to block this PR even if we do.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tried this... but there is something odd going on. Those .private modifiers are only added for shared linking, not when linking statically. This is kind of the opposite of what we need. I'll try to dig deeper later and possibly raise this upstream.

@wegank wegank added 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package labels Sep 30, 2024
@wolfgangwalther
Copy link
Contributor Author

Rebased, now it's simply a matter of disabling the new feature flags we have. Should be good to go, nothing more we can do here for now.

@wegank wegank removed 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package labels Nov 29, 2024
@Ma27
Copy link
Member

Ma27 commented Nov 30, 2024

The idea was to have a package that builds at least so that you can the libpq.a, correct?
For me, it still fails with

***
ERROR: `xmllint' is missing on your system.
***
make[3]: *** [Makefile:72: postgres-full.xml] Error 1
make[3]: Leaving directory '/build/postgresql-17.2/doc/src/sgml'
make[2]: *** [Makefile:8: all] Error 2
make[2]: Leaving directory '/build/postgresql-17.2/doc/src'
make[1]: *** [Makefile:16: all] Error 2
make[1]: Leaving directory '/build/postgresql-17.2/doc'
make: *** [GNUmakefile:16: world-doc-recurse] Error 2
make: *** Waiting for unfinished jobs....

@wolfgangwalther
Copy link
Contributor Author

The idea was to have a package that builds at least so that you can the libpq.a, correct?

Absolutely 🙈

Will look into it.

The underlying problem was fixed as a side-effect of [1], for reasons
unknown to me. In the current state, it's enough to disable a few
breaking dependencies to make the build pass.

Note, that this builds the full package, including backend. However, the
backend is not working, yet: Loading shared modules, which PostgreSQL
heavily depends is still broken. Further, all binaries in the default
output, even client binaries such as psql, are currently dynamically
linked against libpq.so. While the current autoconf based build system
doesn't support changing this, this might be possible in the future with
meson.

However, not all is bad: Fixing the build allows using the static
libpq.a library, which is probably the one thing that most users want
from pkgsStatic.postgresql anyway.

Resolves NixOS#191920

[1]: 7797728
@wolfgangwalther
Copy link
Contributor Author

For me, it still fails with

Right, so while I opened this PR against staging, I was still testing on master, where v16 was the default. This only breaks for v17, where we added new tools for building the docs:

++ lib.optionals (atLeast "17") [ bison flex perl docbook_xml_dtd_45 docbook-xsl-nons libxslt ];

Seems like we should have technically added a nativeBuildInput of libxml2, too. But since this was in buildInputs already, and we don't build with strictDeps = true - this was not a problem as long as not building cross. Also confirmed that postgresql_17 is failing with strictDeps for the same reason on master.

Thus: I enabled strictDeps and added libxml2 as a nativeBuildInput. This should fix it and prevent regressions.

@Ma27 Ma27 merged commit fc5a956 into NixOS:staging Dec 1, 2024
31 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

static build of postgresql (and thus libpq) fails
4 participants