commschamp / comms_champion

C++(11) library to implement and tools to monitor binary communication protocols
https://commschamp.github.io
Mozilla Public License 2.0
243 stars 16 forks source link

cmake vcpkg changes #11

Closed mathisloge closed 4 years ago

mathisloge commented 4 years ago

These are mainly changes for vcpkg to make the whole qt finding process a bit more confortable. To use this outside of vcpkg, just pass cmake -DQt5_DIR=/path/to/qt/lib/cmake/Qt5

I've added ${CMAKE_INSTALL_PREFIX} to BIN_INSTALL_DIR and PLUGIN_INSTALL_DIR since the ${CMAKE_INSTALL_BINDIR} just includes a relative path and the windeploy might not find that path if it is outside of the current directory. With CMAKE_INSTALL_PREFIX it will be the correct install path

arobenko commented 4 years ago

Sorry Mathis, I must reject this pull request. There are many other CMake projects that use current scheme of integration. Your proposed changes will break many things.

Also maybe you should do support for the COMMS library only (-DCC_COMMS_LIB_ONLY=ON), don't do tools yet. We can always do it in the future as separate package. There is a thought that's been popping up in my head for quite a while now to split the "comms_champion" repository into two separate ones "libcomms" and "tools". There is a chance I'll do it in the future.

mathisloge commented 4 years ago

Hi Alex,

I dislike the idea of having absolute paths (BIN_INSTALL_DIR and PLUGIN_INSTALL_DIR). Why do yo need it absolute?

That was just my point of view. But of course, we can make the paths relative.

Why do you need to change the Qt5 path variable? What's wrong with CC_QT_DIR?

The Qt5_DIR is the official way to set the path for qt. With that in mind all components that you will specify withfind_package(Qt5 COMPONENTS Widgets REQUIRED) would be found. So you don't need to write hardcoded variables like CC_QT_DIR + "/lib/" etc. As a reference for my decision:

I like the idea to seperate the library from the tools

arobenko commented 4 years ago

I like the idea to separate the library from the tools

In such case I suppose no changes to CMake scripts are needed at this stage, right? If this is the case let's close this pull request for now.

mathisloge commented 4 years ago

Yep