CMake build improvements #441

stefankiesz · 2024-02-28T22:40:55Z

What was changed?
Build improvements including the following:

BUILD_DEPENDENCIES will now only use dependency libraries built from source. They can also be individually specified to use system-installed when BUILD_DEPENDENCIES is on with library-specific flags
Building the samples can be turned off via BUILD_SAMPLES flag
Added comments throughout the CMakeLists file
The Windows CI job now moves the repo into a short directory incase exceeding path length causes any issues in the future
No longer need to run cmake .. twice for M1 CI tests
Replaced "local-openssl" CI jobs with "system-openssl" jobs which now use a BUILD_CRYPTO flag to determine whether to build from source rather than the now removed LOCAL_OPENSSL_BUILD flag
Added Mac OS "system-deps" tests which set BUILD_DEPENDENCIES to FALSE and use only system-installed libraries
- Note: GTest must be built from source when using GCC compiler on Mac because the system's CLANG-compiled GTest is not compatible with GCC-compiled libraries

Why was it changed?
The SDK build can be very inconsistent when we use find_package and pkg_check_modules for project-built-from-source dependencies. Sometimes they find the system-installed libraries instead of the ones this project builds from source, and this has been the cause for some CI build failures. These commands are no longer used when building the dependencies from source along with the project because setting the known include/header and library directories is sufficient - these are both always in our open-source folder that is generated during the build configuration cmake .. stage.

These kind of build issues were also experienced when building locally on Mac, specifically when there was a rouge version of KVS PIC SDK installed in the usr/local/lib default system library directory and when there was already an openssl library installed via HomeBrew.

How was it changed?
This project's CMakeLists.txt and CI jobs were modified as described in the above sections.

What testing was done for the changes?

Built and ran this SDK solo and as dependency for Producer CPP and WebRTC SDKs locally on Intel Mac and M2 Mac, and on a Windows EC2 instance
- otool -L <library>.dylib was used with the local M2 Mac build to verify that Producer C SDK's dependencies and the Producer C SDK itself when built as a dependency for the above SDKs were not linked to duplicate dependency libraries
Built and ran this SDK with BUILD_DEPENDENCIES off on Mac
The CI for this repo, Producer CPP, and WebRTC C were run to verify all tests are passing
Note: Producer CPP SDK has some small changes necessary for this Producer C to work, but WebRTC C SDK is compatible unmodified

Draft PR for Producer CPP tests: awslabs/amazon-kinesis-video-streams-producer-sdk-cpp#1154
Draft PR for WebRTC C tests: awslabs/amazon-kinesis-video-streams-webrtc-sdk-c#1952

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 · 2024-02-29T00:02:40Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.76%. Comparing base (7f9ff95) to head (eb156ac).

❗ Current head eb156ac differs from pull request most recent head 58da8fe. Consider uploading reports for the commit 58da8fe to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #441      +/-   ##
===========================================
- Coverage    63.93%   63.76%   -0.18%     
===========================================
  Files           32       32              
  Lines         5077     5078       +1     
===========================================
- Hits          3246     3238       -8     
- Misses        1831     1840       +9

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

stefankiesz · 2024-03-23T03:51:49Z

.github/build_windows.bat

@@ -1,3 +1,4 @@
+REM  NOTE: May need to replace "Enterprise" with "Community" int the below command


Typo: int -> in

… and shared

This reverts commit 93993b0.

stefankiesz · 2024-04-18T19:20:11Z

Something to consider:

link_directories should generally be avoided in modern CMake projects, as it can lead to unexpected behavior and is less flexible compared to using target_link_libraries with absolute or relative paths.

stefankiesz · 2024-04-19T04:15:23Z

CMakeLists.txt

+  message(${CC_NAME})
+  if(${CC_NAME} MATCHES ".*gcc-.*")
+    build_dependency(gtest)
+  endif()
 endif()

 find_package(PkgConfig REQUIRED)


TODO: PkgConfig probably does not need to be checked for on Windows.

disa6302

Yet to review the rest. But publishing first half.

disa6302 · 2024-04-26T23:47:16Z

CMakeLists.txt

 project(KinesisVideoProducerC VERSION 1.5.1 LANGUAGES C)

 set(KINESIS_VIDEO_PRODUCER_C_MAJOR_VERSION 1)
 set(KINESIS_VIDEO_PRODUCER_C_MINOR_VERSION 5)
 set(KINESIS_VIDEO_PRODUCER_C_PATCH_VERSION 1)
 set(KINESIS_VIDEO_PRODUCER_C_VERSION ${KINESIS_VIDEO_PRODUCER_C_MAJOR_VERSION}.${KINESIS_VIDEO_PRODUCER_C_MINOR_VERSION}.${KINESIS_VIDEO_PRODUCER_C_PATCH_VERSION})

-include(GNUInstallDirs)
+set(SUPPORTED_CRYPTO_LIBRARIES "{OpenSSL, MbedTLS}")


disa6302 · 2024-04-26T23:48:16Z

CMakeLists.txt

 option(USE_OPENSSL "Use openssl as crypto library" ON)
 option(USE_MBEDTLS "Use mbedtls as crypto library" OFF)
-option(BUILD_COMMON_LWS "Whether or not to build ProducerC libwebsockets common library" OFF)
-option(BUILD_COMMON_CURL "Whether or not to build ProducerC curl common library" ON)
+option(BUILD_STATIC "Static build" OFF)


Can we add a CI for this and also add a verification step in the CI to confirm the generated libraries are in fact static? Similar to this:
https://github.com/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/blob/develop/.github/workflows/ci.yml#L801-L805

disa6302 · 2024-04-26T23:48:41Z

CMakeLists.txt

 option(BUILD_OPENSSL_PLATFORM "If buildng OpenSSL what is the target platform" OFF)
-option(LOCAL_OPENSSL_BUILD "Whether or not to use local OpenSSL build" OFF)
+option(BUILD_CRYPTO "Whether to build OpenSSL or MbedTLS, if not, will use system-installed version" ON)


disa6302 · 2024-04-27T00:00:38Z

CMakeLists.txt

  if (NOT OPEN_SRC_INSTALL_PREFIX)
-    set(OPEN_SRC_INSTALL_PREFIX ${CMAKE_CURRENT_SOURCE_DIR}/open-source)
+    set(OPEN_SRC_INSTALL_PREFIX ${CMAKE_SOURCE_DIR}/open-source)


I think it should be ${CMAKE_CURRENT_SOURCE_DIR}? Did we test webrtc with this build to confirm it works and does not break existing behavior if OPEN_SRC_INSTALL_PREFIX is not set?

disa6302 · 2024-04-27T00:02:43Z

CMakeLists.txt

-option(BUILD_COMMON_CURL "Whether or not to build ProducerC curl common library" ON)
+option(BUILD_STATIC "Static build" OFF)
+option(BUILD_DEPENDENCIES "Whether to build dependency libraries from source" ON) # This is a master OFF switch, none will be built if off, but if it is ON, some can be specified to be not built
+option(BUILD_COMMON_LWS "Whether to build ProducerC libwebsockets common library" OFF)


Nice. For future enhancements, let us see if we can mask these behind a BUILD_ONLY overarching flag. Similar to how AWS SDK does: https://github.com/aws/aws-sdk-cpp/blob/main/docs/CMake_Parameters.md#build_only

disa6302 · 2024-04-27T00:04:41Z

CMakeLists.txt

    set(BUILD_ARGS  -DBUILD_STATIC=${BUILD_STATIC}
                    -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
-                    -DOPENSSL_DIR=${OPEN_SRC_INSTALL_PREFIX}
-                    -DUSE_OPENSSL=${USE_OPENSSL}


Why are we removing this?

stefankiesz added 12 commits February 27, 2024 18:42

InitCommit - Update CMakeLists.txt

2a52d46

Don't include LWS KVS source files

f3e3af1

Remove redundant line from libcurl-CMakeLists.txt

b837e0f

Cleanup? libwebsockets-CMakeLists.txt

998ea4e

Working!

cdad372

Add comments, remove duplicated lines

62b31af

Link top level opensoure libs directory

2cd7c83

Switch more dirs from current to top level

394a4a6

Remove one of the lib links

9c6de41

Link curl, ssl, and crypto as private

175ead2

Some extra reference files to be deleted later

dbba69f

Remove curl test code from test main

2ebb24d

stefankiesz added 4 commits February 28, 2024 18:34

Setup choosing between mbedtls and openssl libs

0afd3da

curl->libcurl for windows

73dde64

Add comments

c7d43b0

Trying to fix Windows curl

bb766aa

stefankiesz marked this pull request as ready for review March 8, 2024 23:41

stefankiesz marked this pull request as draft March 8, 2024 23:42

stefankiesz added 11 commits March 8, 2024 15:44

Merge branch 'develop' into cmake-cleanup

65c7c00

Update CMakeLists.txt

c797aa4

Update CMakeLists.txt

55a4d1b

Testing Windows fixes

9127680

more

f032a13

Undo Windows fix that wasn't it

fe16da2

Bring in LWS building, not tested

e6fa97d

Allow for turning off of sample builds

8881061

Build local curl for windows

2fb90e0

Change openssl libs names

275c578

Same for curl

c3be70d

stefankiesz added 3 commits March 22, 2024 16:38

More commenting and cleanup

be81fba

comment

4d27f7a

Cleanup setting of linker paths for Windows

e69543f

stefankiesz commented Mar 23, 2024

View reviewed changes

stefankiesz added 23 commits March 22, 2024 20:56

Remove unlinking of openssl from CI tests

93993b0

Specify openssl build path when from built from source, tested static…

129262a

… and shared

Switch local openssl test to local everything

f952306

Unset DYLD_LIBRARY_PATH

a392dc0

Print DYLD_LIBRARY_PATH

69c3758

Set openssl root directory for curl to find

5dd013b

Trying more

e7b922e

more

18f5bde

more

85f6d8b

more

3946c18

more

1b61699

more

2ba56c9

more

269630c

printing all openssl find_package vars

a5a535f

Switch my curl fork

38424d9

Isolate job, verify unlink openssl works

9b9be56

Try setting dyld lib path

6432c50

Revert "Remove unlinking of openssl from CI tests"

eb20224

This reverts commit 93993b0.

Remove setting of openssldir

3278951

Tested BUILD_DEPS off for common curl

0b27c82

Update CI tests

a9541a1

Install GTest

aa8ea9e

Add tests

58da8fe

stefankiesz commented Apr 19, 2024

View reviewed changes

disa6302 reviewed Apr 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CMake build improvements #441

CMake build improvements #441

stefankiesz commented Feb 28, 2024 •

edited

codecov-commenter commented Feb 29, 2024 •

edited

stefankiesz Mar 23, 2024

stefankiesz commented Apr 18, 2024

stefankiesz Apr 19, 2024 •

edited

disa6302 left a comment

disa6302 Apr 26, 2024

disa6302 Apr 26, 2024

disa6302 Apr 26, 2024

disa6302 Apr 27, 2024

disa6302 Apr 27, 2024

disa6302 Apr 27, 2024

		@@ -1,3 +1,4 @@
		REM NOTE: May need to replace "Enterprise" with "Community" int the below command

CMake build improvements #441

Are you sure you want to change the base?

CMake build improvements #441

Conversation

stefankiesz commented Feb 28, 2024 • edited

codecov-commenter commented Feb 29, 2024 • edited

Codecov Report

stefankiesz Mar 23, 2024

Choose a reason for hiding this comment

stefankiesz commented Apr 18, 2024

stefankiesz Apr 19, 2024 • edited

Choose a reason for hiding this comment

disa6302 left a comment

Choose a reason for hiding this comment

disa6302 Apr 26, 2024

Choose a reason for hiding this comment

disa6302 Apr 26, 2024

Choose a reason for hiding this comment

disa6302 Apr 26, 2024

Choose a reason for hiding this comment

disa6302 Apr 27, 2024

Choose a reason for hiding this comment

disa6302 Apr 27, 2024

Choose a reason for hiding this comment

disa6302 Apr 27, 2024

Choose a reason for hiding this comment

stefankiesz commented Feb 28, 2024 •

edited

codecov-commenter commented Feb 29, 2024 •

edited

stefankiesz Apr 19, 2024 •

edited