Skip to content

Conversation

hugueskamba
Copy link
Collaborator

@hugueskamba hugueskamba commented Feb 23, 2021

Summary of changes

APP_TARGET needs to be removed as there should not be a name requirement for application CMake variable name.
Furthermore, in certain use cases it prevents successful builds for some Mbed targets. For instance when building Greentea test applications for Mbed targets that require post build operations as they do not define APP_TARGET.

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 needs to be removed as there should not be a
name requirement for application CMake variable name.
Furthermore, in certain uses cases it prevents
successful builds for some Mbed targets. For instance
when building Greentea test applications for Mbed
targets that require post build operations as they do
not define APP_TARGET.
@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Feb 23, 2021
@ciarmcom ciarmcom requested a review from a team February 23, 2021 17:30
@ciarmcom
Copy link
Member

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

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 23, 2021

@ARMmbed/team-cypress Please review

artefacts_location = sys.argv[1]

try:
binary = list(pathlib.Path(artefacts_location).glob("*.bin"))[0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this assuming there is just one binary ? Shall this be documented here (the assumption made)

Copy link
Collaborator Author

@hugueskamba hugueskamba Feb 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that is the assumption made.
@ifyall @RaymondNgun is this a fair assumption?

"""Entry point for the "sign" CLI command."""
try:
elf_file = list(pathlib.Path(args.artefacts_location).glob("*.elf"))[0]
m4hex_file = list(pathlib.Path(args.artefacts_location).glob("*.hex"))[0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ARMmbed/team-cypress will this work - taking the first bin/hex found in the artifacts ?
I would say yes - there's one application, for one core.

set(post_build_command
COMMAND ${Python3_EXECUTABLE} ${MBED_PATH}/targets/TARGET_NXP/scripts/LPC.py
${CMAKE_BINARY_DIR}/${APP_TARGET}.bin
${CMAKE_BINARY_DIR}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be current binary dir ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this instance it has to be CMAKE_BINARY_DIR.
Because with Greentea tests, CMAKE_CURRENT_BINARY_DIR here is not where we want.
For example when building the hal qspi Greentea test, CMAKE_CURRENT_BINARY_DIR is /path/to/mbed-os/hal/tests/TESTS/mbed_hal/qspi/cmake_build/LPC546XX/develop/ARM/build/targets/TARGET_NXP/TARGET_MCUXpresso_MCUS/TARGET_MCU_LPC546XX/. Whilst CMAKE_BINARY_DIR is /path/to/mbed-os/hal/tests/TESTS/mbed_hal/qspi/cmake_build/LPC546XX/develop/ARM/.

@mergify mergify bot added needs: CI and removed needs: review labels Mar 1, 2021
@0xc0170
Copy link
Contributor

0xc0170 commented Mar 1, 2021

CI started

@mbed-ci
Copy link

mbed-ci commented Mar 1, 2021

Jenkins CI Test : ✔️ SUCCESS

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

@0xc0170 0xc0170 merged commit 1f28e6e into ARMmbed:master Mar 1, 2021
@mergify mergify bot removed the ready for merge label Mar 1, 2021
@hugueskamba hugueskamba deleted the hk_cmake_remove_app_target_post_build_calls branch March 1, 2021 15:13
@mbedmain mbedmain added release-version: 6.9.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants