Add extra_targets.json support to build tools #4103

andrewleech · 2017-04-03T08:56:16Z

Description

Enhancement to provide simple method of adding or overriding target definitions without modifying mbed-os library.

If the file extra_targets.json exists in the working directory load it over the top of the built in targets.json for defining new and overriding built in mbed target definitions.

This is currently just a suggestion for replacing functionality removed in PR #2691 below.

Status

IN DEVELOPMENT
Not ready for merging, open for discussion of direction before considering writing docs/tests

Related PRs

branch	PR
no_custom_targets	Removed custom targets from config system

Todos

Tests
Documentation

theotherjimmy · 2017-04-03T16:55:27Z

tools/targets/__init__.py

+        # If extra_targets.json exists in working directory load it over the top
+        extra = os.path.join('.', 'extra_targets.json')
+        if os.path.exists(extra):
+            targets.update(json_file_to_dict(extra))


This will wipe out any old target in targets.json if they are re-defined in an extra_targets.json file.

Yeah that was my intention, so that you can replace/change/fix existing ones if desired.

Obviously if this is likely to confuse people it could be merged in a more intelligent way (I've got a good recursive merge function if needed from another project)

I think that most people will expect it to merge a bit more intelligently, or for merging to be disallowed completely. Something like overwriting only the keys that are in the extra_targets.json is what I would expect to happen.

I added a dict merge function for reference, it's not tested yet though.

theotherjimmy

I like adding this as a separate file, because it makes it clear that it's not an application configuration.

theotherjimmy · 2017-04-03T16:58:02Z

Could you please explain why you need extra targets?

sg-

In general this is much more of what I'd expect use to do. I'd be happy to also see how this should be applied to MCU -> board -> custom boards if a single targets.json was broken up into more manageable chunks

theotherjimmy · 2017-04-03T19:02:33Z

@sg- would you expect to have a single targets structure built from relative paths in the target files or something.

For example:
targets.json

{
  "Target": ...,
  "child_target_files" : ["TARGET_NXP/targets.json", ...]
}

andrewleech · 2017-04-03T23:55:46Z

@theotherjimmy our specific need for something like this is commercial project development.

We might have a small team, each with a dev board and some common peripherals. We'd like to have a new named target with definitions for this exact configuration of dev board+custom extras which we'll each have a copy of.
In theory you could achieve similar by adding the definitions to the existing target with mbed_app.json but then you're still compiling -m original named dev board which makes it less clear the configuration management.

Similar story in the next phase of the project where we'll have a custom board based heavily on the devboard+custom extras. It would be best to have a new named target inheriting from dev board details but specific to this project, not merged into a fork of mbed-os.

theotherjimmy · 2017-04-03T23:57:11Z

Sounds like this is more a personal preference than a pressing "we cannot develop without this" style need.

theotherjimmy · 2017-04-03T23:59:03Z

I urge you to use the mbed_app.json file for your preferences, and use the mbed-CLI configuration system to hide exactly which target you are compiling for. At the end of the day you're just going to run mbed compile (as written, with no arguments) either way.

theotherjimmy · 2017-04-04T00:03:04Z

by mbed CLI configuration, I mean the mbed config, mbed target and mbed toolchain commands.
https://github.com/ARMmbed/mbed-cli#mbed-cli-configuration

They live in the .mbed file in the root. You could check that file in to make it convenient for everyone in your team at the same time.

andrewleech · 2017-04-04T00:04:57Z

Is there any way to create multiple new build targets this way?

Very soon we'll need to be able to target the raw devboard, the devboard+extras and the custom board from the same codebase. It would be by far the most ideal if these can be different target names that can be compiled for.

Currently, I guess we need multiple mbed_app.json files with the different definitions and use mbed compile --add-config=xxx -N xxx to chose which one and compile a different artifact name, but then the rest of the mbed_app.json definitions need to be duplicated between all copies.

Otherwise it's maintaining a private fork of the entire mbed-os repo, and manually merging in upstream updates etc.... ?

andrewleech · 2017-04-04T00:06:20Z

on a slightly related note, it'd be great if config system had a key for artifact name (mbed compile -N). I would raise an issue but I might just make it myself and new PR.

theotherjimmy · 2017-04-04T00:06:40Z

Ah, now you need custom targets. I'm glad we had this discussion, because we got to the bottom of the requirement here. I think custom targets in an extra_targets.json or similar would be a wonderful solution.

theotherjimmy · 2017-04-04T00:07:10Z

on a slightly related note, it'd be great if config system had a key for artifact name (mbed compile -N). I would raise an issue but I might just make it myself and new PR.

PR or issue works. Having it implemented probably gets it in sooner.

FYI, the code you would be looking at changing is the Config class in config.py and the build_project function in build_api.py.

theotherjimmy · 2017-04-04T00:08:18Z

You can do --app-config=<filename> on the command line. Then your compiles would look like mbed complie --app-config=devboard+extras.json and mbed compile --app-config=raw-devboard.json or similar.

theotherjimmy · 2017-04-04T00:11:24Z

Actually, I like the config for -N suggestion so much, I'm going to mock it up real quick and maybe submit a PR shortly.

theotherjimmy · 2017-04-04T00:21:48Z

@andrewleech Check out #4107

andrewleech · 2017-04-04T01:14:27Z

yes, custom targets was the removed feature I was suggesting a replacement for, multiple targets is definitely my key requirement!
I muddied the waters by suggesting it could also be used to modify existing ones etc...

The --app-config route was my best guess at an existing solution, but it's a bit messy due the lack of inheritance; changes to mbed_app.json that aren't target specific will need to be duplicated through all of them.

0xc0170 · 2017-04-05T15:14:52Z

Is there any future work planned here for this patch? Or we can close it?

theotherjimmy · 2017-04-05T17:02:08Z

Well, it should pass CI...

andrewleech · 2017-04-05T21:04:28Z

I'm very keen for the functionality to be able to add multiple targets without forking mbed-os.

If this method is likely to be accepted if I clean it up I'd like to proceed, it'll clean it up but it'll take some days to find the time around work.

theotherjimmy · 2017-04-05T21:26:59Z

There are a few changes that need to happen in this PR for it to be accepted. I know exactly what they are and they are difficult to explain. May I add a few commits to your branch?

andrewleech · 2017-04-05T22:59:49Z

Yeah absolutely, very happy for any guidance!

theotherjimmy · 2017-04-06T18:38:07Z

Thanks for the invite, but if you checked the maintainers can modify box, then I already have access. 😈

theotherjimmy · 2017-04-06T19:51:22Z

I just wanted to make sure that the parsing was done correctly, and that it would be possible to put custom_targets.json in any directory passed to --source

andrewleech · 2017-04-07T01:31:38Z

I see where you're going with that, just needs a couple of little fixes still

theotherjimmy

I have to do one more thing. Excellent job picking up where I left off.

theotherjimmy · 2017-04-07T16:43:43Z

tools/targets/__init__.py


-TARGET_NAMES = TARGET_MAP.keys()
+update_target_data()


Solid + 💯

pi-anl · 2017-05-31T01:04:10Z

Yeah it would be quite straightforward to not override existing targets.
In many ways that does make sense; there's already mechanisms for modifying existing targets via feature add/exclude etc in mbed_app.json and I generally prefer there to be one, obvious way to do things.

theotherjimmy · 2017-06-19T16:49:28Z

@andrewleech @pi-anl Could you rebase to resolve conflicts?

If the file extra_targets.json exists in the working directory load it over the top of the built in targets.json for defining new and overriding built in mbed target definitions.

Recursively merge any target configs in extra_targets.json rather than completely replacing keys at the top level

Avoid duplication of update_target_data() code Keep "custom_targets.json" filename definition in Targets()

A warning will be printed if it is attempted.

adbridge · 2017-06-23T09:36:02Z

@theotherjimmy what is the best way to test this ?

theotherjimmy · 2017-06-23T14:12:44Z

Travis runs the unit tests included in this PR.

0xc0170 · 2017-06-26T10:53:22Z

@theotherjimmy Please review after the rebase. Travis green, then we shall be good to have this ready for integration?

The file the CLI is looking is extra_targets.json, not custom_targets.json ( I think an older version of mbed CLI used custom_targets.json ) I just tried this at the command line and confirmed this. Also, this PR talks about extra_targets ARMmbed/mbed-os#4103

0xc0170 requested review from theotherjimmy and sg- April 3, 2017 10:59

0xc0170 added the needs: review label Apr 3, 2017

theotherjimmy reviewed Apr 3, 2017

View reviewed changes

sg- reviewed Apr 3, 2017

View reviewed changes

theotherjimmy mentioned this pull request Apr 4, 2017

Allow configuration of artifact name in app config #4107

Merged

theotherjimmy reviewed Apr 7, 2017

View reviewed changes

tools/targets/__init__.py

TARGET_NAMES = TARGET_MAP.keys()

update_target_data()

Copy link

Contributor

theotherjimmy Apr 7, 2017

Choose a reason for hiding this comment

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

Solid + 💯

sg- added needs: work and removed needs: review labels Jun 15, 2017

theotherjimmy approved these changes Jun 15, 2017

View reviewed changes

andrewleech and others added 8 commits June 21, 2017 22:14

Add extra_targets.json support to build tools

e6e3d08

If the file extra_targets.json exists in the working directory load it over the top of the built in targets.json for defining new and overriding built in mbed target definitions.

Recursively merge extra_targets into targets

58c52fa

Recursively merge any target configs in extra_targets.json rather than completely replacing keys at the top level

Find extra targets in all source folders

2c4475c

Add custom_targets.json file contents to targets

bf08b10

Avoid duplication of update_target_data() code Keep "custom_targets.json" filename definition in Targets()

Handle situation when options.source_dir is None

2540f01

Clear custom-targets uppon setting new taregts.json location

49645b4

Add basic unit tests for custom_targets.json handling

6bd55a1

Prevent modifying existing targets.

4491d2e

A warning will be printed if it is attempted.

andrewleech force-pushed the extra_targets branch from 17baf19 to 4491d2e Compare June 21, 2017 12:19

adbridge added needs: CI and removed needs: work labels Jun 23, 2017

0xc0170 added needs: review and removed needs: CI labels Jun 26, 2017

theotherjimmy approved these changes Jun 26, 2017

View reviewed changes

theotherjimmy added ready for merge and removed needs: review labels Jun 26, 2017

theotherjimmy merged commit f3dc8a6 into ARMmbed:master Jun 26, 2017

theotherjimmy added release-version: 5.5.2 and removed ready for merge labels Jun 26, 2017

amq mentioned this pull request Jul 5, 2017

Exporter doesn't parse extra_targets.json #4709

Closed

Alex-EEE mentioned this pull request Jul 27, 2018

Update mbed_targets.md ARMmbed/mbed-os-5-docs#646

Closed

Add extra_targets.json support to build tools #4103

Add extra_targets.json support to build tools #4103

Uh oh!

Conversation

andrewleech commented Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Status

Related PRs

Todos

Uh oh!

theotherjimmy Apr 3, 2017

Choose a reason for hiding this comment

Uh oh!

andrewleech Apr 3, 2017

Choose a reason for hiding this comment

Uh oh!

theotherjimmy Apr 3, 2017

Choose a reason for hiding this comment

Uh oh!

andrewleech Apr 4, 2017

Choose a reason for hiding this comment

Uh oh!

theotherjimmy left a comment

Choose a reason for hiding this comment

Uh oh!

theotherjimmy commented Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sg- left a comment

Choose a reason for hiding this comment

Uh oh!

theotherjimmy commented Apr 3, 2017

Uh oh!

andrewleech commented Apr 3, 2017

Uh oh!

theotherjimmy commented Apr 3, 2017

Uh oh!

theotherjimmy commented Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theotherjimmy commented Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewleech commented Apr 4, 2017

Uh oh!

andrewleech commented Apr 4, 2017

Uh oh!

theotherjimmy commented Apr 4, 2017

Uh oh!

theotherjimmy commented Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theotherjimmy commented Apr 4, 2017

Uh oh!

theotherjimmy commented Apr 4, 2017

Uh oh!

theotherjimmy commented Apr 4, 2017

Uh oh!

andrewleech commented Apr 4, 2017

Uh oh!

0xc0170 commented Apr 5, 2017

Uh oh!

theotherjimmy commented Apr 5, 2017

Uh oh!

andrewleech commented Apr 5, 2017

Uh oh!

theotherjimmy commented Apr 5, 2017

Uh oh!

andrewleech commented Apr 5, 2017

Uh oh!

theotherjimmy commented Apr 6, 2017

Uh oh!

theotherjimmy commented Apr 6, 2017

Uh oh!

andrewleech commented Apr 7, 2017

Uh oh!

theotherjimmy left a comment

Choose a reason for hiding this comment

Uh oh!

andrewleech commented Apr 3, 2017 •

edited

Loading

theotherjimmy commented Apr 3, 2017 •

edited

Loading

theotherjimmy commented Apr 3, 2017 •

edited

Loading

theotherjimmy commented Apr 4, 2017 •

edited

Loading

theotherjimmy commented Apr 4, 2017 •

edited

Loading