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

Fix includedir/libdir in pkg-config file when CMAKE_INSTALL_INCLUDEDIR/CMAKE_INSTALL_LIBDIR are absolute paths #145

Merged
merged 1 commit into from
Jul 3, 2024

Conversation

abustany
Copy link
Contributor

@abustany abustany commented Jul 2, 2024

This fixes the pkg-config file generation to properly join paths when computing libdir/includedir.

Relevant CMake documentation
More info in the Nix bugtracker

I ran into that issue while adding OpenJPH to nixpkgs. For now this commit is maintained as a patch in Nix.

@aous72
Copy link
Owner

aous72 commented Jul 3, 2024

Hi Adrian,

Thank you for putting this.
I am happy to merge it.
Do I need to worry about breaking other repos installation scripts? I am not super familiar what the current trend is.

Thank you.

Kind regards,
Aous.

@abustany abustany changed the title Fix includedir/libdir in pkg-config file Fix includedir/libdir in pkg-config file when CMAKE_INSTALL_INCLUDEDIR/CMAKE_INSTALL_LIBDIR are absolute paths Jul 3, 2024
@abustany
Copy link
Contributor Author

abustany commented Jul 3, 2024

As far as I understand, this change should not break anything. For the record, here's the diff between the old and the new pc file when running cmake -DCMAKE_INSTALL_PREFIX=/usr:

--- openjph.pc.old	2024-07-03 09:08:23.779830131 +0200
+++ openjph.pc	2024-07-03 09:08:40.028517484 +0200
@@ -3,7 +3,7 @@
 Version: 0.14.0
 Requires: 
 prefix=/usr
-includedir=${prefix}/include
-libdir=${prefix}/lib64
+includedir=/usr/include
+libdir=/usr/lib64
 Libs: -L${libdir} -lopenjph
 Cflags: -I${includedir}

in this simple case, there's no difference besides the fact that the prefix is now "baked" in includedir/libdir.

The old code breaks however if CMAKE_INSTALL_INCLUDEDIR or CMAKE_INSTALL_INCLUDEDIR are absolute paths, because ${prefix} gets prepended unconditionally. The _FULL variables, on the other hand, handle those cases correctly.

Nix is probably one of the few cases where those flags are set to absolute paths, Nix uses that to output the files in different prefixes that correspond to different subpackages (one for binaries, one for development headers).

@aous72 aous72 merged commit 2e37dc3 into aous72:master Jul 3, 2024
2 checks passed
@aous72
Copy link
Owner

aous72 commented Jul 3, 2024

Than you Adrien.

@kmilos
Copy link

kmilos commented Jul 3, 2024

Do I need to worry about breaking other repos installation scripts?

YES!

The _FULL variables, on the other hand, handle those cases correctly.

This doesn't work on systems that need relocatable packages and rely on relative paths, like Windows, multilib/virtual envs for doing cross-compiling etc.

The way to solve this properly and once and for all is e.g.

zeux/pugixml#611

Please release 0.14.2 as soon as possible w/ a proper fix. @aous72

@kmilos
Copy link

kmilos commented Jul 3, 2024

Furthermore, I suggest dropping PKG_CONFIG_LIBS and PKG_CONFIG_CFLAGS, just hard-code them straight in the .pc file until a real need arises.

@aous72
Copy link
Owner

aous72 commented Jul 4, 2024

Hi @kmilos,

Please let me know if Release 0.14.2 works for you.

Also, @abustany, please let me know if Release 0.14.2 works for you.

Kind regards,
Aous.

@kmilos
Copy link

kmilos commented Jul 4, 2024

Looks good to me, thank you on a rapid response!

@abustany
Copy link
Contributor Author

abustany commented Jul 4, 2024

@aous72 it works! And @kmilos sorry for breaking the windows build, I only tested on UNIX...

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.

3 participants