CMake: remove need for APP_TARGET #14265

0xc0170 · 2021-02-10T14:05:45Z

Summary of changes

The recent changes added a new requirement to app/tests - APP_TARGET to be defined. This PR is addressing this issue. It is not completely getting rid of the application dependency (see commit messages for details). I'll create separate issue to fix Gcc Arm memory map and also how to preprocess linker scripts. Both issues will be addressed in the next 2 weeks. We are not able to fix it in the upcoming release (code freeze later this week).

mbed_configure_app_target stays for now. It will be removed as it should not be required.

I tested couple of targets with blinky to see if compile defs are properly defined and it builds without errors.

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

We set language standard via mbed-core and app inherits it if links to it. This is breaking change for application, please remove the function call from an application and it should build without errors.

This fixes the problem for an app/test to define APP_TARGET as requirements. This is not a proper fix but rather a workaround for broken apps/tests currently. We will address this separately via new pull request.

We need to fix linker script differently, it should not need APP_TARGET. This is a series of commits fixing APP_TARGET in our tree. We should not require it. The linker script preprocessing will be fixed differently. "application" prefix is temporary until we clean this up completely.

ciarmcom · 2021-02-10T14:30:28Z

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

rajkan01 · 2021-02-10T15:00:38Z

@0xc0170 This PR changes remove the dependency of APP_TARGET CMake variable but it fails to create map file for GCC_ARM toolchain with DISCO_L475VG_IOT01A for blinky example build.

0xc0170 · 2021-02-10T15:07:52Z

@0xc0170 This PR changes remove the dependency of APP_TARGET CMake variable but it fails to create map file for GCC_ARM toolchain with DISCO_L475VG_IOT01A for blinky example build.

That should be fixed, it's due to the name . We need to remove the last commit and fix it differently. I'll look at this later today.

rwalton-arm · 2021-02-10T15:10:27Z

@0xc0170 This PR changes remove the dependency of APP_TARGET CMake variable but it fails to create map file for GCC_ARM toolchain with DISCO_L475VG_IOT01A for blinky example build.

That should be fixed, it's due to the name . We need to remove the last commit and fix it differently. I'll look at this later today.

Do we just need to update the memap.py invocation here:

mbed-os/CMakeLists.txt

Line 191 in dd33463

    
           COMMAND ${Python3_EXECUTABLE} ${MBED_PATH}/tools/memap.py -t ${MBED_TOOLCHAIN} ${CMAKE_BINARY_DIR}/${target}${CMAKE_EXECUTABLE_SUFFIX}.map

tools/cmake/set_linker_script.cmake

ladislas · 2021-02-10T15:24:34Z

tools/cmake/set_linker_script.cmake

        target_link_options(${input_target}
            INTERFACE
            "-T" "${LINKER_SCRIPT_PATH}"
-            "-Wl,-Map=${CMAKE_BINARY_DIR}/${APP_TARGET}${CMAKE_EXECUTABLE_SUFFIX}.map"
+            "-Wl,-Map=${CMAKE_BINARY_DIR}/application${CMAKE_EXECUTABLE_SUFFIX}.map"


Removing the APP_TARGET dependency is great as it was the main roadblock to having multiple CMake targets.

But the hardcode won't help much.

Would using CMAKE_CURRENT_BINARY_DIR work? Something like:

Suggested change

"-Wl,-Map=${CMAKE_BINARY_DIR}/application${CMAKE_EXECUTABLE_SUFFIX}.map"

"-Wl,-Map=${CMAKE_CURRENT_BINARY_DIR}/application${CMAKE_EXECUTABLE_SUFFIX}.map"

If this is causing a trouble, can you create an issue? we use binary dir in multiple places so should be fixed for all.

This allows us to keep all in one place and fix (removing this function later)

0xc0170 · 2021-02-10T19:40:24Z

I moved also -Map for Gcc Arm to the function we will remove later and refactor. This should completely fix APP_TARGET usage in our tree. Only reminder there is mbed_configure_app_target that requires more work and we can remove it as well (another day another PR).

0xc0170 · 2021-02-10T19:40:52Z

CI started

mbed-ci · 2021-02-11T00:09:27Z

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_cmake-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-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	✔️
jenkins-ci/mbed-os-ci_cmake-example-test	❌
jenkins-ci/mbed-os-ci_greentea-test	✔️

0xc0170 · 2021-02-11T08:08:30Z

CI restarted

mbed-ci · 2021-02-11T09:16:43Z

Jenkins CI Test : ✔️ SUCCESS

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

CLICK for Detailed Summary

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

0xc0170 · 2021-02-11T11:47:20Z

Waiting for final reviews after my last fix.

0xc0170 added 3 commits February 10, 2021 11:31

CMake: move language standard to mbed-core

b334439

We set language standard via mbed-core and app inherits it if links to it. This is breaking change for application, please remove the function call from an application and it should build without errors.

CMake: linker script preprocessing moved to mbed_configure_app_target

d227723

This fixes the problem for an app/test to define APP_TARGET as requirements. This is not a proper fix but rather a workaround for broken apps/tests currently. We will address this separately via new pull request.

0xc0170 requested review from hugueskamba, rwalton-arm and rajkan01 February 10, 2021 14:05

0xc0170 mentioned this pull request Feb 10, 2021

CMake: Rename TEST_TARGET to APP_TARGET in greentea test CMakeLists.txt file #14258

Closed

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

ciarmcom requested review from a team February 10, 2021 14:30

ciarmcom added the needs: review label Feb 10, 2021

ladislas reviewed Feb 10, 2021

View reviewed changes

CMake: move Gcc Arm memmap to configure app

207dcbb

This allows us to keep all in one place and fix (removing this function later)

0xc0170 mentioned this pull request Feb 10, 2021

mbed-tools new . doesn't work because set(APP_TARGET ) in CMakeLists.txt ARMmbed/mbed-os-5-docs#1413

Closed

mergify bot added needs: work and removed needs: review labels Feb 10, 2021

0xc0170 added needs: CI and removed needs: work labels Feb 11, 2021

0xc0170 added needs: review and removed needs: CI labels Feb 11, 2021

hugueskamba approved these changes Feb 11, 2021

View reviewed changes

0xc0170 added ready for merge and removed needs: review labels Feb 11, 2021

0xc0170 merged commit 33c06fb into ARMmbed:master Feb 11, 2021

mergify bot removed the ready for merge label Feb 11, 2021

ladislas mentioned this pull request Feb 11, 2021

Change CMAKE_BINARY_DIR to CMAKE_CURRENT_BINARY_DIR #14272

Merged

mbedmain added release-version: 6.8.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Feb 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

CMake: remove need for APP_TARGET #14265

CMake: remove need for APP_TARGET #14265

Uh oh!

0xc0170 commented Feb 10, 2021

Uh oh!

ciarmcom commented Feb 10, 2021

Uh oh!

rajkan01 commented Feb 10, 2021

Uh oh!

0xc0170 commented Feb 10, 2021

Uh oh!

rwalton-arm commented Feb 10, 2021

Uh oh!

Uh oh!

ladislas Feb 10, 2021

Uh oh!

0xc0170 Feb 10, 2021

Uh oh!

0xc0170 commented Feb 10, 2021

Uh oh!

0xc0170 commented Feb 10, 2021

Uh oh!

mbed-ci commented Feb 11, 2021

Uh oh!

0xc0170 commented Feb 11, 2021

Uh oh!

mbed-ci commented Feb 11, 2021

Uh oh!

0xc0170 commented Feb 11, 2021

Uh oh!

Uh oh!

	"-Wl,-Map=${CMAKE_BINARY_DIR}/application${CMAKE_EXECUTABLE_SUFFIX}.map"
	"-Wl,-Map=${CMAKE_CURRENT_BINARY_DIR}/application${CMAKE_EXECUTABLE_SUFFIX}.map"

CMake: remove need for APP_TARGET #14265

CMake: remove need for APP_TARGET #14265

Uh oh!

Conversation

0xc0170 commented Feb 10, 2021

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

ciarmcom commented Feb 10, 2021

Uh oh!

rajkan01 commented Feb 10, 2021

Uh oh!

0xc0170 commented Feb 10, 2021

Uh oh!

rwalton-arm commented Feb 10, 2021

Uh oh!

Uh oh!

ladislas Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

0xc0170 Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Feb 10, 2021

Uh oh!

0xc0170 commented Feb 10, 2021

Uh oh!

mbed-ci commented Feb 11, 2021

Jenkins CI Test : ❌ FAILED

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

Uh oh!

0xc0170 commented Feb 11, 2021

Uh oh!

mbed-ci commented Feb 11, 2021

Jenkins CI Test : ✔️ SUCCESS

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

Uh oh!

0xc0170 commented Feb 11, 2021

Uh oh!

Uh oh!