gelldur / EventBus

A lightweight and very fast event bus / event framework for C++17
Apache License 2.0
370 stars 81 forks source link

Add CPack support #22

Open Aang23 opened 5 years ago

Aang23 commented 5 years ago

Is your feature request related to a problem? Please describe. On Linux system it's always easier and cleaner to install everything through the package manager, rather than "make install".

Describe the solution you'd like The solution I would consider to be the best, since you're already using CMake, is using CPack to build .rpm & .deb files, using the "make package" command.

Describe alternatives you've considered Alternatives I considered are, evidently, running "make install", or building those package myself. Those ways works, but the first one isn't that clean or recommended, while the second's only issue is not being "official", which would be a shame since it's quite easy to implement.

CMake & CPack documentation

gelldur commented 5 years ago

Thanks I will look into this in near future. Of course PR are welcome :)

Aang23 commented 5 years ago

So expect a PR soon !

gelldur commented 5 years ago

24 pull request is done :) Thx a lot!

Need to improve:

Aang23 commented 5 years ago

You're welcome !

If you've got a custom formatting configuration, yes, publishing it could be quite useful. My main IDE isn't Clion, but I got installed just it case, so that's not an issue.

CPack is bundled with CMake, even on Windows systems, however an option to disable / enable CPack totally, and enabling / disabling specific package types, could be a great plus. Would you prefer that to be done through something like cmake -DCPAK_PACKAGES, or CPACK_PACKAGE_RPM, CPACK_PACKAGE_DEB, etc ? (I already implemented an ENABLE_CPACK option)

I'll add a travis section and split the file too, but where would you like the EventBus_CPack.cmake file to be ? (Maybe lib/cmake)

gelldur commented 5 years ago

My format of cmake isn't so important. As you can see there is already .clang-format so simply I want everything to be autoformated.

Of course you change can be merged in current state as it is ok. I add approve already. I didn't merge it only because lack of time to test it by my own (just for fun) and I would like to add those above improvements so I don't require them from you :) Of course, if you want to improve it, you are welcome :) If you have better ideas, those are also welcome. I'm open to discussion.

Aang23 commented 5 years ago

I formatted EventBus_CPack.cmake through Clion. I kept the package types configuration in the top level CMakeList. I think that will be enough for now, CPack already disables package that requires missing tools to be built. If you want travis to be able to build all currently defined package types, you'll have to install the "rpm" package (at least on ubuntu).

gelldur commented 5 years ago

Thanks! It looks great :) I will try it out & merge on the evening :+1:

gelldur commented 5 years ago

Need to update:

Aang23 commented 5 years ago

Well since I moved the CPack config in the top level cmake file, it takes its config there, I'll move it back to the EventBus (/lib) CMake. I could also check if rpmbuild / etc is present on the system ? If you want to distribute the packages it needs to be built for each distribution, preferably (unless that's very similar ones like Ubuntu / Debian).

gelldur commented 5 years ago

I didn't thought about distribution this small lib but why not to try & learn something new :)

Aang23 commented 5 years ago

I just moved the CPack option back into /lib's CMakeLists, now everything works fine. Here's what would be installed by one of those packages (The built project still appears to be EventBusDev, but it is only since it's the project defined in the top cmake file). By default CPack will only build TGZ / TZ / STGZ, and sometime depending on the distribution, also for the package manager.

/usr/local/include/eventbus
/usr/local/include/eventbus/AsyncEventBus.h
/usr/local/include/eventbus/EventBus.h
/usr/local/include/eventbus/EventCollector.h
/usr/local/include/eventbus/TokenHolder.h
/usr/local/include/eventbus/internal
/usr/local/include/eventbus/internal/AsyncCallbackVector.h
/usr/local/include/eventbus/internal/CallbackVector.h
/usr/local/include/eventbus/internal/TransactionCallbackVector.h
/usr/local/include/eventbus/internal/common.h
/usr/local/lib64/cmake
/usr/local/lib64/cmake/EventBus
/usr/local/lib64/cmake/EventBus/EventBusConfig.cmake
/usr/local/lib64/cmake/EventBus/EventBusConfigVersion.cmake
/usr/local/lib64/cmake/EventBus/EventBusTargets-noconfig.cmake
/usr/local/lib64/cmake/EventBus/EventBusTargets.cmake
/usr/local/lib64/libEventBus.a
Aang23 commented 5 years ago

So, what I ended up doing is adding set(CPACK_GENERATOR "" CACHE STRING "Set packages CPack should build") in the top CMakeLists, and removing any reference to it in the enable_cpack() function & /lib's CmakeLists. Now the package generator can be configured through : cmake .. -DCPACK_GENERATOR="DEB"

That's also what most other projects using CPack uses.

I'll make another pull request for those changes.

gelldur commented 5 years ago

I think issue is generally solved but I will leave it for some time to think about Travis upload or other package distribution. (Maybe add as AUR ?)

Aang23 commented 5 years ago

The only thing I would change is removed the ${CPACK_GENERATOR} argument in enable_cpack(), as passing ${CPACK_GENERATOR} into enable_cpack() is quite useless, since all it used for is setting ${CPACK_GENERATOR}. That would be great if Travis could at least build the package. An AUR could be quite useful, and I was also thinking about a Copr repository. Launchpad may also be an option for APT.

gelldur commented 5 years ago

Yeah you are right enable_cpack should be renamed, but passing what to build to function looks ok for me. Of course I would like add this support to travis but need some time after work to do that :)