CMake: Dependency scope

[amitrahman1026]

Suggested enhancement

So recently I've been onboarded on to a codebase that uses mbed-TLS as a submodule, and despite only looking at.

This is a snippet of the dependency tree generated by cmake's graphviz :

I observed a significant number of public executables related to testing, and the sheer amount of public executables confuses me when I was trying to analyse the dependencies of my own codebase.

Upon further investigation, I came across a comment in the top-level CMakeLists.txt file that shed some light on the current approach

#
# The C files in tests/src directory contain test code shared among test suites
# and programs. This shared test code is compiled and linked to test suites and
# programs objects as a set of compiled objects. The compiled objects are NOT
# built into a library that the test suite and program objects would link
# against as they link against the mbedcrypto, mbedx509 and mbedtls libraries.
# The reason is that such library is expected to have mutual dependencies with
# the aforementioned libraries and that there is as of today no portable way of
# handling such dependencies (only toolchain specific solutions).
#
# Thus the below definition of the `mbedtls_test` CMake library of objects
# target. This library of objects is used by tests and programs CMake files
# to define the test executables.

I'm curious to know if there are any plans or ongoing efforts to refactor the test code structure and dependency management to align with modern CMake practices, such as using target_link_libraries and target_include_directories for handling dependencies at the target level.

1. mbedtls_test library: Instead of using target_include_directories to specify the include directories for the mbedtls_test library, you could use target_link_libraries to link against the necessary libraries and inherit their include directories. For example:

target_link_libraries(mbedtls_test
    PRIVATE mbedtls mbedx509 mbedcrypto
)

This would automatically propagate the include directories from the linked libraries to the mbedtls_test target.

2. mbedtls_test_helpers library: Similar to the mbedtls_test library, you could use target_link_libraries to link against the required libraries and inherit their include directories. For example:

   target_link_libraries(mbedtls_test_helpers
   PRIVATE mbedtls mbedx509 mbedcrypto
   )

   This would simplify the include directory management for the mbedtls_test_helpers target.

3. Test and program executables: In the subdirectories where test and program executables are defined (e.g., tests/ and programs/), you could use target_link_libraries to specify the dependencies for each executable target. For example:

   add_executable(my_test_executable ...)
   target_link_libraries(my_test_executable
   PRIVATE mbedtls_test mbedtls mbedx509 mbedcrypto
   )

This would ensure that the necessary libraries and their include directories are properly linked to the test and program executables.

4. Library targets (mbedtls, mbedx509, mbedcrypto): In thelibrary/CMakeLists.txt file, you could use target_include_directories to specify the include directories for each library target. For example:

   target_include_directories(mbedtls
   PUBLIC $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../include>
   )

This would make the include directories available to targets that link against the mbedtls library.

Justification

I understand that the current approach of compiling the shared test code into object files and linking them directly with test executables is a workaround for the mutual dependencies between the test code and the main mbed TLS libraries.

However, I wonder if producing a separate static library for the shared test code would be a cleaner and more maintainable approach. This would allow for better encapsulation, modularity, and ease of linking against the test library from multiple test suites or programs.

I'm aware that there might be challenges or limitations that led to the current implementation, and I apologize if I have an imperfect understanding of the situation.

I would greatly appreciate if you could provide some insights into why creating a separate test library is not feasible or preferred in this case.

[gilles-peskine-arm]

A bit of background on this topic:

• For a very long time, nobody on the Mbed TLS development team knew CMake. Nowadays, a minority of the Mbed TLS development team knows CMake. So a lot of the CMake code is fumble-until-the-tests-pass rather than good practice.
• We tend to think of CMake as mostly for integrating in other projects that only want the library and not the tests. Our primary build system is make.
• Until Mbed TLS 3.0 release in 2021 (and even after, for anything that was backported to a 2.x long-time support branch), we kept compatible with CMake 2.8.12.2. That means we couldn't use modern CMake features.

Responding to a few points, as one of the people who don't know CMake...

compiling the shared test code into object files and linking them directly with test executables is a workaround for the mutual dependencies between the test code and the main mbed TLS libraries.

Is it? I thought it kind of just evolved that way. In the default build, code from library/* does not call any symbols from tests/*, but in some test builds, it does (when we enable callbacks, and provide them via test code). Does that impose some constraint in how cmake handles linking?

producing a separate static library for the shared test code would be a cleaner and more maintainable approach

Maybe. There's no deep reason why we aren't doing that, the current structure just grew. Originally programs/* wasn't linked against code from tests/src, but then we gradually wanted to test more configurations that required callbacks from test code, and use functions from tests/src in some programs that are under programs but intended for testing (programs/ssl/ssl_{client,server}2). We should probably restructure the directories so that all test code is under tests, and anything under programs only needs test code in some special builds that test callbacks.

---

Thanks for the detailed suggestions! I'll leave them for my colleagues who do know CMake.

[paul-elliott-arm]

I will need to dig into this a bit further to really properly understand it, however most things you say here seem sane on first inspection. As @gilles-peskine-arm explains, most of our cmake usage has grown rather organically, and its only quite recently that we have properly started to try and signifcantly improve it (for example using MbedTLS as a submodule properly and respecting the various settings we should be)

In regards to the question of "Is there an ongoing stream of work to refactor the test code structure and dependency management", the answer is unfortunately not at the minute, although this could be raised as a future possiblity, however we have limited resources. The quickest thing here would be to invite you to raise a PR to implement some of the ideas you have here, bearing in mind we also have to still support make, so the static library idea is probably going to be more work than you want to do. This could be raised potentially as a future piece of work, but I'm really not sure on timescales here.

[amitrahman1026]

Hey guys, thank you both for taking the time to respond, I now have a better idea of what went on behind the CMake 😅. I totally get the concerns with the resources and its no quick job. If I manage to squeeze some time from work, I'll definitely take a crack at this, but same on this side, not really sure on the timescale either

[gilles-peskine-arm]

Would this break backward compatibility with existing projects that use our CMake files as a subdirectory or subproject?

[amitrahman1026]

I'm not too familiar on the matter regarding upgrading the CMake build of a subproject, but from what I know with policy ranges set, there should not be a problem with upgrading MbedTLS's CMake usage.

What I have encountered so far with using MbedTLS as a subproject is just some warning like, which can be circumvented with settings the appropriate policies for the required behaviour.

CMake Warning (dev) at submodules/mbedtls/CMakeLists.txt:55 (option):
  Policy CMP0077 is not set: option() honors normal variables.  Run "cmake
  --help-policy CMP0077" for policy details.  Use the cmake_policy command to
  set the policy and suppress this warning.
...

However, considering that the minimum used in this project is cmake_minimum_required(VERSION 3.5.1), as long as it is built with a Cmake version that is sufficient for both, there should not be problems