Rework post-build to support multiple executables #14953
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The 'post build' functions are made visible by adding the mbed-os subdirectory. This is not ideal as any components in mbed-os wishing to call the functions must be added after the functions are defined. To improve modularity move these functions to a separate CMake script. We include the post build CMake script in app.cmake for now so we don't break user's projects.
ciarmcom
added
the
release-type: patch
|
@LDong-Arm, thank you for your changes. |
LDong-Arm
commented
Jul 22, 2021
targets/TARGET_Cypress/scripts/mbed_set_post_build_cypress.cmake
Outdated
Show resolved
Hide resolved
LDong-Arm
force-pushed
the
post_build_hook_rework
branch
2 times, most recently
from
d7ed587 to
47152df
Compare
Patater
reviewed
Jul 22, 2021
targets/TARGET_Cypress/scripts/mbed_set_post_build_cypress.cmake
Outdated
Show resolved
Hide resolved
LDong-Arm
force-pushed
the
post_build_hook_rework
branch
from
47152df to
48f9617
Compare
|
@LDong-Arm, thank you for your changes. |
rwalton-arm
previously requested changes
Jul 22, 2021
| # Copyright (c) 2021 Arm Limited. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| set(APP_TARGET app1) |
There was a problem hiding this comment.
Why define APP_TARGET? Seems like far less typing to just reuse app1, or app2. Is this part of some Mbed CMake style guide, or is mbed-os itself still relying on APP_TARGET somewhere?
There was a problem hiding this comment.
I'll change them to app1, app2.
| @@ -144,12 +144,16 @@ target_include_directories(mbed-core | |||
| add_library(mbed-device_key INTERFACE) | |||
| add_library(mbed-rtos INTERFACE) | |||
|
|
|||
| # Include targets/ first, because any post-build hook needs to be defined | |||
There was a problem hiding this comment.
In the commit message for commit "Rework post-build to support multiple executables", we say "guard it with a macro to checks if", that should be "guard it with a macro to check if"
There was a problem hiding this comment.
Good catch. I probably forgot to change it when rephrasing the sentence. 😅
LDong-Arm
force-pushed
the
post_build_hook_rework
branch
from
48f9617 to
ac2c5ab
Compare
mergify
bot
dismissed
rwalton-arm’s stale review
Pull request has been modified.
When building greentea tests, each test is an executable with its
own output binary path. This is also the case when a user project
produces multiple executables. But the current implementation of
post-build operations always assumes there's only one executable,
at the root of the build directory.
The post-build command depends on Mbed target, and it always takes
the the executable we build as an input file. To achieve this, we
let each Mbed target (that has a post-build command) define a function
function(mbed_post_build_function target)
which takes a CMake executable target as an argument from which it can
get its binary path using generator expressions. It generates and adds
to the passed executable target a post-build custom command.
Notes:
* The function name needs to be exact, because CMake only supports
literal function calls - CMake can't dereference a function name from
a variable. To avoid multiple definitions of this function, each Mbed
target needs to guard it with a macro to check if the user is
building this Mbed target.
* `mbed_post_build_function()` is a function, but it is usually
defined by another macro rather than a parent function, because
nesting functions would make many variables inaccessible inside the
innermost `mbed_post_build_function()`.
* There's no more need to force regenerate images. Previously, post-
build commands were custom *targets* which always got to run, so we
force regenerated images on every build to avoid patching an image
that's already been patched once on previous build. Now post-build
commands are custom *commands* of the same executable target, and they
are only run if the executable target itself is rebuilt.
The CMake macro `mbed_post_build_psoc6_merge_hex()` takes the name of
a Cypress target and an optional Cortex-M0 hex image as arguments. The
proper way to define and parse optional arguments of a function or
macro is `cmake_parse_arguments()`, which allows the caller to
indicate what they are passing rather than rely on an argument's
relative position within `${ARGN}` which is not rigorous.
Also, avoid duplicating the common part of the post build command
when the optional argument is passed/not passed.
Add a test to build two executables in two directories under a single project.
LDong-Arm
force-pushed
the
post_build_hook_rework
branch
from
ac2c5ab to
23d659e
Compare
LDong-Arm
commented
Jul 22, 2021
There was a problem hiding this comment.
@Patater @rwalton-arm Thanks for reviewing, I've addressed your comments.
rwalton-arm
approved these changes
Jul 23, 2021
| merge | ||
| --elf $<TARGET_FILE_DIR:${target}>/$<TARGET_FILE_BASE_NAME:${target}>.elf | ||
| --m4hex $<TARGET_FILE_DIR:${target}>/$<TARGET_FILE_BASE_NAME:${target}>.hex | ||
| if(NOT "${CYPRESS_CORTEX_M0_HEX}" STREQUAL "") |
There was a problem hiding this comment.
Looks much better to me, the intent is now clear.
|
CI started |
0xc0170
approved these changes
Jul 23, 2021
Jenkins CI Test : ✔️ SUCCESSBuild Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
mbedmain
added
release-version: 6.14.0
Release-pending
and removed
release-type: patch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of changes
The current implementation of post-build operations always assumes there's only one CMake executable target, at the root of the build directory. When a project creates multiple executables under different subdirectories, the target-specific post-build command fails to locate each built executable to act on.
This PR reworks the post-build mechanism to allow running the command once per executable target with correct path to executable. For details, refer to the commit message of "Rework post-build to support multiple executables".
To verify support for multiple executables (and building apps with Mbed CLI 2 in general), this PR adds a test project containing two executables and a new GitHub Workflow to build it.
Fixes: #14933
Impact of changes
A user project can define multiple executables, as long as each has
mbed_set_post_build()called.Migration actions required
Documentation
None
Pull request type
Test results
Reviewers
@ARMmbed/mbed-os-core