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

feature: provide cmake find_package configuration #149

Closed
wants to merge 759 commits into from

Conversation

elfin-sbreuers
Copy link

No description provided.

@redboltz
Copy link
Owner

Thank you for sending the PR.
After I merged #147 , I've checked some other project such as https://github.com/msgpack/msgpack-c/blob/master/CMakeLists.txt

I noticed that CMakeLists.txt in the include directory is unnatural. I think that we can remove it and add install operation to root CMakeLists.txt. Could you update the PR?

In addition,
https://github.com/redboltz/mqtt_cpp/pull/149/files#diff-46193e2e675adcde81f1d095d8b3e1dbR7
doesn't seem good. -lboost_system is UNIX specific notation. Maybe you need to use CMakeVariables something like ${Boost_SYSTEM_LIBRARY}. See https://github.com/msgpack/msgpack-c/blob/master/example/cpp03/CMakeLists.txt#L97

@redboltz
Copy link
Owner

@elfin-sbreuers , I got confused that #147 is your pull request. But it is by @marcel-behlau-elfin.
Sorry for the mix-up.

Anyway, I'd like to remove include/CMakeLists.txt and move the same functionality to ../CMakeLists.txt.

I'm not expert of cmake. If placing include/CMakeLists.txt is better way than implement the functionality in ../CMakeLists.txt, please let me know.

@redboltz
Copy link
Owner

See the comment #147 (comment)

@marcel-behlau-elfin , could you review the PR? I'm not sure how to provide cmake find_package.


target_include_directories(mqtt_cpp_iface INTERFACE ${HDR_ROOT} ${HDR_MQTT})
target_link_libraries(${PROJECT_NAME} INTERFACE ${Boost_SYSTEM_LIBRARIES} -lpthread)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that ${Boost_SYSTEM_LIBRARIES} isn't defined.
According to the document https://cmake.org/cmake/help/v3.8/module/FindBoost.html

Boost_<C>_LIBRARY      - Libraries to link for component <C> (may include
                         target_link_libraries debug/optimized keywords)

So you need to use ${Boost_SYSTEM_LIBRARY}

-lpthread is platform dependent.

See
https://github.com/redboltz/mqtt_cpp/blob/master/CMakeLists.txt#L42
https://github.com/redboltz/mqtt_cpp/blob/master/test/CMakeLists.txt#L36

@redboltz
Copy link
Owner

redboltz commented Jan 8, 2019

I don't know much about cmake find_package mechanism.
Could you tell me how your modification works?
Why target_link_libraries is required? mqtt_cpp is a header only library. Is it for resolve dependency?

@elfin-sbreuers
Copy link
Author

Hello,

Thanks for your valuable input. I updated the CMakeLists.txt according to your recommandations and added a Config.cmake file that allows for a proper usage in modern cmake applications. This includes the necessary link dependencies for threads and boost libraries.

You library is a header only library, but when using the boost system library, it is not header only, the system library needs to be added.

I did not move it up in the directory structure. I guess, one could argue about the right location of the installation procedure. I left it in includes, since the library is 'built' or located there and there should also be defined what is needed to properly build against the library.

Having said that, the inclusion and configuration of boost should probably go into the test/CMakeList.txt.

On a side note: apparently the usage of static boost libraries is only needed for your continuous-integration system. Your tests built just fine without it locally on my machine.

Are the current changes matching your requirements?

@redboltz
Copy link
Owner

Thank you for update the PR. I need a time to study cmake mechanism. Please wait a couple of days(hopefully ;)

README.md Outdated
@@ -66,6 +66,17 @@ If you want to use MQTT on WebSocket, you need to define `MQTT_USE_WS` macro. mq
* WebSocket
* [example/tls_ws_both.cpp](https://github.com/redboltz/mqtt_cpp/blob/master/example/tls_ws_both.cpp)

## Usage in cmake project

Add following lines to your `CMakeList.txt`
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be CmakeLists.txt.

README.md Outdated

```
find_package(mqtt_cpp REQUIRED)
target_link_libraries(${LIBNAME} LINK_PUBLIC mqtt_cpp::mqtt_cpp)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does ${LIBNAME} mean?
If if is a users target name, it should be something like ${YOUR_TARGET}.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested the PR on my environment.
I did make install mqtt_cpp. Then create the new cmake project.
The CMakeLists.txt is

PROJECT (mytest)

find_package(mqtt_cpp REQUIRED)
target_link_libraries(${PROJECT} LINK_PUBLIC mqtt_cpp::mqtt_cpp)

I did

make build
cd build
cmake -DCMAKE_FIND_DEBUG_MODE=1 ..

Then I got the following error:

CMake Error at CMakeLists.txt:4 (target_link_libraries):
  Cannot specify link libraries for target "LINK_PUBLIC" which is not built
  by this project.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the input. I improved the readme. Apparently the target is missing that you add the target_link_libraries to. That means a target generation command like

add_library(${PROJECT_NAME} <dependencies>)

or

add_executable(${PROJECT_NAME} <dependencies>)

is missing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The target_link_libraries call would then also need the following call

target_link_libraries(${PROJECT_NAME} mqtt_cpp::mqtt_cpp)

since the PROJECT variable itself is not set.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still got the error.

I don't understand the following code at readme.

set(LIBNAME "your_project_name")
add_library(${LIBNAME} <your dependencies here>)
find_package(mqtt_cpp REQUIRED)
target_link_libraries(${LIBNAME} LINK_PUBLIC mqtt_cpp::mqtt_cpp)

The user creates library? I think user creates application.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you give me complete and minimal example of CMakeLists.txt that uses installed mqtt_cpp?
I never success cmake, so far.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Create a library that uses mqtt_cpp

PROJECT(my_mqtt)

MESSAGE(STATUS "Configuring ${PROJECT_NAME}")

FILE(GLOB SRC "*.cpp")

ADD_LIBRARY(my_mqtt ${SRC})
ADD_LIBRARY(my::mqtt ALIAS my_mqtt)

TARGET_INCLUDE_DIRECTORIES(my_mqtt PUBLIC ${MY_FRAMEWORKS_ROOT}/inc/)

TARGET_LINK_LIBRARIES(my_mqtt my::core my::log mqtt_cpp_iface Boost::system OpenSSL::SSL nlohmann_json::nlohmann_json )

Create an application that uses the library.

PROJECT(my-application)

MESSAGE(STATUS "Configuring ${PROJECT_NAME}")

FILE(GLOB SRC "*.cpp")

ADD_EXECUTABLE(my-application ${SRC})

TARGET_LINK_LIBRARIES(my-application my::mqtt Boost::program_options)

INSTALL(TARGETS my-application RUNTIME DESTINATION /usr/bin)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What this PR does is make it so someone (e.g., a distribution packager for a linux distribution) can install mqtt_cpp via "make install", and them someone else who wants to use mqtt_cpp, in their CMakeLists.txt, can use the CMake "find_package" command to locate mqtt_cpp on the filesystem and use the code in their own application.

I imagine that this change will also make it easier to satisfy this issue: #77 (but that's just speculation on my part. I know nothing of vcpkg)

@redboltz
Copy link
Owner

Could you rebase the PR from up to date master?

…_server

Added protocol_version setter for servers.
Users needed to set *_ref property types if the property has _ref
type. It was annoying.

Now, user can always use `::recv` type. It's simpler.
Acked-by: Takatoshi Kondo <redboltz@gmail.com>
Added asynchronous pingreq sending mode.
Use asynchronous mode from async_client.
Removed default parameter.
Migrated to azure-devops-build-pipelines from appveyor.
Added dupulicated travis-ci launch avoiding code.
redboltz and others added 20 commits September 12, 2019 22:37
Break the fixed header into dup, qos, and retain values for publish callback
Use v5 reason_code correctly.
Added missing async_handler_t call on disconnect.
Removed error handling on do_sync_write().
If error happens the error will be handled by async_read.
If `handle_error(ec);` is called in `do_sync_write()`, error_handler
would be called twice.
It is consistent behavior as `do_async_write()`.
Removed redundant handler reset code from test_broker.
Added minimal support for Session Expiry Interval.
Timer is not set yet.
…_with_any

Replaced async_handler_t with any.
…tional

Fixes redboltz#391: Don't use mqtt::optional<> for the suback handler, as the standard requires the values be provided
…:shared_ptr<endpoint_t>.

For `set_*_handler()`, capture `std::weak_ptr<endpoint_t>` at
test_broker and examples.

I think that `weak_ptr` is redundant. Reference is good enough because
`shared_ptr` lifetime is kept by `start_session(life_keeper)`.

However, `wp.lock()` is easy to detect if the lifetime is over unexpectedly.
Refactored test_broker parameter passing.
Updated the parameter of set_accept_handler() from endpoint_t to std:…
@codecov-io
Copy link

Codecov Report

Merging #149 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #149   +/-   ##
=======================================
  Coverage   84.94%   84.94%           
=======================================
  Files          40       40           
  Lines        6342     6342           
=======================================
  Hits         5387     5387           
  Misses        955      955

…sible

Check if boost beast is necessary, do reduce required boost version i…
CMakeLists.txt Outdated
ELSE ()
MESSAGE (STATUS "WebSocket disabled")
SET (BOOSTVERSION "1.59.0")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is accurate anymore.

There were bigger changes that cause incompatibilities with versions of boost older than 1.67.

But if this does actually work, that'd be great to know about.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh, you are right. This will be reverted.

…sible

Revert "Check if boost beast is necessary, do reduce required boost v…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants