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 CMake Glib detection #442

Merged
merged 1 commit into from
Apr 17, 2023
Merged

Fix CMake Glib detection #442

merged 1 commit into from
Apr 17, 2023

Conversation

tprk77
Copy link
Member

@tprk77 tprk77 commented Apr 15, 2023

Restores the original FindGLib2.cmake file with two modifications: First, FOUND_VAR has been removed (it's ignored anyway). Second, I set set(FPHSA_NAME_MISMATCHED TRUE) because find_package_handle_standard_args is called for each component and the mismatched names are expected.

@tprk77 tprk77 requested review from ihilt, kyonifer and nosracd April 15, 2023 19:07
@tprk77 tprk77 self-assigned this Apr 15, 2023
@tprk77 tprk77 mentioned this pull request Apr 15, 2023
Restores the original `FindGLib2.cmake` file with two modifications:
First, `FOUND_VAR` has been removed (it's ignored anyway). Second, I set
`set(FPHSA_NAME_MISMATCHED TRUE)` because
`find_package_handle_standard_args` is called for each component and the
mismatched names are expected.
Copy link
Contributor

@nosracd nosracd left a comment

Choose a reason for hiding this comment

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

I don't really see the value of maintaining a FindGlib2.cmake file over just using pkg-config to find glib, but I'm also not opposed to these changes either.

@tprk77
Copy link
Member Author

tprk77 commented Apr 17, 2023

Yes, to be fair I think it's a minor improvement, mostly that you only need to target_link_libraries and not have to do target_include_directories. I am not really a CMake expert, but I think this is more the "CMake way" of doing it. @mwoehlke-kitware was the person who originally did this work, and he's a CMake expert, so I would defer to him, but he's seemingly not involved with LCM anymore.

@tprk77 tprk77 merged commit 984029b into lcm-proj:master Apr 17, 2023
@tprk77 tprk77 deleted the fix-cmake-glib2 branch April 17, 2023 17:53
nosracd added a commit to nosracd/lcm that referenced this pull request Apr 17, 2023
@nosracd
Copy link
Contributor

nosracd commented Jul 18, 2024

I think it's a minor improvement, mostly that you only need to target_link_libraries and not have to do target_include_directories

I agree that it's nice having less noise in the CMakeLists.txts but I think having those target_include_directories makes it more explicit what headers are being included and could be clearer overall.

I am not really a CMake expert, but I think this is more the "CMake way" of doing it.

I am not a cmake expert either, but it does seem like the winds are shifting towards relying on pkg-config more for finding dependencies rather than custom FindFoo.cmake files. Take, for example, this SO post.

I'm interested in reopening this discussion because it seems that pkg-config is able to find Glib in environments when our FindGlib.cmake is not. In particular, I was running into issues in the cibuildwheel environment on macos-14.

@tprk77 how would you feel about switching to using pkgconfig to find Glib? Alternatively, would you be able to add some additional support to the cmake/FindGLib2.cmake to support additional environments?

Thanks in advance for your input!

@tprk77
Copy link
Member Author

tprk77 commented Jul 19, 2024

I am not a cmake expert either, but it does seem like the winds are shifting towards relying on pkg-config more for finding dependencies rather than custom FindFoo.cmake files. Take, for example, this SO post.

I don't really put much stock in SO, I've seen plenty of misguided or just plain bad answers there. Anyway, I do not think those comments indicate a trend. I would look to the maintainers of CMake to see what they recommend, and for this we have the documentation: https://cmake.org/cmake/help/book/mastering-cmake/chapter/Finding%20Packages.html

You will notice they describe how to write a configuration file. They do not even mention pkg-config.

I'm interested in reopening this discussion because it seems that pkg-config is able to find Glib in environments when our FindGlib.cmake is not. In particular, I was running into issues in the cibuildwheel environment on macos-14.

@tprk77 how would you feel about switching to using pkgconfig to find Glib? Alternatively, would you be able to add some additional support to the cmake/FindGLib2.cmake to support additional environments?

I am not necessary opposed to pkg-config, but I think I should share some history. Many years ago, LCM used autoconf and automake. Those tools work perfectly well, as long as you're on Linux. Not so much for Windows. Those tools sometimes leveraged pkg-config. But pkg-config is not very powerful, it can basically only tell you flags to include headers and link libraries. It's not configurable, you can't specify options to it (besides static/dynamic), it can't tell you anything about other languages, etc. So if you needed something more complicated, you needed to write M4 macros. This was seen as a bad thing. (And I should note, I contributed an improved, mostly re-written ax_lua.m4 to the project when I implemented the Lua binding.)

At some point, someone showed up to modernize the build system. Part of the promise was that the build would now be cross-platform. That is to say, we would have a proper Windows build. Not using MinGW or WSL or whatever, an actual Windows build that would work in Visual Studio. Another benefit was that no one would have to mess around with M4 anymore. This guy did the work, but eventual moved to other things. And it turns out not many people care about Windows (myself included), so the Windows stuff fell into disrepair. But here's the thing: Windows doesn't have pkg-config. So cross-platform CMake doesn't use it, instead it uses CMake package configuration files as described in the CMake documentation.

Anyway, I think you new maintainers have said previously that you are giving up on a proper Windows build, and only a MinGW build is supported. (Which is fine with me, I don't even use Windows.) And in that case, maybe it makes sense to leverage pkg-config, if you know it will always be there on your supported platforms. But I would say using pkg-config is more of a shortcut, and not an idiomatic way of linking packages in CMake.

@nosracd
Copy link
Contributor

nosracd commented Jul 19, 2024

@tprk77 Thanks for the swift response! You make some good points so what do you think about an in-between path? I could modify #520 to leave FindGlib2.cmake alone but still use the variables GLIB_LDFLAGS/GLIB_INCLUDE_DIRS. That way if/when someone is willing to implement and maintain native Windows support all they have to do is set up an if/else to use the custom script on Windows (or whatever environment doesn't have pkg-config) rather than pkg-config.

@nosracd
Copy link
Contributor

nosracd commented Jul 26, 2024

@tprk77 What do you think about 742dbbb as a compromise a bit closer to your position? It preserves the cmake/FindGLib2.cmake and falls back to the pre-existing logic if pkg-config cannot be found or if pkg-config fails to find glib.

@tprk77
Copy link
Member Author

tprk77 commented Jul 27, 2024

@tprk77 What do you think about 742dbbb as a compromise a bit closer to your position? It preserves the cmake/FindGLib2.cmake and falls back to the pre-existing logic if pkg-config cannot be found or if pkg-config fails to find glib.

Yeah, I think this is good! I think some other open source projects do a similar thing.

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