lsalzman / enet

ENet reliable UDP networking library
MIT License
2.66k stars 665 forks source link

Improving/Updating the CMakeLists.txt #174

Open Spirrwell opened 2 years ago

Spirrwell commented 2 years ago

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:

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 commented 2 years ago

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

Some even since September 2017 #82

lsalzman commented 2 years ago

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 commented 2 years ago

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 commented 11 months ago

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 commented 3 months ago
h3xx commented 2 months ago

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.