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

Link Postgres statically #3918

Closed
wants to merge 2 commits into from
Closed

Link Postgres statically #3918

wants to merge 2 commits into from

Conversation

legleux
Copy link
Collaborator

@legleux legleux commented Aug 26, 2021

High Level Overview of Change

Postgres was linked dynamically now it's static.
Had to tack on a few more libraries.

Context of Change

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation Updates
  • Release

@thejohnfreeman
Copy link
Collaborator

What led to this change?

@cjcobb23
Copy link
Contributor

What led to this change?

#3881

Copy link
Contributor

@cjcobb23 cjcobb23 left a comment

Choose a reason for hiding this comment

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

I'll wait for Travis to pass before I approve, but this LGTM. Thanks for cleaning up this file btw.

@thejohnfreeman
Copy link
Collaborator

Curious: is it possible to just install the Postgres shared libraries when they are built as an External Project?

@cjcobb23
Copy link
Contributor

Curious: is it possible to just install the Postgres shared libraries when they are built as an External Project?

Install them system wide? You can just install all of this via the package manager actually.

@thejohnfreeman
Copy link
Collaborator

Hmmm, #3881 wasn't entirely clear. Let me spell out my understanding of the issue:

On a system without Postgres installed, if you build rippled (with CMake), it will download and build Postgres as an External Project. Then if you install rippled (with CMake), it will installed rippled and supporting libraries, but not Postgres, such that if you run rippled from the installed location (/opt/ripple/rippled in the example in #3881), then it will complain that Postgres shared libraries cannot be found.

Is that a fair assessment?

@cjcobb23
Copy link
Contributor

cjcobb23 commented Aug 31, 2021

Hmmm, #3881 wasn't entirely clear. Let me spell out my understanding of the issue:

On a system without Postgres installed, if you build rippled (with CMake), it will download and build Postgres as an External Project. Then if you install rippled (with CMake), it will installed rippled and supporting libraries, but not Postgres, such that if you run rippled from the installed location (/opt/ripple/rippled in the example in #3881), then it will complain that Postgres shared libraries cannot be found.

Is that a fair assessment?

Pretty much. Though I don't know for sure that all supporting libraries are also installed. We link supporting libraries statically by default, in which case they don't need to be installed. In the linked issue, the libraries were being linked statically. Postgres was being built as a shared library always though.

The change has benefits beyond just solving #3881. We would like to link Postgres (and other libs) statically, so that way we can copy the executable around without also having to copy the shared libraries. Also, linking Postgres dynamically when everything else is linked statically (as we were doing) is probably not considered correct behavior.

Copy link
Contributor

@manojsdoshi manojsdoshi left a comment

Choose a reason for hiding this comment

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

LGTM

@manojsdoshi manojsdoshi added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Sep 8, 2021
@manojsdoshi manojsdoshi mentioned this pull request Sep 9, 2021
@nbougalis nbougalis mentioned this pull request Sep 9, 2021
@legleux legleux deleted the pg-static branch September 17, 2021 01:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants