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

Issue using CPM #152

Open
siemjlonn opened this issue Feb 1, 2024 · 5 comments
Open

Issue using CPM #152

siemjlonn opened this issue Feb 1, 2024 · 5 comments

Comments

@siemjlonn
Copy link

So I'm trying to build everest-core using CPM (I know it's recommended to use edm) anyway, I have already added mqttc to my CPM because it is a dependency on this repository(everest-framework), so this conflict will always happen in your find-mqttc.cmake because it will try to add_library(mqttc STATIC IMPORTED) again.

Is kind of a forced error since i have to add mqttc to continue but inside your repository it will conflict because it tries to add again.

@sieukrem
Copy link

We stumbled across same issue by building without of usage of EDM.

Further investigation uncovered that it begins with Line 92 in CMakeLists.txt

find_package(everest-log REQUIRED)
if (EVEREST_ENABLE_ADMIN_PANEL_BACKEND)
find_package(libwebsockets REQUIRED)
endif()
if (BUILD_TESTING)
find_package(Catch2 REQUIRED)
endif()
include(find-mqttc)
endif()

and continuous with confession in find-mqttc.cmake
# FIXME (aw): quite hacky, should check at least if target already exists
add_library(mqttc STATIC IMPORTED)
find_library(MQTTC_LIB_FILE mqttc)
find_path(MQTTC_INCLUDE_DIR mqtt.h)
if(NOT MQTTC_LIB_FILE OR NOT MQTTC_INCLUDE_DIR)
message(FATAL_ERROR "Could not find mqttc library")
endif()
set_target_properties(mqttc PROPERTIES
IMPORTED_LOCATION ${MQTTC_LIB_FILE}
INTERFACE_INCLUDE_DIRECTORIES ${MQTTC_INCLUDE_DIR}
)

I wonder why include is used instead of find_package like few lines above?
The find_package allows to separate the declaration/definition of the target from the expression of dependency to this target and solves in particular the issue we observe here.

Our current solution is to replace the include(find-mqttc.cmake) by find_package(mqttc REQUIRED) and we are done. The declaration of the target mqttc is already accomplished in the overall CMakeLists.txt outside of this repository by using CPM

CPMAddPackage(
  NAME mqttc
  GIT_REPOSITORY https://github.com/LiamBindle/MQTT-C.git
  GIT_TAG v1.1.6
  GIT_PROGRESS TRUE
  OPTIONS
  "MQTT_C_EXAMPLES OFF"
  "CMAKE_POSITION_INDEPENDENT_CODE ON"
)

@corneliusclaussen what is your opinion to, can we proceed with PR!

@hikinggrass
Copy link
Contributor

how about guarding this

include(find-mqttc)

with
if(NOT TARGET mqttc)
and using find_package in the else branch of that? this way we could keep the find-mqttc intact as well

@sieukrem
Copy link

For sure I can try it, but before I do it I would like to know, how can I ensure that the fix does not break existing solution.

  • Did it ever work?

I still have doubts that it would be right solution in comparison to pure find_package which is supporting the principle inversion of control.

@sieukrem
Copy link

@hikinggrass do you know whether the existing solution (the include(find-mqttc)) was working, because this is only executed in case EDM is disabled?

@hikinggrass
Copy link
Contributor

It works in combination with the yocto build with this pretty simple recipe: https://github.com/EVerest/meta-everest/tree/kirkstone/recipes-connectivity/mqttc
The patch there might be relevant

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

No branches or pull requests

3 participants