Add support for GnuTLS #663
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,7 @@ | ||
| CMakeLists.txt.user* | ||
|
|
||
| # Build artifacts | ||
| build/ | ||
|
There was a problem hiding this comment. No, don't add this. Builds should not happen inside the source tree. There was a problem hiding this comment. The repository's ReadMe suggests doing just that. I've seen several other projects follow the same format. What's so wrong with adding the build/ directory to this file? There was a problem hiding this comment. At the very least, if it's going to be added to the repository, it needs to happen in it's own pull request. There are a variety of reasons why builds should not be done inside the source directory, but one specific one that I'll share to help you research the topic, is that cmake natively understands multiple configurations. If you put the build inside the source directory, you pre-clude the ability to have multiple build configurations at the same time. There was a problem hiding this comment. I can remove it. It's just that it was making my life easier as I've always built libraries this way. |
||
|
|
||
| # VS code settings | ||
| .vscode/ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,8 +27,20 @@ IF (POLICY CMP0074) | |
| CMAKE_POLICY(SET CMP0074 NEW) | ||
| ENDIF () | ||
|
|
||
| SET (MQTT_TLS_OPENSSL "MQTT_TLS_OPENSSL") | ||
| SET (MQTT_TLS_GNUTLS "MQTT_TLS_GNUTLS") | ||
|
|
||
| IF (MQTT_USE_TLS) | ||
| MESSAGE (STATUS "TLS enabled") | ||
| IF (MQTT_USE_TLS STREQUAL ${MQTT_TLS_GNUTLS}) | ||
| MESSAGE (STATUS "Using GNUTLS") | ||
| IF (MQTT_BOOST_ASIO_GNUTLS_INCLUDE STREQUAL "") | ||
|
There was a problem hiding this comment. Why namespace the include directory like this? If you need the ability to insert additional include directories, just name it "MQTT_ADDITIONAL_INCLUDE" or something, and take list as the argument. Are you sure CMake doesn't already have a specific feature for this built in? There was a problem hiding this comment. Couldn't find a native Cmake way of doing that. As per this stack overflow post, the other option we have is to let consumers set CXX flags which doesn't seem too appealing. I can change it to be MQTT_ADDITIONAL_INCLUDES and accept a list if that works There was a problem hiding this comment. Personally I don't see the benefit of making something that clearly applies in a generic way named in a feature specific way. I think it'll cause more confusion to people to have it named MQTT_BOOST_ASIO_GNUTLS_INCLUDE. @redboltz can you offer some guidance? There was a problem hiding this comment. I got confused. I don't understand what you want to do, so far. |
||
| MESSAGE (FATAL_ERROR "The MQTT_BOOST_ASIO_GNUTLS_INCLUDE variable needs to be set to the path of the boost-asio-gnutls headers") | ||
| ENDIF () | ||
| ELSE () | ||
| MESSAGE (STATUS "Using OpenSSL") | ||
| SET (MQTT_USE_TLS ${MQTT_TLS_OPENSSL}) | ||
| ENDIF() | ||
| ELSE () | ||
| MESSAGE (STATUS "TLS disabled") | ||
| ENDIF () | ||
|
|
@@ -130,9 +142,13 @@ ELSE () | |
| ENDIF () | ||
|
|
||
| IF (MQTT_USE_TLS) | ||
| IF (MQTT_USE_TLS STREQUAL ${MQTT_TLS_GNUTLS}) | ||
| FIND_PACKAGE (GnuTLS REQUIRED) | ||
| ELSE () | ||
| FIND_PACKAGE (OpenSSL REQUIRED) | ||
| IF (MQTT_USE_STATIC_OPENSSL) | ||
| FIND_PACKAGE (ZLIB REQUIRED) | ||
| ENDIF () | ||
| ENDIF () | ||
| ENDIF () | ||
|
|
||
|
|
||
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.
There's no need to delete this. Github doesn't reuse virtual machines.
That being said, why bother with the copy? I'd rather you just clone the repository directly to the destination. https://stackoverflow.com/questions/600079/how-do-i-clone-a-subdirectory-only-of-a-git-repository
And you should make sure to specify --depth=1, to avoid large downloads.
Alternatively, if somehow doing a direct clone isn't possible, then I'd rather it be a move operation. You can just "mv" the entire "include" folder directly to /usr/local/include/boost-asio-gnutls.
Lastly, why does this even need to be moved to /usr/local ? Can't you just specify the full path ? -DMQTT_BOOST_ASIO_GNUTLS_INCLUDE=$(realpath boost-asio-gnutls/include)
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.
sure, I can do that too.