-
Notifications
You must be signed in to change notification settings - Fork 32
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
set RPATH on illumos and Linux (#36) #37
Conversation
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:
and my change fixes it:
|
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. |
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. |
Abandoning this for the reasons mentioned in #36. |
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 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.