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

Add CPack support #22

Open
Aang23 opened this issue Apr 9, 2019 · 15 comments
Open

Add CPack support #22

Aang23 opened this issue Apr 9, 2019 · 15 comments

Comments

@Aang23
Copy link
Contributor

Aang23 commented Apr 9, 2019

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
Copy link
Owner

gelldur commented Apr 9, 2019

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

@Aang23
Copy link
Contributor Author

Aang23 commented Apr 9, 2019

So expect a PR soon !

@gelldur
Copy link
Owner

gelldur commented Apr 10, 2019

#24 pull request is done :) Thx a lot!

Need to improve:

  • [Nice to have] Format CMakeLists.txt via Clion (need to publish some autoformater / Clion formater?)
  • [Must have] New line on end
  • [Must have] Update contact & vendor
  • [Must have] Consider what if someone doesn't have CPack (should it be surrounded as option ?)
  • [Nice to have] Maybe split file so CPack could be in e.g. EventBus_CPack.cmake (Is file to long ?)
  • [Nice to have] Add travis section ?

@Aang23
Copy link
Contributor Author

Aang23 commented Apr 10, 2019

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
Copy link
Owner

gelldur commented Apr 11, 2019

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.

  • lib/cmake sounds ok! Thanks
  • ENABLE_CPACK is good enough I think
  • Thanks for the info about CPack (never used intentionally)

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
Copy link
Contributor Author

Aang23 commented Apr 11, 2019

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
Copy link
Owner

gelldur commented Apr 11, 2019

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

@gelldur
Copy link
Owner

gelldur commented Apr 11, 2019

Need to update:

  • EventBusDev to EventBus (different target ?)
  • "RPM;DEB;TGZ" should be passed by variable ? E.g. on my system I don't have rpmbuild
  • Update README about how to do this
  • Add Travis section + upload it somewhere maybe ?
  • Check how other open source projects are dealing with "RPM;DEB;TGZ"

@gelldur gelldur pinned this issue Apr 11, 2019
@Aang23
Copy link
Contributor Author

Aang23 commented Apr 12, 2019

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
Copy link
Owner

gelldur commented Apr 12, 2019

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

@Aang23
Copy link
Contributor Author

Aang23 commented Apr 12, 2019

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
Copy link
Contributor Author

Aang23 commented Apr 12, 2019

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
Copy link
Owner

gelldur commented Apr 21, 2019

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
Copy link
Contributor Author

Aang23 commented Apr 23, 2019

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
Copy link
Owner

gelldur commented Apr 24, 2019

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants