Fix ST targets naming and linker script issues #14275
Merged
Conversation
This file contains hidden or 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
ciarmcom
added
the
release-type: patch
|
@rwalton-arm, thank you for your changes. |
jeromecoutant
approved these changes
Feb 12, 2021
rwalton-arm
force-pushed
the
dev/rwalton-arm/fix-more-st-targets
branch
from
4a10144 to
f9b19f3
Compare
|
Is this still WIP? This is a must in the upcoming release :-) I'll start CI meanwhile |
Jenkins CI Test : ✔️ SUCCESSBuild Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
|
This PR cannot be merged due to conflicts. Please rebase to resolve them. |
rwalton-arm
changed the title
rwalton-arm
force-pushed
the
dev/rwalton-arm/fix-more-st-targets
branch
from
f9b19f3 to
9638dde
Compare
Should be ready to merge now. Thanks. |
|
This PR cannot be merged due to conflicts. Please rebase to resolve them. |
|
One last rebase and we are good to go ! |
0xc0170
reviewed
Feb 15, 2021
tools/cmake/toolchain.cmake
Outdated
| @@ -19,8 +19,8 @@ function(mbed_generate_options_for_linker target definitions_file) | |||
| set(_compile_definitions | |||
| "$<$<BOOL:${_compile_definitions}>:-D$<JOIN:${_compile_definitions}, -D>>" | |||
| ) | |||
| file(GENERATE OUTPUT "${CMAKE_CURRENT_BINARY_DIR}/compile_time_defs.txt" CONTENT "${_compile_definitions}\n") | |||
| set(${definitions_file} @${CMAKE_CURRENT_BINARY_DIR}/compile_time_defs.txt PARENT_SCOPE) | |||
| file(GENERATE OUTPUT "${CMAKE_BINARY_DIR}/compile_time_defs.txt" CONTENT "${_compile_definitions}\n") | |||
There was a problem hiding this comment.
Also this should be "current" - the recent fix to allow multiple targets to be built
rwalton-arm
force-pushed
the
dev/rwalton-arm/fix-more-st-targets
branch
from
9638dde to
039a98a
Compare
rwalton-arm
force-pushed
the
dev/rwalton-arm/fix-more-st-targets
branch
from
039a98a to
380d88d
Compare
We need to generate a "response file" to pass to the C preprocessor when we preprocess the linker script, because of path length limitations on Windows. We set the response file and bind the path to a global property. The MBED_TARGET being built queries this global property when it sets the linker script. We must set this global property before the targets subdirectory is added to the project. This is required because the MBED_TARGET depends on the response file. If the path to the response file is not defined when the target requests it the config definitions will not be passed to CPP.
0xc0170
previously approved these changes
Feb 15, 2021
rwalton-arm
force-pushed
the
dev/rwalton-arm/fix-more-st-targets
branch
from
380d88d to
fd63d33
Compare
mergify
bot
dismissed
0xc0170’s stale review
Pull request has been modified.
|
Aborting CI and restarting |
Jenkins CI Test : ❌ FAILEDBuild Number: 3 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
|
I restarted CI (CI error) |
Patater
approved these changes
Feb 15, 2021
Jenkins CI Test : ✔️ SUCCESSBuild Number: 4 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
mbedmain
added
release-version: 6.8.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 recent PR to change the CMake target names to lowercase seems to have missed a few targets, and introduced other typos.
The PR that moved
mbed_generate_options_for_linkertombed_configure_apphas caused another issue: the response file property is not set at the time the targets request it. This is because we must call the function to set the response file after adding thembed-osdirectory in the application due to our non-obvious backward coupling betweenmbed-osand the application. Thembed_generate_options_for_linkerfunction is not available to the app before addingmbed-os, and thembed-targetexpects the global response file property to be set at the timembed-osis added. This causes build failures for some targets and incorrect linker script config for others. So, I've moved thembed_generate_options_for_linkerfunction back tombed-os/CMakeLists.txtand made it take the compile options frommbed-corerather than the app. This should work as the linker scripts only seem to rely on config definitions coming from thembed_config.cmakefilembed-toolsproduces, which are added to thembed-coretarget. However, we need to run a full regression test of all targets to ensure this approach does work for everything.Impact of changes
Migration actions required
Documentation
None
Pull request type
Test results
Reviewers