skypjack / uvw

Header-only, event based, tiny and easy to use libuv wrapper in modern C++ - now available as also shared/static library!
MIT License
1.84k stars 209 forks source link

+ improve CMake-based package config files. #246

Closed moodyhunter closed 2 years ago

moodyhunter commented 3 years ago

Callers can now use find_package(uvw) and link with the uvw::uvw target to use uvw in their projects directly using CMake.

skypjack commented 3 years ago

@stefanofiorentino it looks good to me but could you review it too? I'm far from being a cmake expert, so two extra eyes can help here. 😅

stefanofiorentino commented 3 years ago

I’ll check it later this week.

skypjack commented 3 years ago

Change request: I would spin-off the conan-related commit in an other PR.

I'm fine with the request from @stefanofiorentino I don't have a strong opinion here, so I follow him. 🙂

moodyhunter commented 3 years ago

Hi all, I've just force pushed and removed that conanfile related commit from the git log, for now, it should be clean and only contains a single commit.

stefanofiorentino commented 3 years ago

@skypjack should we put this into an experimental branch and let us time to be tried by someone else?

moodyhunter commented 3 years ago

I agree, I deleted my fork by accident :( , it'll be better if an experimental branch can be created, this also allows more people to test the changes.

skypjack commented 3 years ago

Makes sense. I would update uvw to wrap libuv v1.42 though, then we can create a branch from the last version.

moodyhunter commented 3 years ago

It seems that a dl library is missing, (See https://github.com/moodyhunter/QvPersonal/runs/3201566023?check_suite_focus=true#step:8:273 and https://github.com/skypjack/uvw/blob/master/src/CMakeLists.txt#L80) however the fork is deleted and I can't update this PR.

stefanofiorentino commented 3 years ago

It seems that a dl library is missing, (See https://github.com/moodyhunter/QvPersonal/runs/3201566023?check_suite_focus=true#step:8:273 and https://github.com/skypjack/uvw/blob/master/src/CMakeLists.txt#L80) however the fork is deleted and I can't update this PR.

This dependency comes from libuv (and that's the only one IIRC), as they have API regarding the management of shared libraries. Now, is it uvw responsibility to link under-the-hood to dl or is better to leave it to-do to the final linker? That's a matter of taste and philosophy up-to-me. So I guess @skypjack has to be invoked to the final decision: uvw users would link to dl implicitly or explicitly?

skypjack commented 3 years ago

I'd make users link it explicitly. dl isn't a direct dependency of uvw. Instead, it's a dependency of the underlying library. As an user, I wouldn't feel comfortable with an indirect dependency pulled in by a third party library honestly.

stefanofiorentino commented 3 years ago

I'd make users link it explicitly. dl isn't a direct dependency of uvw. Instead, it's a dependency of the underlying library. As an user, I wouldn't feel comfortable with an indirect dependency pulled in by a third party library honestly.

I like when the linker breaks and I discover hidden dependencies. I feel the developers are playing fair with me. So we agreed.

stefanofiorentino commented 2 years ago

Ciao, I've committed some more modifications to this PR here: experimental. As already stated somewhere, this additions will encapsulate the right version of libuv in the uvw subfolder of system folders during installation. This avoid system folders pollution/overwriting. I guess that's the best we can achieve up to now. Do you agree?

stefanofiorentino commented 2 years ago

@moodyhunter could you please try this and see if it finally solve your issue? https://github.com/skypjack/uvw/tree/experimental Long story short: the installation phase of the dependent libuv is done in the uvw subfolder.

moodyhunter commented 2 years ago

Yeah, it seems to be good, all header-only, shared and static libraries are properly installed now.

skypjack commented 2 years ago

@stefanofiorentino I would merge this upstream. Any objection?

moodyhunter commented 2 years ago

No, I certainly agree.

stefanofiorentino commented 2 years ago

@stefanofiorentino I would merge this upstream. Any objection?

Let's proceed with this. I mean upstream is to me your repo as my origin is my fork. Anyway I think we will get it to master too, right? That's ok to me.