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

Add support for setting LIBDIR via env #35

Merged
merged 1 commit into from
Mar 24, 2023

Conversation

ferdnyc
Copy link
Contributor

@ferdnyc ferdnyc commented Mar 24, 2023

Hardcoding the libdir as ${prefix}/lib isn't correct for systems that keep 64-bit libraries in ${prefix}/lib64.

Allow setting a variable LIBDIR in the environment, to define both the pkg-config libdir and the path passed to cdylib_link_lines().

LIBDIR may be an absolute path or one based on ${prefix}, which will be substituted before creating the PathBuf for cdylib_link_lines.

(Note that the libdir argument to cdylib_link_lines is actually only used on macos, so this won't actually have any effect on the linker args emitted on linux. Nevertheless, for the sake of completeness custom LIBDIR values based on ${prefix} are supported.)

@nwalfield
Copy link
Collaborator

This looks great, thanks! Can you please document this in the build instructions in the README.md. Thanks!

@nwalfield
Copy link
Collaborator

Great. Since this is one atomic change (in my mind, the feature and its documentation belong together), can you please squash the commits. Thanks.

Hardcoding the libdir as `${prefix}/lib` isn't correct for systems
that keep 64-bit libraries in `${prefix}/lib64`.

Allow setting a variable `LIBDIR` in the environment, to define both
the pkg-config libdir and the path passed to cdylib_link_lines().

`LIBDIR` may be an absolute path or one based on `${prefix}`, which
will be substituted before creating the PathBuf for cdylib_link_lines.
@ferdnyc
Copy link
Contributor Author

ferdnyc commented Mar 24, 2023

@nwalfield Done.

Tangentially...

There's also the Requires.private in the pkg-config metadata, which is currently hardcoded to nettle. I'm about to submit a PR to the Fedora rust-rpm-sequoia package with both this patch, and one to just hard-replace nettle with openssl for their builds... but it'd be cool if there was some easy way to autoconfigure that from build.rs as well.

...I'm also wondering, given that RPM is the main consumer for this, whether it wouldn't make sense to generate a CMake configuration (rpm-sequoia-config.cmake and rpm-sequoia-targets.cmake files) in the build output dir? CMake actually supports linking library targets with no SONAME (you just have to set the IMPORTED_NO_SONAME property TRUE on the IMPORTED SHARED target), so the symlink wouldn't even be necessary to make linking from the builddir work. You'd just do a cmake -DCMAKE_PREFIX_PATH=/path/to/rpm-sequoia/targets/<profile>/ when configuring CMake, which would use find_package(rpm-sequoia) instead of pkg_check_modules() to import it.

@nwalfield
Copy link
Collaborator

@nwalfield Done.

Tangentially...

There's also the Requires.private in the pkg-config metadata, which is currently hardcoded to nettle. I'm about to submit a PR to the Fedora rust-rpm-sequoia package with both this patch, and one to just hard-replace nettle with openssl for their builds... but it'd be cool if there was some easy way to autoconfigure that from build.rs as well.

...I'm also wondering, given that RPM is the main consumer for this, whether it wouldn't make sense to generate a CMake configuration (rpm-sequoia-config.cmake and rpm-sequoia-targets.cmake files) in the build output dir? CMake actually supports linking library targets with no SONAME (you just have to set the IMPORTED_NO_SONAME property TRUE on the IMPORTED SHARED target), so the symlink wouldn't even be necessary to make linking from the builddir work. You'd just do a cmake -DCMAKE_PREFIX_PATH=/path/to/rpm-sequoia/targets/<profile>/ when configuring CMake, which would use find_package(rpm-sequoia) instead of pkg_check_modules() to import it.

I've moved this issue to here: #36

@nwalfield nwalfield merged commit 194d18b into rpm-software-management:main Mar 24, 2023
@nwalfield
Copy link
Collaborator

Thanks for working on this!

@nwalfield nwalfield mentioned this pull request Mar 24, 2023
@ferdnyc ferdnyc deleted the fix-pkgconfig branch March 24, 2023 10:13
@ferdnyc
Copy link
Contributor Author

ferdnyc commented Mar 24, 2023

I'm about to submit a PR to the Fedora rust-rpm-sequoia package with both this patch, and one to just hard-replace nettle with openssl for their builds...

That PR, for the record: https://src.fedoraproject.org/rpms/rust-rpm-sequoia/pull-request/1

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.

2 participants