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

Improving/Updating the CMakeLists.txt #174

Open
Spirrwell opened this issue Sep 18, 2021 · 6 comments
Open

Improving/Updating the CMakeLists.txt #174

Spirrwell opened this issue Sep 18, 2021 · 6 comments

Comments

@Spirrwell
Copy link

Spirrwell commented Sep 18, 2021

Hi there.

My primary goal is to make the CMake build a little more vcpkg friendly, so the existing vcpkg port doesn't have to ship its own modified version after the next enet point release, whenever that is.

But the CMakeLists.txt does have a few general issues that if fixed would make it nicer to use/integrate with CMake based projects in general.

I'd be happy to do a pull request for that if that's okay. Here are the changes I would like to make:

  • Bump the minimum CMake version to 3.0.2 (this is the lowest version that I believe supports some of the following changes)
  • Export a config that can be used by find_package()
  • Switch to using target_include_directories using the $<BUILD_INTERFACE:> and $<INSTALL_INTERFACE:> generator expressions so that when "installed" the include directory can be pulled from that through the exported config/find_package()
  • Remove STATIC from the add_library() call so that enet may be built as either a static or a shared library
  • Check against BUILD_SHARED_LIBS and add target_compile_definitions(enet PUBLIC ENET_DLL PRIVATE ENET_BUILDING_LIB), any CMake project that includes enet should automatically get ENET_DLL, but not ENET_BUILDING_LIB
  • Swap the MINGW check with a WIN32 check for linking winmm and ws2_32
  • Add some install() rules
  • Add namespaced enet::enet ALIAS

If this is alright, I can go ahead and do a pull request, if there's no interest, this issue can be closed.

Thank you for your time.

@Croydon
Copy link
Contributor

Croydon commented Sep 18, 2021

Even small CMake improvements did not get any feedback since August 2020 #147 #137

Some even since September 2017 #82

@lsalzman
Copy link
Owner

I'd be willing to revisit the CMake support, but I would like to avoid gigantic gestalt PRs that are all-or-nothing. Let's first decide on the minimum necessary quality-of-life improvements to keep the CMake support functional, and then we can decide about nice-to-haves.

@Spirrwell
Copy link
Author

Ah I should've checked for open pull requests. It looks like #147 does do what might be the minimum for fixing general issues. Though I would prefer the add_definitions(-DENET_DLL=1) it uses be replaced with

target_compile_definitions(enet
    PUBLIC ENET_DLL # Make ENET_DLL public so projects that include us get ENET_DLL
    PRIVATE ENET_BUILDING_LIB # Make ENET_BUILDING_LIB private so that projects that include us don't get it
)

It looks like they also export all symbols for Windows which I'm not sure is necessary, it looks like this is what ENET_BUILDING_LIB is for which I just noticed. This isn't in the CMakeLists either.

Other than that, I guess install() rules and exporting configurations is more on the nice-to-have side.

@ruabmbua
Copy link

Hi, I am the maintainer of the rust bindings: https://github.com/ruabmbua/enet-sys/tree/master

Currently there is an issue, where some cmake installs, particularly on windows have inconsistent output folder structures. This makes it hard to reliably lookup the built static library for linking.

As already mentioned in this thread, a install() rule in the cmake file would be really nice. It would solve my problem, since cmake apparently has a predictable install folder structure, unlike the build folder structure.

@Croydon
Copy link
Contributor

Croydon commented Apr 24, 2024

@h3xx
Copy link

h3xx commented Apr 29, 2024

There's a partial fix for the find_package() compatibility you mentioned at least on *NIX systems.

IIRC find_package() defaults to using pkg-config, and #233 fixes the missing .pc file, at the very least.

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

No branches or pull requests

5 participants