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

configure: Use pg_config to locate the library location too #3995

Merged
merged 2 commits into from
Aug 30, 2020

Conversation

whitslack
Copy link
Collaborator

On some systems (e.g., Gentoo), there can be multiple major versions of the PostgreSQL client library installed in parallel. There exists a separate pg_config and libpq.so* for each major version. This Pull Request adds support to C-Lightning's build system to allow the user to choose a specific version of the PostgreSQL client library to build and link against.

Summary of proposed changes:

  • Introduce a POSTGRES_LDLIBS config variable, analogous to the existing POSTGRES_INCLUDE variable, and use it in place of -lpq, both in Makefile and in the Configurator script.
  • When running configure, allow the user to override the name of pg_config by setting the PG_CONFIG environment variable. If unset, the default continues to be pg_config, which preserves the existing behavior.
  • The above changes have the happy side-effect that setting PG_CONFIG to an empty string will now cause Configurator's test for PostgreSQL to fail, so C-Lightning will be built without PostgreSQL support, even if a PostgreSQL client library is present on the build system. (The Gentoo ebuild has been hacking the Configurator test code by inserting an #error directive to force the test to fail when the user has requested that C-Lightning not support PostgreSQL. That hack is no longer necessary with the changes in this PR.)

Also, allow the pg_config binary to be specified through the PG_CONFIG
environment variable, defaulting to 'pg_config' if unset. Explicitly
setting PG_CONFIG to an empty string will forcibly disable PostgreSQL
support, even if a PostgreSQL library is installed.

Changelog-Fixed: build: On systems with multiple installed versions of the PostgreSQL client library, C-Lightning might link against the wrong version or fail to find the library entirely. `./configure` now uses `pg_config` to locate the library.
configure Outdated
if command -v pg_config 2> /dev/null; then
POSTGRES_INCLUDE="-I$(pg_config --includedir)"
POSTGRES_LDLIBS=""
if command -v "${PG_CONFIG-pg_config}" >/dev/null; then
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note, I fixed the redirection here. (command -v prints to stdout, not stderr.)

Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

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

  • The above changes have the happy side-effect that setting PG_CONFIG to an empty string will now cause Configurator's test for PostgreSQL to fail, so C-Lightning will be built without PostgreSQL support, even if a PostgreSQL client library is present on the build system. (The Gentoo ebuild has been hacking the Configurator test code by inserting an #error directive to force the test to fail when the user has requested that C-Lightning not support PostgreSQL. That hack is no longer necessary with the changes in this PR.)

This applies only to users explicitly setting it to an empty string, not the default behavior, correct? I.e., if the user has the libs installed and doesn't customize the PG_CONFIG envvar, then postgresql support will be built in. Just want to make sure that that's still the default behavior :-)

configure Outdated
Comment on lines 234 to 236
if command -v "${PG_CONFIG-pg_config}" >/dev/null; then
POSTGRES_INCLUDE="-I$("${PG_CONFIG-pg_config}" --includedir)"
POSTGRES_LDLIBS="-L$("${PG_CONFIG-pg_config}" --libdir) -lpq"
Copy link
Member

Choose a reason for hiding this comment

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

We could set the $PG_CONFIG at the top of the script to avoid having to use the get-or-default construct here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've already left the computer for the night (going to bed), but I'll make this change within the day.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@whitslack
Copy link
Collaborator Author

This applies only to users explicitly setting it to an empty string, not the default behavior, correct? I.e., if the user has the libs installed and doesn't customize the PG_CONFIG envvar, then postgresql support will be built in. Just want to make sure that that's still the default behavior :-)

You have to explicitly set it to empty if you want to forcibly disable PostgreSQL support even though you have the PostgreSQL library installed. If you leave PG_CONFIG unset, then you'd get the same automagic detection that the existing script implements.

@whitslack
Copy link
Collaborator Author

(Actually you can set PG_CONFIG to any nonexistent path name to disable PostgreSQL support. It just so happens that command -v '' fails too, so empty is a useful value for this purpose.)

@cdecker cdecker self-assigned this Aug 28, 2020
@cdecker
Copy link
Member

cdecker commented Aug 28, 2020

You have to explicitly set it to empty if you want to forcibly disable PostgreSQL support even though you have the PostgreSQL library installed. If you leave PG_CONFIG unset, then you'd get the same automagic detection that the existing script implements.

Excellent, thanks for putting my mind at ease :-)

Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Ack a29632d

@rustyrussell rustyrussell merged commit c1aa33a into ElementsProject:master Aug 30, 2020
@whitslack whitslack deleted the multi-postgresql branch August 30, 2020 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants