Fix usage of custom targets in applications and tests #2196
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
Commit 43e036d removed the ability to use custom targets (defined in mbed_app.json) by restricting the type of the "target" argument to the list of the known targets. The problem with this is that the configuration system can dynamically add new custom targets to the list of known targets. In order to do this, however, it needs the list of top level source directories, so that it can look for a mbed_app.json file in them. This commit fixes this problem by splitting argument parsing in two steps: 1. in step 1, only '--source' is parsed. 2. the config object is created after this. 3. the rest of the command line options are parsed. 4. the config object created in step 2 is passed around to the various build functions (build_project, build_tests and so on) when needed.
|
Cc @screamerbg |
|
@bogdanm I can confirm that this is fixed both for apps and tests. Thanks for the fix! @screamerbg @0xc0170 @theotherjimmy Please note that this issue is now a blocker for the public release of the uVisor example ARMmbed/mbed-os-example-uvisor, which uses a custom target. |
|
This creates a spaghetti dependencies:
Why not simply remove the target verification from options and leave that to the toolchain object?? |
|
Because you need to know the full list of targets when you parse the arguments, so that you can display it to the user if he specifies a wrong target (or simply because he needs to know all available targets). |
|
@bogdanm I'm aware of the reasoning of the implementation. The question is "Why not simply remove the target verification from options and leave that to the toolchain object??" |
|
You could probably do that, but I don't really see the advantages, especially since I believe the implementation effort is actually larger than the one presented in this PR. |
|
@bogdanm What implementation effort? We can simply stop validating the target argument and let the toolchain and config handle it. |
|
The implementation effort that comes from re-implementing something that's already implemented in this PR, without real benefits from what I can tell. |
|
Usage information is broken by this PR. was: |
'-h' was broken because it was showing help for the first (--source only) argument parser. Fixed by creating the first argument parser without `-h` and re-adding the `--source` argument to the second one to get a full help text. Tested with `python tools make.py -h` and `python tools/test.py -h`.
|
@theotherjimmy, thanks, I didn't notice that. Fix with the latest commit. Let me know if that looks OK to you; if so, I'll rebase against master and merge. |
|
Trying to use custom targets as documented, and I believe Im being bitten by this. |
|
@theotherjimmy: please let me know if the current solution looks OK, so that I can rebase and merge it. |
|
I'm still opposed to making two argument parsers for each entry point, and maintaining two copies of the |
|
I don't think custom targets is something we should to support until the inheritance and structure of targets is better defined. |
|
Agreed, they seem to be causing quit a bit of issues. Maybe we should deprecate them? |
Please do and we can come back to this later |
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.
Commit 43e036d removed the ability to use custom targets (defined in
mbed_app.json) by restricting the type of the "target" argument to the
list of the known targets. The problem with this is that the
configuration system can dynamically add new custom targets to the list
of known targets. In order to do this, however, it needs the list of top
level source directories, so that it can look for a mbed_app.json file
in them. This commit fixes this problem by splitting argument parsing in
two steps:
build functions (build_project, build_tests and so on) when needed.