CMake: remove MBED_PATH and use <PROJECT-NAME>_SOURCE_DIR/MODULE_PATH instead #14516

0xc0170 · 2021-04-08T12:58:04Z

Summary of changes

This PR is helping UNITTESTS so it should be merged prior #14426 - they need to use files within Mbed OS and already defines project().

We used MBED_PATH previously, it's own defined variable for Mbed OS project root. Usage were to reference CMake scripts or files out of it's current source tree.

The approach should be to use cmake module path for listfiles like .cmake to find them. Note, due to our target labels, some CMake files are hidden so include must be also protected if it touches conditionally added library (for instance TFM).

For CMakeLists.txt files, use mbed-os_SOURCE_DIR - this is useful for CMakeLists.txt. For listfiles like .cmake files, please use CMAKE_CURRENT_LIST_DIR (as they are included).

Impact of changes

Migration actions required

Documentation

Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

This enables multiple variables we can use. One of them is source dir for a project. It will replace MBED_PATH in the tree.

ciarmcom · 2021-04-08T13:00:22Z

@0xc0170, thank you for your changes.
@ARMmbed/mbed-os-maintainers please review.

0xc0170 · 2021-04-08T13:02:39Z

@rwalton-arm @rajkan01 Please review

0xc0170 · 2021-04-08T13:04:54Z

To do once approved:

I'll check docs if we reference MBED_PATH there
clean up examples, remove MBED_PATH there as well as it's not required anymore

rajkan01 · 2021-04-08T14:23:32Z

@0xc0170 Instead of going with mbed-os_SOURCE_DIR, we can make use of CMake PROJECT_NAME and CMAKE_PROJECT_NAME variable like for example ${PROJECT_NAME}_SOURCE_DIR what is your thoughts

0xc0170 · 2021-04-08T14:57:08Z

@0xc0170 Instead of going with mbed-os_SOURCE_DIR, we can make use of CMake PROJECT_NAME and CMAKE_PROJECT_NAME variable like for example ${PROJECT_NAME}_SOURCE_DIR what is your thoughts

The current way it is to explicitly use mbed-os project name as a reference rather than the last/current project name. Wouldn't this be a problem when Mbed OS is taken as a component or we have multiple projects in the tree ?

rajkan01 · 2021-04-08T15:18:19Z

connectivity/FEATURE_BLE/source/cordio/TESTS/cordio_hci/driver/CMakeLists.txt

@@ -3,10 +3,10 @@

 cmake_minimum_required(VERSION 3.19.0 FATAL_ERROR)

-set(MBED_PATH ${CMAKE_CURRENT_SOURCE_DIR}/../../../../../../.. CACHE INTERNAL "")
+set(mbed-os_SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/../../../../../../.. CACHE INTERNAL "")


As a current implementation, every greentea test has its CMakeLists.txt similar to the application CMake. Why do we need to change into mbed-os_SOURCE_DIR? this will be a problem when we do "Separate GreenTea from Mbed"

I see, this is setting it. I missed it during refactoring, I'll remove all

rajkan01 · 2021-04-08T15:25:30Z

@0xc0170 Instead of going with mbed-os_SOURCE_DIR, we can make use of CMake PROJECT_NAME and CMAKE_PROJECT_NAME variable like for example ${PROJECT_NAME}_SOURCE_DIR what is your thoughts

The current way it is to explicitly use mbed-os project name as a reference rather than the last/current project name. Wouldn't this be a problem when Mbed OS is taken as a component or we have multiple projects in the tree ?

In the case of mbed os is taken as a component or multiple projects in the tree, how the way the CMake file gets included decides the PROJECT_NAME but mbed-os root CMake retains PROJECT_NAME as a mbed-os, but it will be a problem

rajkan01 · 2021-04-08T15:44:42Z

connectivity/FEATURE_BLE/source/cordio/TESTS/cordio_hci/driver/CMakeLists.txt

 set(TEST_TARGET mbed-connectivity-ble-cordio-hci-driver)

-include(${MBED_PATH}/tools/cmake/mbed_greentea.cmake)
+include(${mbed-os_SOURCE_DIR}/tools/cmake/mbed_greentea.cmake)


Sorry I missed to comment here, Do we need to change this as well because if it is top-level CMake similar to application one then PROJECT_NAME and CMAKE_PROJECT_NAME will be mbed-connectivity-ble-cordio-hci-driver and the mbed-os_SOURCE_DIR variable is not yet created

The test should not care where it is.

How we create greentea shall be changed and this MBED_PATH should be fixed there. I'll create a separate issue for greentea tests, how they use MBED_PATH and get mbed-os in. I'll revert the changes here.

I'll keep this to fix the Mbed OS files (excluding greentea tests).

Created #14523 and I'll revert now greentea tests changes.

0xc0170 · 2021-04-08T16:04:57Z

Updated, greentea tests removed from this PR. They need different fix unfortunately.

rajkan01 · 2021-04-08T18:12:15Z

tools/cmake/mbed_greentea.cmake

@@ -3,7 +3,7 @@

 set(MBED_CONFIG_PATH ${CMAKE_CURRENT_BINARY_DIR} CACHE INTERNAL "")

-include(${MBED_PATH}/tools/cmake/app.cmake)
+include(${mbed-os_SOURCE_DIR}/tools/cmake/app.cmake)


Can we fix this as part of #14523? and revert this change

rajkan01

LGTM

rwalton-arm · 2021-04-09T10:18:25Z

targets/TARGET_ARM_SSG/TARGET_MUSCA_B1/CMakeLists.txt

@@ -1,7 +1,7 @@
 # Copyright (c) 2020-2021 ARM Limited. All rights reserved.
 # SPDX-License-Identifier: Apache-2.0

-include(${MBED_PATH}/platform/FEATURE_EXPERIMENTAL_API/FEATURE_PSA/TARGET_TFM/TARGET_TFM_LATEST/scripts/mbed_set_post_build_tfm.cmake)
+include(${mbed-os_SOURCE_DIR}/platform/FEATURE_EXPERIMENTAL_API/FEATURE_PSA/TARGET_TFM/TARGET_TFM_LATEST/scripts/mbed_set_post_build_tfm.cmake)


I'm wondering if we can use CMAKE_MODULE_PATH to avoid hard coding all of the include paths. We could set the module directories in mbed-os/CMakeLists and then include modules with e.g include(mbed_set_post_build_tfm) in other files. The benefit of this approach, in my view, is if we ever move the location of these files we will have fewer changes to make.

+1, I'll check how we can create the modules for our scripts

rwalton-arm · 2021-04-09T10:21:55Z

...ERIMENTAL_API/FEATURE_PSA/TARGET_TFM/TARGET_TFM_LATEST/scripts/mbed_set_post_build_tfm.cmake

@@ -17,7 +17,7 @@ function(mbed_post_build_tfm_sign_image
    set(mbed_target_name ${mbed_target})
    set(post_build_command
        COMMAND ${Python3_EXECUTABLE}
-            ${MBED_PATH}/platform/FEATURE_EXPERIMENTAL_API/FEATURE_PSA/TARGET_TFM/TARGET_TFM_LATEST/scripts/generate_mbed_image.py
+            ${mbed-os_SOURCE_DIR}/platform/FEATURE_EXPERIMENTAL_API/FEATURE_PSA/TARGET_TFM/TARGET_TFM_LATEST/scripts/generate_mbed_image.py


Can this not be reduced to ${CMAKE_CURRENT_LIST_DIR}/generate_mbed_image.py?

we are in the targets area, not in the tools so current list dir points to targets/ somewhere.

Anyway, this path is incorrect. I'll fix it.

rwalton-arm · 2021-04-09T10:23:26Z

tools/cmake/app.cmake

@@ -12,11 +12,11 @@ include(${MBED_CONFIG_PATH}/mbed_config.cmake)
 # Load toolchain file
 if(NOT CMAKE_TOOLCHAIN_FILE OR MBED_TOOLCHAIN_FILE_USED)
    set(MBED_TOOLCHAIN_FILE_USED TRUE CACHE INTERNAL "")
-    include(${MBED_PATH}/tools/cmake/toolchain.cmake)
+    include(${mbed-os_SOURCE_DIR}/tools/cmake/toolchain.cmake)


${CMAKE_CURRENT_LIST_DIR}/toolchain.cmake?

yes, fixed in the later commit.

rwalton-arm · 2021-04-09T10:23:41Z

tools/cmake/app.cmake

 endif()

 # Specify available build profiles and add options for the selected build profile
-include(${MBED_PATH}/tools/cmake/profile.cmake)
+include(${mbed-os_SOURCE_DIR}/tools/cmake/profile.cmake)


${CMAKE_CURRENT_LIST_DIR}/profile.cmake?

yes, fixed in the later commit.

rwalton-arm · 2021-04-09T10:24:14Z

tools/cmake/mbed_greentea.cmake

@@ -3,7 +3,7 @@

 set(MBED_CONFIG_PATH ${CMAKE_CURRENT_BINARY_DIR} CACHE INTERNAL "")

-include(${MBED_PATH}/tools/cmake/app.cmake)
+include(${mbed-os_SOURCE_DIR}/tools/cmake/app.cmake)


${CMAKE_CURRENT_LIST_DIR}/app.cmake?

yes, fixed in the later commit. I realized we should use list dir in tools

rwalton-arm · 2021-04-09T10:25:01Z

tools/cmake/profile.cmake

@@ -29,4 +29,4 @@ else()
    endif()
 endif()

-include(${MBED_PATH}/tools/cmake/profiles/${LOWERCASE_CMAKE_BUILD_TYPE}.cmake)
+include(${mbed-os_SOURCE_DIR}/tools/cmake/profiles/${LOWERCASE_CMAKE_BUILD_TYPE}.cmake)


${CMAKE_CURRENT_LIST_DIR}/profiles/${LOWERCASE_CMAKE_BUILD_TYPE}.cmake?

rwalton-arm · 2021-04-09T10:26:18Z

tools/cmake/app.cmake

@@ -12,11 +12,11 @@ include(${MBED_CONFIG_PATH}/mbed_config.cmake)
 # Load toolchain file
 if(NOT CMAKE_TOOLCHAIN_FILE OR MBED_TOOLCHAIN_FILE_USED)
    set(MBED_TOOLCHAIN_FILE_USED TRUE CACHE INTERNAL "")
-    include(${mbed-os_SOURCE_DIR}/tools/cmake/toolchain.cmake)
+    include(${CMAKE_CURRENT_LIST_DIR}/toolchain.cmake)


OK, I see you changed these in a later commit. It would be easier to review if we didn't make changes to already reviewed commits later on in the PR.

I do not want to push force while it's being reviewed but I can clean it up.

It would be easier to review if we didn't make changes to already reviewed commits later on in the PR.

I do not follow. I fixed earlier commit in the subsequent one. how else can I make review easier without rebasing?

I usually would force push, but marking them as fixup commits would also work for me.

+1 for force push but that is hard for some reviewers so I can mark as fixu and later rebase to get them out of history.

mbed-ci · 2021-04-13T15:09:06Z

Jenkins CI Test : ❌ FAILED

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM	❌
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM	❌
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️

0xc0170 · 2021-04-13T15:16:49Z

Errors are legit, I'll fix them

- list files included via module path - <project-name>_SOURCE_DIR for sources that are out of the current processed CMake - CMAKE_CURRENT_LIST_DIR for listfiles

As we still use target labels, TFM is not visible to CMake. Protect include of their scripts with the same mechanism.

Excluding greentea tests, they will be fixed separately (more work needed).

All files within tools/cmake can now include within tools/cmake. We do not expose internal folders like core/profiles/toolchains. They should be included via toolchain file (mbed_toolchain in our case).

CMAKE_CURRENT_LIST_DIR behaves differently in functions. We store it in the CMakeLists itself, so anyone calling a function would get the actual list dir where the scripts are. To illustrate: if I call a function from src/CMakelists.txt, function located in src/scripts, `CMAKE_CURRENT_LIST_DIR` in the function would point to the src/ folder but not to src/scripts.

To avoid conflicts as we expose our CMake list files via CMAKE_MODULE_PATH. Some files already include it, this aligns the rest of files. I leave app.cmake as it is - it's user facing and it would be breaking change. We can clean this one for the next major version.

Update CMAKE_MODULE_PATH at once place. Note, we update also CMAKE_MODULE_PATH in app.cmake. This is temporary until we get a proper way to include Mbed Os (removing app.cmake need to be included by an application).

0xc0170 · 2021-04-14T13:17:33Z

I fixed an issue with TFM. I can't still build MUSCA though - a path in python script is incorrect. :/ I'll find out what has changed.

0xc0170 · 2021-04-14T18:19:18Z

CI started, I would like to see if it can reproduce my testing locally and if so will compare the logs.

mbed-ci · 2021-04-14T21:10:07Z

Jenkins CI Test : ✔️ SUCCESS

Build Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-test	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️
jenkins-ci/mbed-os-ci_tfm-integration	✔️

mbed-ci · 2021-04-14T21:46:53Z

Jenkins CI Test : ✔️ SUCCESS

Build Number: 3 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️
jenkins-ci/mbed-os-ci_cmake-example-test	✔️
jenkins-ci/mbed-os-ci_tfm-integration	✔️

0xc0170 · 2021-04-15T12:00:16Z

@rajkan01 I'll merge this now, you can update unittests to remove using MBED_PATH completely.

CMake: use project name for Mbed OS

1413c42

This enables multiple variables we can use. One of them is source dir for a project. It will replace MBED_PATH in the tree.

ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Apr 8, 2021

ciarmcom requested a review from a team April 8, 2021 13:00

ciarmcom added needs: review devices: arm component: core component: tools component: connectivity component: hal devices: cypress component: security devices: nxp labels Apr 8, 2021

0xc0170 requested review from rwalton-arm and rajkan01 April 8, 2021 13:02

rajkan01 reviewed Apr 8, 2021

View reviewed changes

0xc0170 force-pushed the fix-cmake-project-name branch 2 times, most recently from 2e4ce1c to 25c5de3 Compare April 8, 2021 16:03

rajkan01 reviewed Apr 8, 2021

View reviewed changes

rajkan01 previously approved these changes Apr 9, 2021

View reviewed changes

mergify bot added needs: CI and removed needs: review labels Apr 9, 2021

rwalton-arm reviewed Apr 9, 2021

View reviewed changes

mergify bot added needs: work and removed needs: review labels Apr 13, 2021

0xc0170 force-pushed the fix-cmake-project-name branch from b16a3b1 to 3abf9d5 Compare April 14, 2021 12:06

0xc0170 added 7 commits April 14, 2021 13:09

CMake targets: remove MBED_PATH, use stadard variables

dfc4010

- list files included via module path - <project-name>_SOURCE_DIR for sources that are out of the current processed CMake - CMAKE_CURRENT_LIST_DIR for listfiles

CMake platform: remove MBED_PATH, use standard variables

afd9384

As we still use target labels, TFM is not visible to CMake. Protect include of their scripts with the same mechanism.

CMake tools: use CMAKE_CURRENT_LIST_DIR instead of MBED_PATH

b88d568

Excluding greentea tests, they will be fixed separately (more work needed).

CMake app: add our cmake folder to CMAKE_MODULE_PATH

ed609f5

All files within tools/cmake can now include within tools/cmake. We do not expose internal folders like core/profiles/toolchains. They should be included via toolchain file (mbed_toolchain in our case).

CMake: set CMAKE_MODULE_PATH in the Mbed OS CMakelists.txt

121c828

Update CMAKE_MODULE_PATH at once place. Note, we update also CMAKE_MODULE_PATH in app.cmake. This is temporary until we get a proper way to include Mbed Os (removing app.cmake need to be included by an application).

0xc0170 force-pushed the fix-cmake-project-name branch from 3abf9d5 to 121c828 Compare April 14, 2021 12:10

0xc0170 added needs: review and removed needs: work labels Apr 15, 2021

rwalton-arm approved these changes Apr 15, 2021

View reviewed changes

0xc0170 added ready for merge and removed needs: review labels Apr 15, 2021

0xc0170 merged commit 1fd4cfd into ARMmbed:master Apr 15, 2021

0xc0170 deleted the fix-cmake-project-name branch April 15, 2021 12:00

mergify bot removed the ready for merge label Apr 15, 2021

mbedmain added release-version: 6.10.0 Release-pending labels Apr 16, 2021

mbedmain removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Apr 26, 2021

CMake: remove MBED_PATH and use <PROJECT-NAME>_SOURCE_DIR/MODULE_PATH instead #14516

CMake: remove MBED_PATH and use <PROJECT-NAME>_SOURCE_DIR/MODULE_PATH instead #14516

Uh oh!

Conversation

0xc0170 commented Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

ciarmcom commented Apr 8, 2021

Uh oh!

0xc0170 commented Apr 8, 2021

Uh oh!

0xc0170 commented Apr 8, 2021

Uh oh!

rajkan01 commented Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rajkan01 Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajkan01 commented Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Apr 8, 2021

Uh oh!

rajkan01 Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajkan01 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 Apr 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Apr 8, 2021 •

edited

Loading

rajkan01 commented Apr 8, 2021 •

edited

Loading

0xc0170 commented Apr 8, 2021 •

edited

Loading

rajkan01 Apr 8, 2021 •

edited

Loading

rajkan01 commented Apr 8, 2021 •

edited

Loading

rajkan01 Apr 8, 2021 •

edited

Loading

0xc0170 Apr 9, 2021 •

edited

Loading

0xc0170 Apr 9, 2021 •

edited

Loading