Refactor ST Mbed targets to be CMake buildsystem targets #14199
Conversation
ciarmcom
added
the
release-type: patch
ciarmcom
requested review from
0xc0170,
hugueskamba,
Patater,
urutva and
a team
|
@rwalton-arm, thank you for your changes. |
rwalton-arm
force-pushed
the
dev/rwalton-arm/refactor-st-mbed-targets
branch
from
0065c44 to
99614a0
Compare
|
Got an error : |
Thanks for taking a look. I need to raise PRs removing the function call from the examples. If you delete |
|
mbedtools compile -m NUCLEO_H743ZI2 -t GCC_ARM -f This works now! |
There was a problem hiding this comment.
Awesome! We are getting slowly there, CMake targets in targets directory ! More to go!
[x] Major update (Breaking change E.g. Return code change / API behaviour change)
Can we make this nonbreaking ? It would simplify things, I don't think we are ready to release a new major version the next month.
what if we keep the old function there, and print a warning that the function disappears in the next major? We will accumulate more things like this. Or do we need to have the linker in this PR ?
another one: this is porting the targets keeping the name. If we are creating something new, can we fix the naming to follow the components names?
to illustrate: NUCLEO_F072RB would be nucleo-f072rb ? Either this or we rename all the components however the application ones use mbed-os/mbed-os-baremetal so I guess rather to follow them than otherway around. To translate this from old names to new should be fairly straightforward (find and replace function in CMake?).
Shall we also create mcu targets ? Previous tools had the limitation there to use TARGET_ as no MCU_ prefix was implemented. Thus target MCU_STM32L4R9xI was using actually TARGET_STM32L4R9xI. This is a base target, shouldn't it be named as such? The custom target would inherit from mcutargets. The tree would look likenucleo-f32l4inherits frommcu-stm32l4`. I had so many discussions about this problem in the tree, we started adding mcu too late and never had an opportunity to fix it. We might have now.
What about naming scheme for targets, would it make sense to have mbed-target prefix? these are specific to mbed, they are targets. What naming should custom targets use? For instance, we used to have target-frmd-k64f-gcc in Mbed 3.
| INTERFACE | ||
| system_clock.c | ||
| ${STARTUP_FILE} | ||
| ) | ||
|
|
||
| target_include_directories(mbed-core | ||
| target_include_directories(STM32F091xC |
There was a problem hiding this comment.
I assume this follows the naming convection we have in targets.json? do we need to keep this as mixed uppercase with lower case?
I would use lower case in all targets if possible (the translation should be straightforward from old to new). Or not?
There was a problem hiding this comment.
This is the convention from the folder structure and targets.json. Generally CMake style is to use camel case for targets, which I see we don't follow in Mbed, so I just went with the folder naming convention. Is the Mbed style to use all lowercase?
Yeah, I'll look into making this non-breaking. I can keep the old function around and add a warning. We do need the linker script changes in this PR for the new stuff to work correctly (it's also a more flexible architecture going forward if the target provides the linker script rather than setting a global property).
We can't change the target names, they need to match what we have in targets.json for now as we pass the target name from the tools straight to |
target.json stays as it is, we just do postprocessing in CMake to translate old naming to the new one. I'll create a draft to illustrate (based on this PR, to do the minimal effort to see if its worth doing). |
|
Regarding the breaking change, do you know how many users are using it? Can you get the pip install stats? As the new tools are still in beta, and based on the issues/feedback on Github and the forum, it doesn't seem like too many people are using it in their daily work. I would not worry about a breaking change on something that's not widely used and that will only make the life of the users better in the short term. IMHO, more generally, all things related to CMake should be treated as non-breaking as the user base actively using them is quite small (because of the current limitatations) and people can always pin the version they need. Plus, I'd say that if you want to cmake, updating your main CMakeLists.txt is really not an issue. :) |
rwalton-arm
force-pushed
the
dev/rwalton-arm/refactor-st-mbed-targets
branch
from
99614a0 to
7e50d55
Compare
|
a quick search for https://github.com/search?p=6&q=mbed_set_mbed_target_linker_script&type=Code |
I agree that we shouldn't make this a "breaking" change. I've changed the PR type to "feature". The confusion here was my fault for marking the PR as a "major" incorrectly. |
targets/TARGET_STM/TARGET_STM32F3/TARGET_STM32F334x8/CMakeLists.txt
Outdated
Show resolved
Hide resolved
rwalton-arm
force-pushed
the
dev/rwalton-arm/refactor-st-mbed-targets
branch
from
7e50d55 to
222a556
Compare
There was a problem hiding this comment.
The outstanding questions are:
- can we be consistent in CMake to use lower cases with
-delimeter (translation in CMake via a simple string and replace), no change in tagets.json or tools, just we keep CMake target naming same
Future wish: I wish to have mcu, vendor or similar in the target names to describe better what is it - and also better hierarchy of targets. This might wait as it needs to refactor targets.json and much more thinking how to structure them (it's not trivial task). we live what we have for now, targets.json
CMakeLists.txt
Outdated
| @@ -6,6 +6,7 @@ | |||
| cmake_minimum_required(VERSION 3.19.0 FATAL_ERROR) | |||
|
|
|||
| include(${MBED_CONFIG_PATH}/mbed_config.cmake) | |||
| include(tools/cmake/SetLinkerScript.cmake) | |||
There was a problem hiding this comment.
can we rename to set_linker_script ? we do not use camel case naming just CMakeLists does.
| @@ -67,7 +67,7 @@ target_compile_definitions(mbed-core | |||
|
|
|||
| # Add compile definitions for backward compatibility with the toolchain | |||
| # supported. New source files should instead check for __GNUC__ and __clang__ | |||
| # for the GCC_ARM and ARM toolchains respectively. | |||
| # for the GCC_ARM and ARM toolchains respectively. | |||
There was a problem hiding this comment.
how this spaces got in... Jamie proposed to have formatter for CMake, we should review that one to avoid these in future.
There was a problem hiding this comment.
I've used https://github.com/cheshirekow/cmake_format before and it worked well. There are pre-commit hooks available, so we could add it to CI quite easily. It's very easy to miss trailing whitespace if your editor isn't set up correctly to flag it.
There was a problem hiding this comment.
I found the PR adding format: #14011 ,if you have a moment, please review
There was a problem hiding this comment.
besides that, I can make a PR to add .editorconfig -- it's almost a standard and it's super useful for this kind of case.
There was a problem hiding this comment.
besides that, I can make a PR to add .editorconfig -- it's almost a standard and it's super useful for this kind of case.
That might be useful.
Refactor all ST targets to be CMake buildsystem targets. This removes the need for checking MBED_TARGET_LABELS repeatedly and allows us to be more flexible in the way we include MBED_TARGET source in the build. A side effect of this is it will allow us to support custom targets without breaking the build for 'standard' targets, as we use CMake's standard mechanism for adding build rules to the build system, rather than implementing our own layer of logic to exclude files not needed for the target being built. Using this approach, if an MBED_TARGET is not linked to using `target_link_libraries` its source files will not be added to the build. This means custom target source can be added to the user's application CMakeLists.txt without polluting the build system when trying to compile for a standard MBED_TARGET.
Patater
force-pushed
the
dev/rwalton-arm/refactor-st-mbed-targets
branch
from
a156131 to
d9e184b
Compare
mergify
bot
dismissed stale reviews from Patater and 0xc0170
Pull request has been modified.
|
Rebased on latest |
|
I merged all examples, started CI. |
Jenkins CI Test : ❌ FAILEDBuild Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
|
CI restarted |
|
Ci restarted |
Jenkins CI Test : ❌ FAILEDBuild Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
Jenkins CI Test : ✔️ SUCCESSBuild Number: 3 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
mbedmain
added
release-version: 6.8.0
Release-pending
and removed
release-type: patch
Summary of changes
Refactor all ST targets to be CMake buildsystem targets. This removes
the need for checking MBED_TARGET_LABELS repeatedly and allows us to be
more flexible in the way we include MBED_TARGET source in the build.
A side effect of this is it will allow us to support custom targets
without breaking the build for 'standard' targets, as we use CMake's
standard mechanism for adding build rules to the build system, rather
than implementing our own layer of logic to exclude files not needed for
the target being built. Using this approach, if an MBED_TARGET is not
linked to using
target_link_librariesits source files will not beadded to the build. This means custom target source can be added to the
user's application CMakeLists.txt without polluting the build system
when trying to compile for a standard MBED_TARGET.
Impact of changes
You will need to remove any calls to
mbed_set_mbed_target_linker_scriptin your application level
CMakeLists.txtas this function has been removed.Migration actions required
This PR depends on PRs to fix the examples' CMakeList files. I'll link the
PRs here after they've been raised.
Documentation
None
Pull request type
Test results
Reviewers
@Patater @hugueskamba @0xc0170 @urutva