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.

Sign up for GitHub

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

v1.11.0 Release #2018

Merged
merged 22 commits into from
Oct 4, 2024
Merged

v1.11.0 Release #2018

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
1eaede8
Release 1.10.0 (#1910)
disa6302 Mar 12, 2024
f107343
Viewer fix (#1953)
disa6302 Mar 20, 2024
4f1d3ad
Release 1.10.2 (#1981)
niyatim23 May 9, 2024
b54d180
addIceServerConfig revert
disa6302 Jun 24, 2024
c93c47c
CI
disa6302 Jun 24, 2024
323d91d
UT fixes, unused var
disa6302 Jun 24, 2024
936f5ec
one more unused
disa6302 Jun 24, 2024
b2162c6
version update
disa6302 Jun 25, 2024
4cd8eed
port static build fix prodc
hassanctech Aug 12, 2024
6bc0d2d
build fixes, remove call to timer queue kick
hassanctech Aug 27, 2024
4b5ed47
update mbedtls to latest LTS in 2.28 track
hassanctech Aug 28, 2024
e40fab5
Update libwebsockets-CMakeLists.txt
hassanctech Aug 29, 2024
ad2922a
fix mac build mbedtls, add option for ca cert
hassanctech Aug 30, 2024
38e942c
need to add definition so it can be used downstream even if it was sp…
hassanctech Aug 30, 2024
3652e86
Default ENABLE_KVS_THREADPOOL to OFF
stefankiesz Sep 17, 2024
df7f38b
Update README.md to reflect threadpool changes
stefankiesz Sep 17, 2024
096cafe
Add cmake flag for iot creds (#2056)
sirknightj Sep 18, 2024
e22e08b
Prepare for 1.11.0 release.
stefankiesz Sep 26, 2024
d9ab52d
Add instructions for using IoT credentials in the samples and documen…
sirknightj Oct 2, 2024
d5d09c8
Change branch target from master to main (#2059)
sirknightj Oct 3, 2024
77e2c47
Merge branch 'main' into 1.10.3
hassanctech Oct 3, 2024
86aed1d
Update libkvsCommonLws-CMakeLists.txt to new release
stefankiesz Oct 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion CMake/Dependencies/libkvsCommonLws-CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,12 @@ include(ExternalProject)

ExternalProject_Add(libkvsCommonLws-download
GIT_REPOSITORY https://github.com/awslabs/amazon-kinesis-video-streams-producer-c.git
GIT_TAG v1.5.2
GIT_TAG link-mbedtls-properly
stefankiesz marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

target_link_libraries(kvsCommonLws
+           ${PRODUCER_CRYPTO_LIBRARY}
            ${OPENSSL_CRYPTO_LIBRARY}
            ${OPENSSL_SSL_LIBRARY}
            ${LIBWEBSOCKETS_LIBRARIES}

Looking at the cmake 1-line change in this Producer C branch, its seems we should only link the ${OPENSSL_CRYPTO_LIBRARY} ${OPENSSL_SSL_LIBRARY} libraries if not opting for MbedTLS - or better yet only have the PRODUCER_CRYPTO_LIBRARY one which should cover both cases:

set(PRODUCER_CRYPTO_LIBRARY
        OpenSSL::Crypto
        OpenSSL::SSL)
if (USE_MBEDTLS)
    set(CPRODUCER_COMMON_TLS_OPTION KVS_USE_MBEDTLS)
    set(PRODUCER_CRYPTO_LIBRARY
            MbedTLS
            MbedCrypto)
endif()

So I propose the following change:

target_link_libraries(kvsCommonLws
            ${PRODUCER_CRYPTO_LIBRARY}
-           ${OPENSSL_CRYPTO_LIBRARY}
-           ${OPENSSL_SSL_LIBRARY}
            ${LIBWEBSOCKETS_LIBRARIES}

And with a nit variable name change that makes more sense as there are multiple libraries and to be more aligned with how we also named LIBWEBSOCKETS_LIBRARIES:

target_link_libraries(kvsCommonLws
-           ${PRODUCER_CRYPTO_LIBRARY}
+           ${PRODUCER_CRYPTO_LIBRARIES}
            ${LIBWEBSOCKETS_LIBRARIES}

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I thought of this as well, I have no idea why it was set up the way it is, however the openssl cmake variables will be blank in this case so it will be ignored by the target link libraries (I verified this). Preference though would be to do as you suggested as the final change.

PREFIX ${CMAKE_CURRENT_BINARY_DIR}/build
LIST_SEPARATOR |
CMAKE_ARGS
-DCMAKE_INSTALL_PREFIX=${OPEN_SRC_INSTALL_PREFIX}
-DCMAKE_PREFIX_PATH=${OPEN_SRC_INSTALL_PREFIX}
-DBUILD_COMMON_LWS=ON
-DBUILD_COMMON_CURL=OFF
-DBUILD_DEPENDENCIES=FALSE
Expand Down
2 changes: 2 additions & 0 deletions CMake/Dependencies/libwebsockets-CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ ExternalProject_Add(project_libwebsockets
-DLWS_WITH_STATIC=1
-DLWS_WITH_SHARED=${LWS_WITH_SHARED}
-DLWS_WITH_MBEDTLS=${LWS_WITH_MBEDTLS}
-DLWS_MBEDTLS_LIBRARIES=${LWS_MBEDTLS_LIBRARIES}
-DLWS_MBEDTLS_INCLUDE_DIRS=${LWS_MBEDTLS_INCLUDE_DIRS}
-DLWS_WITH_MINIMAL_EXAMPLES=1
-DLWS_HAVE_PTHREAD_H=1
-DLWS_WITH_THREADPOOL=${LWS_WITH_THREADPOOL}
Expand Down
19 changes: 18 additions & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,11 @@ message(STATUS "Kinesis Video WebRTC Client path is ${KINESIS_VIDEO_WEBRTC_CLIEN
message(STATUS "dependencies install path is ${OPEN_SRC_INSTALL_PREFIX}")

# pass ca cert location to sdk
add_definitions(-DKVS_CA_CERT_PATH="${CMAKE_SOURCE_DIR}/certs/cert.pem")

if(NOT DEFINED KVS_CA_CERT_PATH)
Copy link

Choose a reason for hiding this comment

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

How was it working, before this addition?

Copy link
Contributor

Choose a reason for hiding this comment

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

Customers could specify an environment variable as well, or they would make the change in the CMakeLists themselves manually, or when you're running from the sample place it was built, the path will be right.

add_definitions(-DKVS_CA_CERT_PATH="${CMAKE_SOURCE_DIR}/certs/cert.pem")
endif()

add_definitions(-DCMAKE_DETECTED_CACERT_PATH)

if (ENABLE_KVS_THREADPOOL)
Expand Down Expand Up @@ -163,6 +167,17 @@ if(BUILD_DEPENDENCIES)
build_dependency(mbedtls ${BUILD_ARGS})
endif()

# This step is necessary because the next set of dependencies have a dependency on
# mbedtls/openssl and the find_package command populates certain cmake variables which
# are needed to pass to the cmake commands for the next set of dependencies
if (USE_OPENSSL)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be able to remove the existing find_package calls on the crypto libs that start down on line 262 right?

Copy link
Contributor

Choose a reason for hiding this comment

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

No because this is in the if build dependencies section, so this needs to run in both cases and since it's idempotent I kept it there.

Copy link
Contributor

@stefankiesz stefankiesz Sep 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sounds good, might want to consider breaking up the build_deps case into two blocks to handle the before find_package logic and the after find_package logic.

find_package(OpenSSL REQUIRED)
set(OPEN_SRC_INCLUDE_DIRS ${OPEN_SRC_INCLUDE_DIRS} ${OPENSSL_INCLUDE_DIR})
else()
find_package(MbedTLS REQUIRED)
set(OPEN_SRC_INCLUDE_DIRS ${OPEN_SRC_INCLUDE_DIRS} ${MBEDTLS_INCLUDE_DIRS})
string(REPLACE ";" "|" MBEDTLS_LIBRARIES_ALT_SEP "${MBEDTLS_LIBRARIES}")
endif()

if(WIN32)
set(OPENSSL_INCLUDE_DIRS "${OPEN_SRC_INSTALL_PREFIX}/include/")
Expand All @@ -188,6 +203,8 @@ if(BUILD_DEPENDENCIES)
-DLWS_OPENSSL_CRYPTO_LIBRARY=${OPENSSL_CRYPTO_LIBRARY}
-DLWS_OPENSSL_SSL_LIBRARY=${OPENSSL_SSL_LIBRARY}
-DLWS_OPENSSL_INCLUDE_DIRS=${OPENSSL_INCLUDE_DIR}
-DLWS_MBEDTLS_INCLUDE_DIRS=${MBEDTLS_INCLUDE_DIRS}
-DLWS_MBEDTLS_LIBRARIES=${MBEDTLS_LIBRARIES_ALT_SEP}
-DCMAKE_C_FLAGS=${CMAKE_C_FLAGS})
endif()
build_dependency(websockets ${BUILD_ARGS})
Expand Down
Loading