Add CMakeLists.txt for Mbed CLI v2 #35
Conversation
|
Please let me know if there is something wrong with my approach of single CMakeLists.txt file or with the PR at all. I am ready to re-implement this another way if you suggest one. |
| @@ -0,0 +1,37 @@ | |||
| add_library(mbed-mqtt INTERFACE) | |||
There was a problem hiding this comment.
Please add a license header
There was a problem hiding this comment.
Does this need to be an interface? I suspect STATIC would work for this library (no weak linking symbols) ?
There was a problem hiding this comment.
Would this license header be sufficient? No names, just copyright, and SPDX identifier.
# Copyright (c) 2021 ARM Limited. All rights reserved.
# SPDX-License-Identifier: Apache-2.0
There was a problem hiding this comment.
Does this need to be an interface? I suspect STATIC would work for this library (no weak linking symbols) ?
I think you are right. Give me a couple more days and I will figure it out how to make it compile being STATIC. I'm not really competent neither with cmake nor with mbed-os' CMakeLists.txt files organization.
There was a problem hiding this comment.
I've changed my mind. Here is the reason why I think this library should be declared as INTERFACE:
-
When it is declared as
STATIC, it anyways turns out to be built alongside with mbed-os in any project. Therefore it uses all the same macros passed to the compiler on invocation. Consider an example when I havembedtls-config-changes.hfile in the root of my project and I pass it as"MBEDTLS_USER_CONFIG_FILE=\"mbedtls-config-changes.h\""inmbed_app.json. ThisMBEDTLS_USER_CONFIG_FILEmacro is passed to mbed-mqtt sources too when it is being built and this config header file cannot be found, because mbed-mqtt is being built separately as a static library and it is not aware of my project's root directory where the file is located. To mitigate this error I have to declare something like this in my project's rootCMakeLists.txt:target_include_directories(mbed-mqtt PRIVATE .). When mbed-mqtt declared asINTERFACEthere is no such problems. -
When it is declared as
STATIC, all mbed-mqtt dependencies are built separately from the project and number of files to be compiled becomes almost twice as many than if it was declared asINTERFACE- 868 files whenINTERFACEvs 1717 whenSTATICin my case. Basically the whole OS is being built twice. This is inconvenient. -
I have no evidence of any library for mbed-os declared as
STATIC- they all are declared asINTERFACE. Why mbed-mqtt should not be declared asINTERFACE?
README.md
Outdated
| ``` | ||
| add_subdirectory(mbed-mqtt) | ||
|
|
||
| target_link_libraries(${APP_TARGET} mbed-mqtt) |
There was a problem hiding this comment.
group them together to one target_link_libraries()
There was a problem hiding this comment.
Done. I've declared all dependencies explicitly, so this list has shrunk to just single entry mbed-mqtt.
README.md
Outdated
| target_link_libraries(${APP_TARGET} mbed-mqtt) | ||
| target_link_libraries(${APP_TARGET} mbed-mbedtls) | ||
| target_link_libraries(${APP_TARGET} mbed-nanostack) | ||
| target_link_libraries(${APP_TARGET} mbed-greentea) |
There was a problem hiding this comment.
is greentea required? I dont think so (testing with greentea needs work to be enable with CMake)
There was a problem hiding this comment.
Technically it is required for now, because otherwise it won't compile. But if PR #34 would be merged, then it will no longer be required and mbed-mqtt will be compileable without greentea dependency.
README.md
Outdated
|
|
||
| target_link_libraries(${APP_TARGET} mbed-mqtt) | ||
| target_link_libraries(${APP_TARGET} mbed-mbedtls) | ||
| target_link_libraries(${APP_TARGET} mbed-nanostack) |
There was a problem hiding this comment.
why nanostack is required , shouldn't this be up to a consumer of this library (mqtt) to choose a stack (netsockets/nanostack, etc) ?
There was a problem hiding this comment.
Well, it is up to a consumer here, since it is just a suggestion in README.md for a user. It provides an example of how you link all dependencies while linking against mbed-mqtt library.
It now fails to compile, though, when I began tinkering with STATIC declaration you've mentioned above: TCPSocket.h: No such file or directory. I need some time to figure out how properly declare dependency on a network stack.
Anyway, thank you very much for the review so far!
There was a problem hiding this comment.
It now has mbed-netsocket explicitly declared as it's dependency.
No description provided.