-
-
Notifications
You must be signed in to change notification settings - Fork 229
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: simplified pkgconfig generation #259
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,11 +1,11 @@ | ||||||||||||||||||
prefix=@prefix@ | ||||||||||||||||||
exec_prefix=@exec_prefix@ | ||||||||||||||||||
libdir=@libdir@ | ||||||||||||||||||
includedir=@includedir@ | ||||||||||||||||||
prefix=@CMAKE_INSTALL_PREFIX@ | ||||||||||||||||||
exec_prefix="${prefix}" | ||||||||||||||||||
libdir="${exec_prefix}/lib" | ||||||||||||||||||
includedir="${prefix}/include" | ||||||||||||||||||
Comment on lines
-1
to
+4
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is broken and breaks cmake. (It also breaks autotools which uses the same template, but that's a side point). Why does it break cmake? Good question. The answer is because it completely disrespects
But AFAICT, the previous version of this code already handled the case where these assumptions were true, and also handled what Debian always does ( And it handled this by including the idiomatic logic, where if
raises hand I know best practices for pkg-config, and also best practices for CMake + pkgconfig. :) This does NOT seem to align with what I see in other CMake projects that work. Usually, I and various others PR "fixes" that remove the logic you added, and add the logic you removed. :( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @eli-schwartz thanks for the catch and investigation #272 reverts this as you suggested
Comment on lines
+1
to
+4
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That results in non-idiomatic pkg-config files that don't handle relocation.
|
||||||||||||||||||
|
||||||||||||||||||
Name: @PACKAGE_NAME@ | ||||||||||||||||||
Description: OpenGL Mathematics (glm) for C | ||||||||||||||||||
URL: https://github.com/recp/cglm | ||||||||||||||||||
Version: @PACKAGE_VERSION@ | ||||||||||||||||||
Name: @PROJECT_NAME@ | ||||||||||||||||||
Description: @CMAKE_PROJECT_DESCRIPTION@ | ||||||||||||||||||
URL: @CMAKE_PROJECT_HOMEPAGE_URL@ | ||||||||||||||||||
Version: @PROJECT_VERSION@ | ||||||||||||||||||
Cflags: -I${includedir} | ||||||||||||||||||
Libs: -L${libdir} -lcglm @LIBS@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me how it went wrong in NixOS, unless
IS_ABSOLUTE
in cmake itself is broken? Is NixOS setting a bad value for -DCMAKE_INSTALL_LIBDIR and -DCMAKE_INSTALL_INCLUDEDIR?If these are set to "lib" and "include" respectively, then NixOS should generate pkg-config files with the same logic both before and after this PR.
If these are set to absolute paths, then NixOS should generate absolute paths already.
I'm not even sure how it's possible to end up with:
This would imply that prefix= sees
-DCMAKE_INSTALL_PREFIX=/nix/store/hashed-dir-cglm-0.8.5
but that libdir and includedir see CMAKE_INSTALL_PREFIX with an ending slash, and see-DCMAKE_INSTALL_LIBDIR=nix/store/hashed-dir-cglm-0.8.5/include
without a leading slash? Alternatively,if (IS_ABSOLUTE "/nix/store/hashed-dir-cglm-0.8.5")
reports that that path is NOT an absolute path, which I simply do not understand.@jtojnar is both a Nix person and the author of https://github.com/jtojnar/cmake-snips, which is a cmake function library that implements effectively the same logic as this PR moves away from. The README for the snip basically says "people frequently break Nix by not using this snip" :) so I think that NixOS probably specifically wants to avoid this PR after all?
But maybe @jtojnar can help shed light on why the NixOS package for cglm was getting weird pkg-config files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are the cmake flags for cglm
and yet they fail on 0.8.5 which doesn't have this commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is expanding out to:
Why is cmake claiming IS_ABSOLUTE is not, in fact, absolute, and then following all the "else" branches here? This doesn't make sense -- it directly contradicts CMake's documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the cmake source for FileIsFullPath https://github.com/Kitware/CMake/blob/02599da236fd22db0dcfb6503194e5bad086aea9/Source/kwsys/SystemTools.cxx#L4262-L4293
are the quotes messing with it? also note that the directory does not exist at that point in the build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The quotes really should not matter, that's just cmake's optional syntax for strings.
Directories shouldn't matter either. I'm not using Nix.
Test file:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your investigation!
your test works for me too in nix develop
and the pc in /build contains the correct paths too in nix develop after running
cmakeConfigurePhase
i made this nix reproducer which weirdly works
@alexshpilkin @jtojnar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0.8.5 did not have the
is_absolute
check:https://github.com/recp/cglm/blob/v0.8.5/CMakeLists.txt#L153-L154
So this PR breaks custom directory names and removes what would already work correctly for Nixpkgs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So no one tested cglm to see if it was already fixed. I see.
In fact, the right hand of NixOS reported the same issue in #249 and fixed it in #250 and then the left hand of NixOS reverted that fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed #272
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eli-schwartz Thanks for bringing this up. This has been a while back and I can't really remember why I did this PR, but I clearly wasn't aware of the earlier fixes. I think I went overboard simplifying the cmake configuration. I need to learn more about cmake best practices. It has confused me in another project as well. If you have any sources on how to write proper cmake configuration I'm very interested.
Sorry for the confusion/problems this has caused. The revert seems like the best solution for now.