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.

Sign up for GitHub

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

Jump to bottom

set RPATH on illumos and Linux (#36) #37

Closed
wants to merge 1 commit into from
Closed

set RPATH on illumos and Linux (#36) #37

wants to merge 1 commit into from

Conversation

davepacheco
Copy link

@davepacheco davepacheco commented Sep 3, 2021 •
edited

This PR fixes #36.

This change to build.rs instructs Cargo to pass -Wl,-R/path/to/library to the compiler driver, which causes -R/path/to/library to be passed to the linker at build-time, which causes RPATH in the final binary to contain /path/to/library. This is important when libpq is in a directory that's not part of the runtime linker's default search path. See #36 for an example of what happens without this change when the library is in a different path.

Backwards compatibility

The cargo:rustc-link-arg instruction is stabilized in rust-lang/cargo#9557. This will be in 1.56. Current stable is 1.54.

If you're using a version of Cargo that predates this flag, it will essentially be a no-op. (Technically, it will create a piece of metadata instead, but that should have no impact.)

If you're using current stable, you'll get a warning "warning: cargo:rustc-link-arg requires -Zextra-link-arg flag" and the result will be a noop, just as above.

If you're running on nightly, or on 1.56 once that stabilizes, you get the desired behavior.

The net result I think is that this is no worse for anybody and improves things for people on "nightly" or 1.56 (once that's stabilized).

Platforms

The change only affects illumos and Linux. I expected this problem to happen on MacOS, but it doesn't appear to. I did not dig into why -- I'm not too familiar with Mach-O, but the generated binary had some metadata referring to the correct path.

I tested this by hand on illumos, both on "stable" and "nightly" to verify the behavior above. It works as expected on both: on nightly, everything works. On stable, I get the above warning and the binary is missing the RPATH entry and the test fails.

I used a GitHub Action to test the behavior on MacOS, Windows, and Linux on 1.40 (which predates the Cargo instruction altogether), current stable, and nightly. You can see the results here:
https://github.com/oxidecomputer/pq-sys/runs/3510366648?check_suite_focus=true

On Linux, we see the expected behavior:

  • on 1.40, the build and tests pass, we get no warning, and the resulting binary is lacking the RPATH entry
  • on stable, the build and tests pass, we get the warning, and the resulting binary is lacking the RPATH entry
  • on nightly, the build and tests pass, we get no warning, and the resulting binary has the RPATH entry

On MacOS (which should be unaffected), the build and tests pass on all versions.
On Windows (which should also be unaffected), everything failed because the GitHub environment doesn't have libpq there. But this change really shouldn't affect Windows.

@davepacheco
Copy link
Author

For completeness, I decided to manually test on Linux as well and confirmed that the original problem does exist. This is Amazon Linux 2 on AWS:

$ uname -a
Linux ip-172-31-40-169.us-west-2.compute.internal 4.14.243-185.433.amzn2.x86_64 #1 SMP Mon Aug 9 05:55:52 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
$ cargo version
cargo 1.54.0 (5ae8d74b3 2021-06-22)
$ pg_config --libdir
/home/ec2-user/postgres-install/lib
$ cargo test
   Compiling pq-sys v0.4.6 (/home/ec2-user/pq-sys)
    Finished test [unoptimized + debuginfo] target(s) in 1.44s
     Running unittests (target/debug/deps/pq_sys-aad7df9a5fa1f060)

running 8 tests
test bindgen_test_layout__PQconninfoOption ... ok
test bindgen_test_layout__PQprintOpt ... ok
test bindgen_test_layout___sFILE ... ok
test bindgen_test_layout___sbuf ... ok
test bindgen_test_layout__bindgen_ty_8 ... ok
test bindgen_test_layout__bindgen_ty_8__bindgen_ty_1 ... ok
test bindgen_test_layout_pgNotify ... ok
test bindgen_test_layout_pgresAttDesc ... ok

test result: ok. 8 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/smoke.rs (target/debug/deps/smoke-f27a8dd9e97ba2e4)
/home/ec2-user/pq-sys/target/debug/deps/smoke-f27a8dd9e97ba2e4: error while loading shared libraries: libpq.so.5: cannot open shared object file: No such file or directory
error: test failed, to rerun pass '--test smoke'

and my change fixes it:

$ git checkout 2428548
Note: switching to '2428548'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 2428548 set RPATH on illumos and Linux
$ git log -1
commit 2428548750eeb5f9eedddb8d2583d5e331728322 (HEAD, origin/issue-36)
Author: David Pacheco <dap@oxidecomputer.com>
Date:   Fri Sep 3 15:13:28 2021 -0700

    set RPATH on illumos and Linux
$ cargo +nightly test
   Compiling pq-sys v0.4.6 (/home/ec2-user/pq-sys)
    Finished test [unoptimized + debuginfo] target(s) in 1.47s
     Running unittests (target/debug/deps/pq_sys-89b06ff22c12b611)

running 8 tests
test bindgen_test_layout__PQconninfoOption ... ok
test bindgen_test_layout__PQprintOpt ... ok
test bindgen_test_layout___sFILE ... ok
test bindgen_test_layout__bindgen_ty_8 ... ok
test bindgen_test_layout___sbuf ... ok
test bindgen_test_layout__bindgen_ty_8__bindgen_ty_1 ... ok
test bindgen_test_layout_pgresAttDesc ... ok
test bindgen_test_layout_pgNotify ... ok

test result: ok. 8 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/smoke.rs (target/debug/deps/smoke-0f94be61412f77f2)

running 1 test
test test_ssl_init ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

   Doc-tests pq_sys

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

@davepacheco davepacheco mentioned this pull request Sep 4, 2021
Closed
@davepacheco
Copy link
Author

Update: I wouldn't merge this yet. It's not wrong, but it doesn't fully solve the problem. It allows binaries built in this repo to work, but it doesn't cause binaries in other crates that depend on this crate to work.

@weiznich
Copy link
Collaborator

weiznich commented Sep 4, 2021

I just want to leave a comment here that I recently got the required permissions to maintain this repository. I hadn't any time yet to evaluate the current state of the bindings and look for potential improvements. I would like to do that first before merging anything here, so expect some weeks to pass before anything happens here from my side.

@davepacheco davepacheco mentioned this pull request Sep 10, 2021
Open
@davepacheco
Copy link
Author

Abandoning this for the reasons mentioned in #36.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

build does not set RPATH
2 participants
@davepacheco @weiznich