[tools] Modified config to aggregate cumulative overrides against targets

[geky]

Modified '_process_config_and_overrides' to update cumulative attributes based on cumulative overrides found during walking config files.

This should remove most of the disparity between the config target cumulative overrides and cumulative attributes set in targets.json. These cumulative overrides are updated after the global target
instantiation though, which may cause internal build tool state dependent on cumulative attributes to not correctly reflect all cumulative overrides.

Example:

{
  "target_overrides": {
    "K64F": {
      "target.features_add": ["COOL_THING"],
      "target.extra_labels_add": ["COOL_THING_SUPPORTED"],
      "target.macros_add": ["COOL_THING_IS_HERE"],
      "target.device_has_add": ["COOL_THING_YES"]
    }
  }
}

cc @sg-, @screamerbg, @theotherjimmy
related #2209

[AlessandroA]

@geky I tested it with uVisor and the label does not appear in the macros list, nor in the mbed_config.h file.

My sample config file:

{
    "target_overrides": {
        "K64F": {
            "target.features_add": ["UVISOR"],
            "target.extra_labels_add": ["UVISOR_SUPPORTED"]
        }
    }
}

[geky]

Oh, looking into this there are complicated dependencies between the config system and the target system. This will require a bit of extra work to sort out the labels correctly.

[bogdanm]

This feature was not part of the original config system design and implementation, because those overrides are not actually configuration parameters. There are overrides of JSON attributes of the target object, and these attributes are not part of the configuration mechanism, This makes things confusing and hard to understand, just like it did with features before. Suggestions:

• turn these internal attributes into actual configuration parameters
• even if you don't turn them into configuration parameters, at the very least update the documentation of the configuration system, so that people understand what they can override.

[geky]

Hi @bogdanm, I agree the config system could be improved/refactored. However I don't see how your points are relevant to the issue in #2209, which is the goal of this pr right now.

[geky]

Hi @AlessandroA, I have updated this pr with additional handling for the extra_labels which I've tested locally. Let me know if this doesn't work for you.

[bogdanm]

However I don't see how your points are relevant to the issue in #2209, which is the goal of this pr right now.

Relevant:

even if you don't turn them into configuration parameters, at the very least update the documentation of the configuration system, so that people understand what they can override.

#2209 or not, a non-trivial change like this to the config system should be followed by a documentation update.

[geky]

I agree we should add additional documentation for target overrides in the config, but it seems inappropriate to combine a documentation update and a bug fix.

I can make a separate pr for adding the target override documentation?

[sg-]

docs are actually moving out of this repo for now due to the way the website renders things 😕

[AlessandroA]

@geky I tested the PR with the uVisor app and the symbols are all correctly defined. Thanks!
I agree with @bogdanm that this system should be documented ASAP, as the config implementation is diverging from those docs, and it's very hard for us (users) to track the changes.

PS: @sg- If I rebase @geky's branch on top of the latest master the compilation fails because of some UDP/TCPSocket error – Is this a known issue?

[sg-]

This could be the case due to duplicate network socket declarations in features of os an core. Try to delete one from mbed-os and let me know what happens

[AlessandroA]

@sg- Yes, that was it. Thanks!

[bogdanm]

I agree we should add additional documentation for target overrides in the config, but it seems inappropriate to combine a documentation update and a bug fix.

How is that inappropriate, when the actual fix for the bug introduces new behaviour in the configuration system, and people need to know how to use that? I would say that updating the documentation with the implementation is the most appropriate thing to do.

[geky]

I just don't see how coupling this bug fix and adding documentation for a mechanism that has been around since #1906 is beneficial for either issues.

Sorry we're not agreeing here and if I'm in the wrong. Has an issue been made on github for the missing documentation on cumulative attributes? I can look into adding what is missing.

[bogdanm]

I always thought that updating the documentation together with the code is always a good idea, and I can't recall a single time when this wasn't the case. What tends to happen if you don't do it right after you modify the code (and in the same PR) is that you'll forget about it and go on doing other things. It has happened over and over again, to myself and pretty much all the programmers I know. I don't believe that documentation updates need a separate issue on github, they should be part of the regular workflow.