Skip to content

mbed_app.json update does not mark build as dirty #2073

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
janjongboom opened this issue Jun 30, 2016 · 10 comments
Closed

mbed_app.json update does not mark build as dirty #2073

janjongboom opened this issue Jun 30, 2016 · 10 comments

Comments

@janjongboom
Copy link
Contributor

  1. Build a project.
  2. Now change something in mbed_app.json

Expected: project gets rebuild

Actual: project does not rebuild. Need manually clean for changes to be picked up.

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 30, 2016

cc @bogdanm

@bogdanm
Copy link
Contributor

bogdanm commented Jun 30, 2016

Interesting, thank you. Currently, mbed_config.h is an implicit dependency for every source file that we compile (since the compiler includes it automatically using --preinclude), so if the configuration changes, the whole project needs to be rebuilt (which doesn't happen currently because we don't check at all if mbed_config.h changes). The only alternative to always rebuilding all the source files is for sources that actually need the configuration to include it explicitly, which is something we can probably consider. --preinclude is used currently for convenience (so that code has direct access to the configuration without having to include any headers), but we can change that if needed.

So, to recap, there are two ways to change this:

  1. check if mbed_config.h changes between compiles (using hash sums) and recompile everything if it does (keeps the current --preinclude behaviour (convenience)).
  2. require the code that needs configuration data to include mbed_config.h explicitly (removes the curent --preinclude behaviour).

Short term, 1 will work, but I'm wondering if 2 might be a better solution long term. @screamerbg @sg-, thoughts?

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 30, 2016

I was thinking about 2nd previously as --preinclude is convenient, but we found few shortcomings (iar asm preinclude as an example). The biggest disadvantage of the 2nd option is that we would need to add ìnclusion of mbed_config.h almost everywhere (I disregard including it in mbed.h, as that is for the application, but we need it within mbed SDK or other modules, in C or possibly asm). It is acceptable and should be done now now.

I would go for 2nd. Include it in the mbed.h for apps. Include mbed_config.h where it is required as it should be within mbed modules/repos. This would mean that config header should be present even though is empty, or at least protected via macro like MBED_CONFIG_ACTIVE (sounds better to go with former - the header always present).

@theotherjimmy
Copy link
Contributor

What @0xc0170 Outlines sounds good to me. I'm going to start on the implementation of it. @bogdanm @0xc0170 , any thoughts before I do?

@bogdanm
Copy link
Contributor

bogdanm commented Jul 11, 2016

@theotherjimmy: that sounds just about right to me. To recap:

  1. mbed_config.h is going to be automatically included by mbed.h
  2. if HAL or other sources that don't include mbed.h need to access the configuration data, they'll need to include mbed_config.h manually.
  3. if mbed_config.h doesn't exist for whatever reason, it'll be automatically created by the build system (as an empty file).
  1. above might be a breaking change for some of our sources, but I think we need to go ahead and implement this anyway.

@bogdanm
Copy link
Contributor

bogdanm commented Jul 12, 2016

@theotherjimmy: let me know if you're already working on this; if not, I'll implement it.

@theotherjimmy
Copy link
Contributor

@bogdanm I have not started yet, I got sidetracked.

@bogdanm
Copy link
Contributor

bogdanm commented Jul 12, 2016

OK, thank you. I'll work on this then.

bogdanm pushed a commit that referenced this issue 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.
bogdanm pushed a commit that referenced this issue 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.
@ciarmcom
Copy link
Member

ciarmcom commented Aug 1, 2016

ARM Internal Ref: IOTMORF-147

@sg- sg- removed the mirrored label Aug 12, 2016
@bridadan
Copy link
Contributor

bridadan commented Jan 5, 2017

Closing as it looks like #2177 fixed this.

@bridadan bridadan closed this as completed Jan 5, 2017
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

No branches or pull requests

7 participants