Remove hiding of config errors related to ROM regions. #12007

JammuKekkonen · 2019-12-03T08:36:06Z

Description

Currently any misconfiguration of bootloader feature will cause the build system
to just silently drop it and continue building which can lead to completed
builds of something the user didn't want to build in worst case and failing
builds after compilation (=wasted time) in the best.

Summary of change

Remove the silent recovery of the config errors and let the Exception flow through resulting in a meaningful error message to the user.

Pull request type

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@teetak01

Release Notes

Summary of changes

Impact of changes

Migration actions required

bulislaw

Makes sense, @madchutney please review

JanneKiiskila · 2019-12-03T11:04:43Z

Into next release, please.

0xc0170 · 2019-12-03T13:16:10Z

Into next release, please.

Means 6.0 in this case ? Or is this 5.15rc2?

0xc0170

Not having pass if you catch it, but why not at least to log the error if you catch.

As it is now, it would bomb up with ConfigException error, wouldn't it be better if this bombs rather with message like "config error in ROM regions" ?

JammuKekkonen · 2019-12-03T13:39:17Z

Not having pass if you catch it, but why not at least to log the error if you catch.

As it is now, it would bomb up with ConfigException error, wouldn't it be better if this bombs rather with message like "config error in ROM regions" ?

Running a misconfigured build with this results into the following. It does already give the error string (missing bootloader img file named missing_file.bin)

(mbed) /work/mbed-os-example-blinky [master]$ mbed compile
[mbed] Working path "/work/mbed-os-example-blinky" (program)
Building project mbed-os-example-blinky (K64F, GCC_ARM)
Scan: mbed-os-example-blinky
[ERROR] Bootloader /work/mbed-os-example-blinky/missing_file.bin not found
[mbed] ERROR: "/.virtualenvs/mbed/bin/python" returned error.
Code: 1
Path: "/work/mbed-os-example-blinky"
Command: "/.virtualenvs/mbed/bin/python -u /work/mbed-os-example-blinky/mbed-os/tools/make.py -t GCC_ARM -m K64F --source . --build ./BUILD/K64F/GCC_ARM"
Tip: You could retry the last command with "-v" flag for verbose output

madchutney

Fine for 5.15.1, 5.15.0 is in code freeze for OS and tools.

0xc0170 · 2019-12-03T13:55:34Z

Fine for 5.15.1, 5.15.0 is in code freeze for OS and tools.

Means this gets 6.0 label then

0xc0170 · 2019-12-03T13:56:02Z

CI started

JanneKiiskila · 2019-12-03T14:07:22Z

The error note is definitely good enough as is. We need it to 5.15.1 and 6.0.

0xc0170 · 2019-12-03T14:56:56Z

There's no 5.15.1 , so if this in 5.15, it should be in rc2.

TeroJaasko · 2019-12-03T17:02:02Z

Would it make sense to remove the error-hiding also from few places below the change in this PR:
https://github.com/ARMmbed/mbed-os/pull/12007/files#diff-9ae87a175893404591fd8c1fe20dd0cbR915
https://github.com/ARMmbed/mbed-os/pull/12007/files#diff-9ae87a175893404591fd8c1fe20dd0cbR934
https://github.com/ARMmbed/mbed-os/pull/12007/files#diff-9ae87a175893404591fd8c1fe20dd0cbR950

mbed-ci · 2019-12-03T18:25:25Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

0xc0170 · 2019-12-04T09:11:56Z

Would it make sense to remove the error-hiding also from few places below the change in this PR:

@JammuKekkonen Let us know how to proceed

TeroJaasko · 2019-12-04T10:55:17Z

According to some git archaeology, the exception ignoring appeared in 0c594a4 . @0xc0170, you know the area better, what do you think, should the change be done or not? For the change now in PR there is a test case, but don't know if there are for the other parts.

0xc0170 · 2019-12-04T11:30:24Z

@JammuKekkonen Can you clean up other 3 exception pass please ? I've reviewed them, they really silent the errors coming from the config system. Thanks @TeroJaasko.

Currently any misconfiguration of, for example, bootloader feature will cause the build system to just silently drop it and continue building which can lead to completed builds of something the user didn't want to build in worst case and failing builds after compilation (=wasted time) in the best.

JammuKekkonen · 2019-12-04T12:35:50Z

@0xc0170 removed the remaining 3 try-excepts

0xc0170 · 2019-12-05T14:28:03Z

@madchutney Should be better now?

0xc0170 · 2019-12-06T12:04:10Z

CI started

mbed-ci · 2019-12-06T12:43:48Z

Test run: FAILED

Summary: 3 of 4 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_build-ARM
jenkins-ci/mbed-os-ci_build-IAR
jenkins-ci/mbed-os-ci_build-GCC_ARM

0xc0170 · 2019-12-06T12:57:14Z

@JammuKekkonen Nice one, this now uncovers some devices that did not define device_name but enabled bootloader.

List of all targets with failures:

ARM_CM3DS_MPS2/
ARM_MPS2_M0/
ARM_MPS2_M0P/
ARM_MPS2_M3/
ARM_MPS2_M4/
ARM_MPS2_M7/
CY8CKIT_062S2_43012/
CY8CKIT_062_BLE/
CY8CKIT_062_WIFI_BT/
CY8CKIT_064S2_4343W/
CY8CPROTO_062S3_4343W/
CY8CPROTO_062_4343W/
CY8CPROTO_063_BLE/
CY8CPROTO_064_SB/
CYW943012P6EVB_01/
CYW9P62S1_43012EVB_01/
CYW9P62S1_43438EVB_01/
DISCO_L4R9I/
FUTURE_SEQUANA/
FUTURE_SEQUANA_M0/
FVP_MPS2_M0/
FVP_MPS2_M0P/
FVP_MPS2_M3/
FVP_MPS2_M4/
FVP_MPS2_M7/
GD32_E103VB/
GD32_F307VG/
GR_LYCHEE/
LPC54114/
MAX32600MBED/
MAX32620FTHR/
MAX32630FTHR/
MIMXRT1050_EVK/
NCS36510/
NRF51_DONGLE/
NUCLEO_WB55RG/
RDA5981X/
RZ_A1H/
SDT32620B/
TB_SENSE_1/
UNO_91H/
VK_RZ_A1H/
WIZWIKI_W7500/
WIZWIKI_W7500ECO/
WIZWIKI_W7500P/

I checked only few. For instance, TB_SENSE_1 does not enable bootloader but inherits from EFR32MG1P233F256GM48 - this one enables bootloader but does not have device_name defined - an error. Valid error.

Another error, this one does not seem to be valid (should be fixed here):
[ERROR] Bootloader not supported on this target. ROM start not found in targets.json. - for WIZWIKI_W7500 - this target has bootloader_supported set false. All should be OK.
This is the code I assume triggers the error:

This might need more work. pass should not be there , we should error if bootloader is requested (set to true) but can't be supported.

JanneKiiskila · 2019-12-08T20:16:12Z

We should fix what can be fixed, but mostly for now - just remove the bootloader is supported attribute, if it really isn't. The ROM_START is required, in my opinion though. If it's missing, it can't work/bootloader will not compile. But, it's often trivial to add. If that's the only thing that blocks it.

However, we could sort of consider those issues to be separate issues - the silent failure we have had previously is much worse.

0xc0170

This should fix it

0xc0170 · 2019-12-09T10:40:49Z

tools/toolchains/mbed_toolchain.py

@@ -895,62 +895,52 @@ def add_regions(self):
        """Add regions to the build profile, if there are any.
        """


Suggested change

"""

"""

if not getattr(self.target, "bootloader_supported", False):

return

Commit msg (explanation):

tools: regions only if bootloader is supported We removed catching and passing, we want to know for any misconfiguration if a bootloader is supported. Regions function should check if a bootloader is supported. In case not, just return. Otherwise we catch any error. This should help us to uncover missing regions or other config error (in case bootloader is enabled via bootloader_supported set to true).

I was not able to push to your branch . I tested this for WIZWIKI_W7500, it builds without an error. but target like TB_SENSE_1 fails with a proper error - bootloader can't be enabled due to configuration error.

I tried to catch proper config exception but it is not easy the way config is executed (not that clear what with/without bootloader should work), I found this check as the best one to do prior doing any region work (it is done in config anyway later but then we are deeply in already bootoader case).

IF we can test this commit, we should get reduce list of targets that are not supporting bootloader properly.

@madchutney please review

0xc0170 · 2019-12-09T13:00:01Z

@JanneKiiskila @JammuKekkonen is not responding (tried various IM), is he OoO today? I am wondering if I should create new PR as a replacement for this one (I can't commit to his branch)

bulislaw approved these changes Dec 3, 2019

View reviewed changes

0xc0170 requested a review from a team December 3, 2019 13:15

0xc0170 added the needs: review label Dec 3, 2019

0xc0170 reviewed Dec 3, 2019

View reviewed changes

madchutney approved these changes Dec 3, 2019

View reviewed changes

0xc0170 approved these changes Dec 3, 2019

View reviewed changes

0xc0170 added release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 needs: CI and removed needs: review labels Dec 3, 2019

0xc0170 added release-version: 5.15.0-rc2 and removed release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 labels Dec 3, 2019

0xc0170 added ready for merge and removed needs: CI labels Dec 4, 2019

0xc0170 added needs: work and removed ready for merge labels Dec 4, 2019

0xc0170 removed the needs: work label Dec 5, 2019

0xc0170 added the needs: review label Dec 5, 2019

0xc0170 approved these changes Dec 5, 2019

View reviewed changes

0xc0170 requested changes Dec 9, 2019

View reviewed changes

0xc0170 mentioned this pull request Dec 9, 2019

Config: fix bootloader config errors - propagate errors #12059

Merged

0xc0170 closed this Dec 9, 2019

		@@ -895,62 +895,52 @@ def add_regions(self):
		"""Add regions to the build profile, if there are any.
		"""

-        """
+         """
+         if not getattr(self.target, "bootloader_supported", False):
+            return

Remove hiding of config errors related to ROM regions. #12007

Remove hiding of config errors related to ROM regions. #12007

Uh oh!

Conversation

JammuKekkonen commented Dec 3, 2019

Description

Summary of change

Pull request type

Test results

Reviewers

Release Notes

Summary of changes

Impact of changes

Migration actions required

Uh oh!

bulislaw left a comment

Choose a reason for hiding this comment

Uh oh!

JanneKiiskila commented Dec 3, 2019

Uh oh!

0xc0170 commented Dec 3, 2019

Uh oh!

0xc0170 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JammuKekkonen commented Dec 3, 2019

Uh oh!

madchutney left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Dec 3, 2019

Uh oh!

0xc0170 commented Dec 3, 2019

Uh oh!

JanneKiiskila commented Dec 3, 2019

Uh oh!

0xc0170 commented Dec 3, 2019

Uh oh!

TeroJaasko commented Dec 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mbed-ci commented Dec 3, 2019

Test run: SUCCESS

Uh oh!

0xc0170 commented Dec 4, 2019

Uh oh!

TeroJaasko commented Dec 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Dec 4, 2019

Uh oh!

JammuKekkonen commented Dec 4, 2019

Uh oh!

0xc0170 commented Dec 5, 2019

Uh oh!

0xc0170 commented Dec 6, 2019

Uh oh!

mbed-ci commented Dec 6, 2019

Test run: FAILED

Uh oh!

0xc0170 commented Dec 6, 2019

Uh oh!

JanneKiiskila commented Dec 8, 2019

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

0xc0170 Dec 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 Dec 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 Dec 9, 2019

0xc0170 left a comment •

edited

Loading

TeroJaasko commented Dec 3, 2019 •

edited

Loading

TeroJaasko commented Dec 4, 2019 •

edited

Loading

0xc0170 Dec 9, 2019 •

edited

Loading

0xc0170 Dec 9, 2019 •

edited

Loading

0xc0170 commented Dec 9, 2019 •

edited

Loading