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.

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

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

Open
gyuri-szing opened this issue Mar 30, 2021 · 7 comments
Open

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

gyuri-szing opened this issue Mar 30, 2021 · 7 comments

Comments

@gyuri-szing
Copy link

Description

  • Bug
  • Priority: Minor

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(<mbedtls_root>/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
- tests/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
Copy link
Contributor

CJKay commented Mar 31, 2021

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
Copy link
Author

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
Copy link
Contributor

CJKay commented Mar 31, 2021

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
Copy link
Author

"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
Copy link
Contributor

d3zd3z commented Jul 19, 2021

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
Copy link
Author

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
Copy link
Contributor

d3zd3z commented Aug 23, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants