v1.11.0 Release

[disa6302]

Issue #, if available:

What was changed?

• New release removing addConfigToServerList API that was introduced in 1.10.0.

Why was it changed?

• To avoid enforcing samples to use this API to generate relay candidates.

Additional summary of changes:

Commit Hash	Description
4cd8eed	Edit to CMakeLists to properly build static version of executable and libs
6bc0d2d	1. Update mbedtls from 2.25.0 to 2.28.5 2. Properly INSTALL threadpool libs 3. Remove checkIceAgentStateMachine and checkTurnConnectionStateMachine APIs (which in where the timerQueueKick calls were being made.
4b5ed47	Update mbedtls to 2.28.8 since it is the latest in the 2.28 LTS track
e40fab5	Fixes build for mips32 on old compilers [was necessary for gcc 5.4 cross toolchain with uclibc], being tracked in GitHub lws repo as well but this is the current workaround that works: warmcat/libwebsockets#3163
ad2922a	Propagate the mbedtls found versions to lws cmake (as suggested by lws project itself) necssary when you have a local install as well and that one gets picked up first. Also added cmake command line argument to specify CA CERT PATH (but this didn't work, fixed in next commit below).
38e942c	Properly add -DKVS_CA_CERT_PATH argument for cmake so a custom one can be specified (all embedded devices will need this), tested on Ingenic T31

How was it changed?

• Reverted the commits that introduced this change. Restored unit tests too.

What testing was done for the changes?

• Ran samples under different config to confirm nothing is broken.
• Unit test

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 0% with 155 lines in your changes missing coverage. Please review.

Project coverage is 75.09%. Comparing base (bfa6667) to head (096cafe).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
samples/Common.c	0.00%	155 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2018      +/-   ##
==========================================
+ Coverage   74.81%   75.09%   +0.27%     
==========================================
  Files          46       48       +2     
  Lines       12119    13835    +1716     
==========================================
+ Hits         9067    10389    +1322     
- Misses       3052     3446     +394     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[stefankiesz]

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

[hassanctech]

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.

[stefankiesz]

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.

[stefankiesz]

Reviewed the build changes portion of the PR. Will do WebRTC changes next.

[stefankiesz]

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}

[hassanctech]

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.

[stefankiesz]

All consuming libraries of kvsWebRtcThreadpool (A.K.A. EXTRA_DEPS) already link with kvspicUtils, so seems PRIVATE linking would be more appropriate here since we don not need to expose the consuming libraries to kvspicUtils through kvsWebRtcThreadpool. Is there another reason this change was made?

[hassanctech]

It doesn't work on SoCs without this change for static build.

[cheersalam]

Latest is 3.6.1. Why 2.28.8?

[hassanctech]

We never tested with the latest major version bump, but 2.28.x is an LTS track so I updated to the latest.

[cheersalam]

How was it working, before this addition?

[hassanctech]

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.

[cheersalam]

nit:
maxTurnServer = 1; Should be a macro or const static

[sirknightj]

Note for later: Let's see if we can make this a runtime parameter option as well.

[sirknightj]

Could you share which error we're seeing here?

Can't we also make this one ON only the failing builds (mac), conditional with comment linking to the lws issue in their repo?

[hassanctech]

Added in description, but it's not on mac, this fails on mips gcc older toolchains and I think it's specific with uclibc toolchains (this was required to build in Ingenic T31) with gcc 5.4 uclibc toolchain. Details and link to lws issue has been added to the description.

[sirknightj]

Can we add a check log error for this line?

The value is not being checked below.

[hassanctech]

Agree, we should log this.

[sirknightj]

Could you add a log check error here please?

[sirknightj]

These seem like something we should log. Why else are we having these variables be stored locally?

[stefankiesz]

iceConfigCount is not used.

[stefankiesz]

Not clear why this mutex lock was moved to after the createTurnConnection call.

[unicornss]

Approving the timer-queue changes based on the benchmarking in SoCs

commit/4cd8eed6fa388d6d1d6596e0bd396cd402f08ae0 commit/6bc0d2dc4fd4de585117e3c1c01dbb188f6fbe2e commit/4b5ed4749f4ccc6abdcf07f2c6e7e5808cecd8cd
commit/e40fab5f8c38a34abade9ce6c2c161b60c030fa5
commit/ad2922a492b274845f14366c9af6f4d6db0378d6 commit/38e942c835fba43993859dd857bcecbfa09f26ad

[sirknightj]

Approving the commits between
4cd8eed and
38e942c

[sirknightj]

This PR includes bumping MBED TLS, which currently does not build previously on my Macbook (https://github.com/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/tree/master, commit ec75ba1)

Cloning into 'project_libmbedtls'...
HEAD is now at 1c54b5410 Merge pull request #786 from ARMmbed/dev/yanesca/mbedtls-2.25.0r0-pr
CMake Deprecation Warning at CMakeLists.txt:23 (cmake_minimum_required):
  Compatibility with CMake < 3.5 will be removed from a future version of
  CMake.

  Update the VERSION argument <min> value or use a ...<max> suffix to tell
  CMake that the project does not need compatibility with older versions.


/work/open-source/libmbedtls/build/src/project_libmbedtls/library/bignum.c:1547:29: error: variable 't' set but not used [-Werror,-Wunused-but-set-variable]
    mbedtls_mpi_uint c = 0, t = 0;
                            ^
1 error generated.
gmake[5]: *** [library/CMakeFiles/mbedcrypto.dir/build.make:174: library/CMakeFiles/mbedcrypto.dir/bignum.c.o] Error 1
gmake[4]: *** [CMakeFiles/Makefile2:166: library/CMakeFiles/mbedcrypto.dir/all] Error 2
gmake[3]: *** [Makefile:136: all] Error 2
gmake[2]: *** [CMakeFiles/project_libmbedtls.dir/build.make:87: build/src/project_libmbedtls-stamp/project_libmbedtls-build] Error 2
gmake[1]: *** [CMakeFiles/Makefile2:83: CMakeFiles/project_libmbedtls.dir/all] Error 2
gmake: *** [Makefile:91: all] Error 2
CMake Error at CMake/Utilities.cmake:75 (message):
  CMake step for libmbedtls failed: 2
Call Stack (most recent call first):
  CMakeLists.txt:163 (build_dependency)

After trying out this PR, it builds successfully.

2024-09-20 08:12:18,351 [INFO] [100%] Built target project_libmbedtls

[unicornss]

Release based on the performance tests review.