Fixing constant cleaning when compiling tests #2712
Conversation
|
@bridadan needs a conflict resolved. @AlessandroA bump for a review of the PR description |
There was a problem hiding this comment.
@bridadan Yes, we can now use --app-config for that purpose, so our workflow is not affected by this PR! LGTM 👍
This addresses the issue where building tests via test.py always triggered a clean build. This is because the mbed_config.h file was being deleted from the shared OS build to ensure that the correct config was always being used. However, this contanstly triggered a rebuild of the OS since the config file was not present. Due to the shared build, having multiple app configurations that could override the OS settings is not possible. For this reason, we now ignore app config files unless explicitly set via the command line option '--app-config'. Though there will now be two mbed_config.h files in the include path of the build, it shouldn't matter since the contents will be the same.
bridadan
force-pushed
the
test-building-and-config
branch
from
6934863
to
aeca796
Compare
|
Thanks @AlessandroA! @sg- rebased, re-triggering tests. /morph test |
Result: SUCCESSYour command has finished executing! Here's what you wrote!
Outputmbed Build Number: 926 All builds and test passed! |
|
👍 |
|
@mazimkhan Would you mind checking to see if the two Cambridge CI failures were due to this PR? Should this be blocked by them? |
It's not , those failures have been here few days ago. I'll talk to @mazimkhan to get details how to fix them Edit: those jobs are going to be updated today, that will be resolved. It's an issue elsewhere. |
|
retest uvisor |
This PR was spurred in response to ARMmbed/mbed-cli#344, however it has larger implications to make the implementation cleaner and to provide a more consistent experience when building tests.
Description
This addresses the issue where building tests via test.py always triggered
a clean build. This is because the mbed_config.h file was being deleted from
the shared OS build to ensure that the correct config was always being
used. However, this constantly triggered a rebuild of the OS since the
config file was not present.
Due to the shared build, having multiple app configurations that could
override the OS settings is not possible. For this reason, we now ignore
app config files unless explicitly set via the command line option
'--app-config'. Though there will now be two mbed_config.h files in the
include path of the build, it shouldn't matter since the contents will be
the same.
Status
READY
Migrations
If this PR changes any APIs or behaviors, give a short description of what API users should do when this PR is merged.
YES (potentially, pending feedback from @AlessandroA)
@AlessandroA from the conversation we had on #2397, it seemed like you were going to be using
mbed_app.jsonfiles in each test case folder to get configuration for tests. Would this PR break your tests? Or are you already using the--app_configoption formbed test? In that case, this PR shouldn't affect your workflow.Todos
Also, @sg- we discussed this offline but if you have a moment, would confirm the strategy I implemented here is consistent with what you had in mind?