CMake: Address review comment from PR#13566 #13870

rajkan01 · 2020-11-05T18:43:01Z

Summary of changes

Addressed review comments from @rwalton-arm in the PRhttps://github.com//pull/13566

Replaced mbed_set_language_standard with target_compile_features.
Renamed input source files from CMakelists.txt to CMakeLists.txt.
Simplify the inclusion of cryptocell310 library files.
Added more information to TODO comment.
Removed empty input source files.

Impact of changes

None.

Migration actions required

None.

Documentation

None.

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)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[x] Tests / results supplied as part of this PR

Manula testing:
Successful compilation of the blinky example based on CMake.

Reviewers

@0xc0170 @hugueskamba @rwalton-arm

ciarmcom · 2020-11-05T19:00:22Z

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

0xc0170 · 2020-11-06T09:21:16Z

@rajkan01 Please fill in the template above (patch, tests, etc).

0xc0170 · 2020-11-06T09:24:53Z

CMakeLists.txt

@@ -109,7 +105,11 @@ add_subdirectory(cmsis/device/rtos EXCLUDE_FROM_ALL)
 # Configures the application
 #
 function(mbed_configure_app_target target)
-    mbed_set_language_standard(${target})
+    target_compile_features(${target}


shouldn't this be in our toolchain file (related to toolchain set up, although for an app)

I reverted the changes and keep the mbed_set_language_standard funcion and moved target_compile_feature inside of it, please re-review

tools/cmake/toolchain.cmake

connectivity/drivers/mbedtls/FEATURE_CRYPTOCELL310/TARGET_MCU_NRF52840/CMakeLists.txt

connectivity/drivers/mbedtls/FEATURE_CRYPTOCELL310/binaries/CMakeLists.txt

mergify · 2020-11-06T15:56:10Z

This PR cannot be merged due to conflicts. Please rebase to resolve them.

Pull request has been modified.

adbridge · 2020-11-09T11:52:52Z

@rajkan01 please update your PR header.

List what the changes actually are, or at least summarise. An alternative to summarising in the header is to have a separate commit for each of the required review changes and to have the details there instead. Currently there is no way of determining what this PR is actually fixing without trawling through all the review comments elsewhere ...
file out the header correctly as per the guidelines

…ed_lib.json

rajkan01 · 2020-11-12T11:30:58Z

Preceding PR# ARMmbed/mbed-os-example-ble#342

- Added absolute path as cmake failed to find linker file from relative path - Added the missing MBED_CONF_CRYPTOCELL310_PRESENT - Added the dependency library to BLE Cordio stack

rajkan01 · 2020-11-12T14:34:59Z

@0xc0170 I moved the changes from BLE_Advertising example into the mbed-os side, please re-review

rajkan01 · 2020-11-12T19:36:11Z

@0xc0170 cloud-client-test is failed with timeout_error_msg = "Didn't find client registered in 300 s", , is there any known issue if not could you please restart client test

mbed-ci · 2020-11-12T21:59:28Z

Jenkins CI Test : ❌ FAILED

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_build-cloud-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-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
jenkins-ci/mbed-os-ci_dynamic-memory-usage	✔️
jenkins-ci/mbed-os-ci_cloud-client-pytest	❌
jenkins-ci/mbed-os-ci_greentea-test	✔️
jenkins-ci/mbed-os-ci_cmake-example-test	❌

0xc0170 · 2020-11-13T08:52:15Z

both failures are not related to this PR, we will restart once fixed.

connectivity/drivers/mbedtls/FEATURE_CRYPTOCELL310/TARGET_MCU_NRF52840/CMakeLists.txt

0xc0170 · 2020-11-17T22:06:43Z

CI restarted

mbed-ci · 2020-11-18T00:30:51Z

Jenkins CI Test : ✔️ SUCCESS

Build Number: 4 | 🔒 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_build-greentea-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-GCC_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_dynamic-memory-usage	✔️
jenkins-ci/mbed-os-ci_cmake-example-test	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️
jenkins-ci/mbed-os-ci_cloud-client-pytest	✔️

mergify bot added the release-version: feature branch label Nov 5, 2020

ciarmcom requested review from 0xc0170, hugueskamba, rwalton-arm and a team November 5, 2020 19:00

ciarmcom added the needs: review label Nov 5, 2020

rajkan01 mentioned this pull request Nov 5, 2020

CMake: Add initial support #13566

Merged

0xc0170 reviewed Nov 6, 2020

View reviewed changes

rajkan01 force-pushed the feature-cmake-review-comment branch from f2d559f to cf3e86d Compare November 6, 2020 10:36

rajkan01 requested a review from 0xc0170 November 6, 2020 10:52

0xc0170 previously approved these changes Nov 6, 2020

View reviewed changes

hugueskamba reviewed Nov 6, 2020

View reviewed changes

mergify bot added needs: work and removed needs: review labels Nov 6, 2020

hugueskamba force-pushed the feature-cmake branch from 73a9c03 to 073ae1d Compare November 6, 2020 17:25

rajkan01 changed the base branch from feature-cmake to master November 9, 2020 10:52

rajkan01 force-pushed the feature-cmake-review-comment branch from cf3e86d to 3d35701 Compare November 9, 2020 10:57

rajkan01 force-pushed the feature-cmake-review-comment branch 2 times, most recently from ee57dcd to ecc5709 Compare November 9, 2020 16:00

rajkan01 changed the title ~~Incorporate the PR#13566 review comments from Rob~~ CMake: Address review comment from PR#13566 Nov 9, 2020

rajkan01 requested review from hugueskamba and 0xc0170 November 9, 2020 16:07

hugueskamba previously approved these changes Nov 9, 2020

View reviewed changes

0xc0170 previously approved these changes Nov 9, 2020

View reviewed changes

0xc0170 removed the needs: work label Nov 9, 2020

rajkan01 added 2 commits November 12, 2020 11:27

CMake: Simplify inclusion of cryptocell310 library files

cc7ae38

Remove add subdirectory of TARGET_MCU_NRF52840 as it contains only mb…

4c05ce8

…ed_lib.json

rajkan01 force-pushed the feature-cmake-review-comment branch from 0f0157a to 4c05ce8 Compare November 12, 2020 11:27

Fix CI issue:

438994d

- Added absolute path as cmake failed to find linker file from relative path - Added the missing MBED_CONF_CRYPTOCELL310_PRESENT - Added the dependency library to BLE Cordio stack

rajkan01 force-pushed the feature-cmake-review-comment branch from 890b6b2 to 438994d Compare November 12, 2020 14:32

rajkan01 requested a review from 0xc0170 November 12, 2020 14:40

0xc0170 approved these changes Nov 12, 2020

View reviewed changes

mergify bot added needs: CI needs: work and removed needs: work needs: CI labels Nov 12, 2020

hugueskamba requested changes Nov 16, 2020

View reviewed changes

connectivity/drivers/mbedtls/FEATURE_CRYPTOCELL310/TARGET_MCU_NRF52840/CMakeLists.txt Show resolved Hide resolved

hugueskamba approved these changes Nov 16, 2020

View reviewed changes

mergify bot added needs: CI and removed needs: work labels Nov 16, 2020

mergify bot added ready for merge and removed needs: CI labels Nov 18, 2020

0xc0170 merged commit fad7f34 into ARMmbed:master Nov 18, 2020

mergify bot removed the ready for merge label Nov 18, 2020

mbedmain added release-version: 6.5.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Nov 18, 2020

CMake: Address review comment from PR#13566 #13870

CMake: Address review comment from PR#13566 #13870

Uh oh!

Conversation

rajkan01 commented Nov 5, 2020 • 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 Nov 5, 2020

Uh oh!

0xc0170 commented Nov 6, 2020

Uh oh!

0xc0170 Nov 6, 2020

Choose a reason for hiding this comment

Uh oh!

rajkan01 Nov 6, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mergify bot commented Nov 6, 2020

Uh oh!

adbridge commented Nov 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rajkan01 commented Nov 12, 2020

Uh oh!

rajkan01 commented Nov 12, 2020

Uh oh!

rajkan01 commented Nov 12, 2020

Uh oh!

mbed-ci commented Nov 12, 2020

Jenkins CI Test : ❌ FAILED

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

Uh oh!

0xc0170 commented Nov 13, 2020

Uh oh!

Uh oh!

0xc0170 commented Nov 17, 2020

Uh oh!

mbed-ci commented Nov 18, 2020

Jenkins CI Test : ✔️ SUCCESS

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

Uh oh!

Uh oh!

rajkan01 commented Nov 5, 2020 •

edited

Loading

adbridge commented Nov 9, 2020 •

edited

Loading