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 -lrapidcheck to pkg-config module #326

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

roberth
Copy link

@roberth roberth commented Jun 25, 2024

This adds the library to rapidcheck.pc, so that it doesn't have to be specified manually in projects that consume it.

The other modules don't need it because they have rapidcheck in their Requires: field.

I've tested this by temporarily patching the rapidcheck Nix package, and it behaves as expected.

@Ericson2314
Copy link

Ericson2314 commented Jun 25, 2024

https://github.com/emil-e/rapidcheck/pull/319/files#r1438905129 oh oops see this thread. I think it's just missing the -L${libdir}?.

CMakeLists.txt Outdated
@@ -128,7 +128,7 @@ set(PKG_CONFIG_DESCRIPTION_SUMMARY "C++ framework for property based testing ins
set(PKG_CONFIG_VERSION)
set(PKG_CONFIG_LIBDIR "\${prefix}/lib")
set(PKG_CONFIG_INCLUDEDIR "\${prefix}/include")
set(PKG_CONFIG_LIBS)
set(PKG_CONFIG_LIBS "-lrapidcheck")
Copy link

@Ericson2314 Ericson2314 Jun 25, 2024

Choose a reason for hiding this comment

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

Suggested change
set(PKG_CONFIG_LIBS "-lrapidcheck")
set(PKG_CONFIG_LIBS "-L\${libdir} -lrapidcheck")

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

It did not appear necessary, and isn't this redundant with set(PKG_CONFIG_LIBDIR "\${prefix}/lib") above?
Also should that line be set(PKG_CONFIG_LIBDIR "\${libdir}") then?

Choose a reason for hiding this comment

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

It did not appear necessary,

I think that is because Nixpkgs adds all the -L flags. (BTW I think we should split out that part of the CC wrapper hook, so packages can opt to relying on proper pkg-configs a bit harder.)

and isn't this redundant with set(PKG_CONFIG_LIBDIR "\${prefix}/lib") above?

No because I think that is for defining libdir=....

Also should that line be set(PKG_CONFIG_LIBDIR "\${libdir}") then?

Then we could get the impredicative

libdir=${libdir}

What we want to end up with is

Libs: -L${libdir} -lrapidcheck

Copy link
Author

Choose a reason for hiding this comment

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

Nixpkgs adds all the -L flags.

Makes sense, so I've applied your suggestion.

No because I think that is for defining libdir=....

Ah, of course. I didn't see through the indirection and I thought this referred to some implicit CMake variables. It doesn't, and that's pretty obvious from the .pc contents.

Thanks!

This adds the library to rapidcheck.pc, so that it doesn't have
to be specified manually in projects that consume it.

The other modules don't need it because they have rapidcheck in
their Requires field.
@roberth roberth mentioned this pull request Jun 25, 2024
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