Mbed-TLS / mbedtls

An open source, portable, easy to use, readable and flexible TLS library, and reference implementation of the PSA Cryptography API. Releases are on a varying cadence, typically around 3 - 6 months between releases.
https://www.trustedfirmware.org/projects/mbed-tls/
Other
5.2k stars 2.54k forks source link

CMake: usage of CMAKE_SOURCE_DIR in CMakeLists.txt files. #4268

Open gyuri-szing opened 3 years ago

gyuri-szing commented 3 years ago

Description


The following CMakeLists.txt files [1] use CMAKE_SOURCE_DIR to set MBEDTLS_DIR which may have unwanted side-effects when these "projects" are used with add_subdirectory(). This is the case i.e. if the CMakeLists.txt file of an external project uses add_subdirectory(/libarary) to only build the library. In such case MBEDTLS_DIR will pint to the location of the top-level cmake file of the external project.

1: - library/CMakeLists.txt

There are at least the following possible workarounds:

  1. Use CMAKE_CURRENT_SOURCE_DIR and the assumption mbedtls directory structure is standard, and the location of MBEDTLS_DIR is known relative to the CMakeLists.txt file in "libarary" or in "tests".
  2. Issue an error if MBEDTLS_DIR is not set, and require the parent project to set it correctly.

(Note: using a CACHE variable would allow the same behavior which is implemented currently with the conditional block around setting the variable.)

mbed TLS build:
Version: tip of development

CJKay commented 3 years ago

Just a quick note on this as I was passing by, but CMake defines PROJECT_SOURCE_DIR for getting the source directory of the current project, and <PROJECT-NAME>_SOURCE_DIR for getting the source directory of arbitrary projects. You could replace all instances of MBEDTLS_DIR with PROJECT_SOURCE_DIR, and you would no longer have to maintain MBEDTLS_DIR.

gyuri-szing commented 3 years ago

That depends on where MBEDTLS_DIR is expected to point to, root directory of mbedlts repo, or to the directory where the last project() command was called from cmake. Note: if this is the top-level cmake file, there is no explicit project() command and cmake will act like there would be one. See bottom of this page.

CJKay commented 3 years ago

Are these subdirectories intended to be used directly? Seems a bit unusual to try to do that as, yes, CMake will complain about the lack of cmake_minimum_required() and project() directives. I might suggest doing away with trying to use them as independent build systems and as top-level subprojects simultaneously - if it needs to know where the top-level project is, it's not really independent to begin with and nothing's really gained from trying to build it in isolation.

gyuri-szing commented 3 years ago

"Are these subdirectories intended to be used directly?" Nothing says sub-projects can/shall not be built standalone. (Curse of cmake no making a difference in file names of these.)

"CMake will complain about the lack of cmake_minimum_required() and project() directives" Depends on cmake version.

"it's not really independent to begin with" Yes, but you can clone the repo as a whole only, so depending on the details the build might still work fine.

"I might suggest doing away with trying to use them as independent build systems and as top-level subprojects simultaneously - .... and nothing's really gained from trying to build it in isolation." Building from this directory might be a way to avoid the compiler specific global settings in the top-level CMakeLists.txt being executed. Also if I only need the libraries why not? On the other hand ExternalProject() is a better way of integration for this project as it is.

Anyways, let's leave it to the owners to decide if standalone build is needed or not, or how they wish to re-structure cmake scripts.

d3zd3z commented 3 years ago

There is definitely some inconsistency with the CMakeFiles (e.g. library defines the projects within the directory, but doesn't work by itself), whereas other libraries, e.g. mbedtls_test, are defined in the top-level CMakeFile).

I don't really see a strong benefit to being able to include just a single subdirectory from another project. Doing this would require some significant restructuring of our cmake scripts.

I think we should focus on any problems caused when another project tries to use the top-level cmake script.

I'd also suggest defining a value in this top-level script that we check for in the sub scripts, and if it isn't defined, generate a clearer error. The current behavior about a bunch of undefined symbols is not helpful.

gyuri-szing commented 3 years ago

There is definitely some inconsistency with the CMakeFiles (e.g. library defines the projects within the directory, but doesn't work by itself), whereas other libraries, e.g. mbedtls_test, are defined in the top-level CMakeFile).

I don't really see a strong benefit to being able to include just a single subdirectory from another project. Doing this would require some significant restructuring of our cmake scripts.

I think we should focus on any problems caused when another project tries to use the top-level cmake script.

I'd also suggest defining a value in this top-level script that we check for in the sub scripts, and if it isn't defined, generate a clearer error. The current behavior about a bunch of undefined symbols is not helpful.

Unfortunately I can not remember the details, but we were building the library directory only to avoid some issues due to "improper" cmake files. I.e. the top level file sets global C flags based on some in-house compiler identification. If one has to avoid that, a workaround can be to use the library directory. Of course the proper solution would be to fix the top-level cmake file. My point is: if the top level cmake file is written properly, there might be no real use-case to build subdirectories.

d3zd3z commented 3 years ago

if the top level cmake file is written properly, there might be no real use-case to build subdirectories.

I'd like to pursue this approach. Currently, Zephyr uses its own CMake file when using Mbed TLS, because of these issues with globals and such. Making the top-level cmake file usable as a sub directory, and perhaps even making it so that it detects and flags an error when an external project tries to use sub cmake files directly.