Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce FEATURE_ which maps to Target.features #1878

Merged
merged 1 commit into from
Jun 9, 2016
Merged

Introduce FEATURE_ which maps to Target.features #1878

merged 1 commit into from
Jun 9, 2016

Conversation

screamerbg
Copy link
Contributor

@screamerbg screamerbg commented Jun 8, 2016

Introduce FEATURE_ keyword which behaves like TARGET_ but is generated from Target.features (see targets.json).
This allows specific functionality (and source-code directories) to be turned on and off depending on target features. E.g. feature "MPU" can turn on uVisor for targets that do have MPU and also implement support for uVisor

@0xc0170, @sg-, @bogdanm, @meriac, @theotherjimmy

@bogdanm
Copy link
Contributor

bogdanm commented Jun 8, 2016

Are we sure we want to do this? Some thoughts:

  • we're adding to the magic of TARGET_ and TOOLCHAIN_.
  • a bunch of directories will have to be renamed for them to work with this change.
  • if for any reason a directory has more than one feature inside it, there's no simple way to specify that using this mechanism.
  • this is mostly a mechanism for specifying dependencies in disguise, so we might as well go ahead and try to implement a proper, more generic dependency mechanism.

-1 from me for the above reasons.
Adding @jaustin to the thread.

@screamerbg
Copy link
Contributor Author

  • we're adding to the magic of TARGET_ and TOOLCHAIN_.

FEATURE_ is naturally extending TARGET_ and TOOLCHAIN_.

  • a bunch of directories will have to be renamed for them to work with this change.

4 directories will be renamed and these are the one generated by the uVisor compile script which creates the binary builds. It's a change to a single place to replace TARGET_ with FEATURE_

  • if for any reason a directory has more than one feature inside it, there's no simple way to specify that using this mechanism.

Has the same limitations as TARGET_/TOOLCHAIN_, which worked quite well so far. Can provide an example where this didn't work with TARGET_/TOOLCHAIN_?

  • this is mostly a mechanism for specifying dependencies in disguise, so we might as well go ahead and try to implement a proper, more generic dependency mechanism.

Can't think of more obvious mechanism that a folder starting with a FEATURE_ is ignored/not-built unless feature is defined by the target. It is absolutely consistent with the build logic we've implemented for the past 4 years on mbed.

And last but not least this allows the config system to extend features in the main application and add a feature even if not provided by the target. For example a shield BLE library (CSR, ST..) can have config which extends the target features with 'BLE' which triggers the inclusion of the mbed BLE API.

@bogdanm
Copy link
Contributor

bogdanm commented Jun 8, 2016

OK, after further discussions, I understand the purpose of this PR better. It is not supposed to be a replacement for a dependency system, as I originally read it, but rather to support a way to define various capabilities provided by targets and libraries (still to come). This looks like a good step in that direction with the tools that we have, so I'll change my vote to a +1.

bogdanm pushed a commit that referenced this pull request Jun 8, 2016
"features" will make more sense in the context of the feature support
that we'll introduce later (related to #1878). Plus, "device_has" is
arguably a better description of the actual content of this key.
@0xc0170
Copy link
Contributor

0xc0170 commented Jun 9, 2016

Please rebase

@sg- sg- mentioned this pull request Jun 9, 2016
@0xc0170 0xc0170 merged commit e7c3c88 into ARMmbed:master Jun 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants