Skip to content

Change how the config system is used #2162

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

Closed
wants to merge 1 commit into from
Closed

Conversation

bogdanm
Copy link
Contributor

@bogdanm bogdanm commented Jul 13, 2016

This commit address the issue presented in
#2073. Previously, mbed_config.h
was automatically included by the toolchain. With this change, all the
sources that need "mbed_config.h" need to include it manually. However,
"mbed.h" now includes "mbed_config.h", so configuration data is
automatically available to sources that include "mbed.h".

With this change, even if there's no configuration data in the project
being built, an empty "mbed_config.h" is generated by the build system.

This change looks big, but most of it is an almost complete reversal of
#1975.

This is likely a breaking change, and needs to be properly
communicated.

This commit address the issue presented in
#2073. Previously, mbed_config.h
was automatically included by the toolchain. With this change, all the
sources that need "mbed_config.h" need to include it manually. However,
"mbed.h" now includes "mbed_config.h", so configuration data is
automatically available to sources that include "mbed.h".

With this change, even if there's no configuration data in the project
being built, an empty "mbed_config.h" is generated by the build system.

This change looks big, but most of it is an almost complete reversal of
#1975.

This is likely a breaking change, and needs to be properly
communicated.
@bogdanm
Copy link
Contributor Author

bogdanm commented Jul 13, 2016

Up for discussion for now, needs a lot more testing. @theotherjimmy @0xc0170 (Martin, please take a look at the exporter changes).

@bogdanm
Copy link
Contributor Author

bogdanm commented Jul 13, 2016

Cc @screamerbg

@theotherjimmy
Copy link
Contributor

looks fine to me. I'll test some things with it in a bit.

@screamerbg
Copy link
Contributor

@bogdanm 2 quick questions:

  • if timestamp of mbed_config.h is changed every time even though the contents are identical, then this would trip the need_update() and force full build every time. Could we have logic to check whether the contents are identical and skip writing it?
  • not sure whether we want to -I the build dir. perhaps have a separate folder "config" for example and include only that one?

@bogdanm
Copy link
Contributor Author

bogdanm commented Jul 14, 2016

@screamerbg:

  1. the current implementation does exactly what you suggest, check https://github.com/mbedmicro/mbed/pull/2162/files#diff-486f048c4c5775670840b518bf8b0ad5R949.
  2. if there's a problem with adding the build dir as an include directory, then sure, we can put the config in another directory.

@bogdanm
Copy link
Contributor Author

bogdanm commented Jul 15, 2016

After a talk with @screamerbg, I will close this PR and provide one that fixes #2073 in a different way that keeps the existing --preinclude behaviour.

@bogdanm bogdanm closed this Jul 15, 2016
@bogdanm bogdanm deleted the explicit_mbed_config branch July 15, 2016 10:57
bogdanm pushed a commit that referenced this pull request Jul 15, 2016
This commit adds a check for configuration data changes. If a change in
configuration data is detected, all the sources in the tree are rebuilt.

This is a fix for #2073. #2162 was originally proposed as a fix, but it
was agreed that `--preinclude` is a more convenient way to include
configuration data, since it can be used to change the behaviour of
source files that don't include "mbed_config.h" directly, which is a big
advantage when importing 3rd party source trees. Compared to #2162, this
commit has the disadvantage of rebuilding all the source files if a
configuration change is detected, but it was agreed that the advantage
of using `--preinclude` outweighs the disadvantage of the increased
compilation time.
@bogdanm
Copy link
Contributor Author

bogdanm commented Jul 15, 2016

Replaced by #2177.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants