-
Notifications
You must be signed in to change notification settings - Fork 625
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
Create target for installation of libraries and headers into file system #1519
Conversation
@@ -9,6 +9,13 @@ target_include_directories( clock_posix | |||
PRIVATE | |||
${PLATFORM_DIR}/include ) | |||
|
|||
# Install clock abstraction as library. | |||
if(INSTALL_PLATFORM) | |||
install(TARGETS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is the header installation path accounted for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is possible to also install the headers in this file. However, what ends up happening is that the make install
output will no longer group libraries and headers together. To fix that, the headers are installed here instead:
https://github.com/aws/aws-iot-device-sdk-embedded-C/pull/1519/files/96807066b1b35fd543d45f6a57063325cc28dd69#diff-7021d478f66d585e3f9025b7212b1ca40b1db63995592041e28b5dd819c06e53R154-R157
Actually, I did want to also install all libraries in this file but it is not backwards compatible with older versions of CMake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
install all libraries in this file but it is not backwards compatible with older versions of CMake.
What aspect about installing all libraries in the file is not backwards compatible with older version of CMake?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cmake file that calls add_library()
for a target is also where install()
needs to be called for that target in older versions of cmake.
if(${config_prefix} STREQUAL "PKCS") | ||
target_include_directories("${library_name}" PRIVATE | ||
${DEMOS_DIR}/pkcs11/common/include | ||
${LOGGING_INCLUDE_DIRS}) | ||
target_compile_definitions("${library_name}" PUBLIC | ||
-DMBEDTLS_CONFIG_FILE="mbedtls_config.h") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If PKCS11 requires a config file, then the user should be required to pass it instead of the one from our demos being used (imo).
I think it would be important to support selective library installation so that the users that do not want to install PKCS11 and also don't want to provide custom configs for any of the other libraries they want, they don't have to be forced to provide custom config for PKCS11
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine if it is using the configs from the demo as those configs are specific to POSIX. I am supporting selective library installation though.
Upon entering `make install`, the location of each library will be specified first | ||
followed by the location of all installed headers: | ||
``` | ||
-- Installing: /usr/local/lib/libaws_iot_defender.so |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we support building and installing libraries as static archives if user wants? If that increases scope of this PR, it can be done in a follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is already a feature of CMake and can be specified with -DBUILD_SHARED_LIBS=0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test
277971e
to
32fe27b
Compare
if(NOT(${LIB_RT} STREQUAL "LIB_RT-NOTFOUND")) | ||
add_library(ota_posix | ||
${OTA_OS_POSIX_SOURCES}) | ||
target_link_libraries(ota_posix PUBLIC ${LIB_RT} ota_pal) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the ota_pal
library built?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In platform/posix/ota_pal
, and it is an implementation specific to POSIX.
837850f
to
be173cf
Compare
be173cf
to
b9e74e6
Compare
This adds the feature requested in issue #309, #1344, and #1424. It works by creating targets for each individual library and platform abstractions that can then be installed into any directory of a file system.
For instance, the following commands will install only the coreMQTT and coreHTTP libraries in the default system header and library path:
cmake .. -DINSTALL_LIBS="MQTT;HTTP" -DINSTALL_PLATFORM_ABSTRACTIONS=0 -DINSTALL_TO_SYSTEM=1 sudo make install
Once installed, the files can be easily linked and included as a system header:
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.