uARM: Fix deprecate warning printing wrongly for GCC_ARM/IAR toolchain build #12198

rajkan01 · 2020-01-07T15:19:23Z

Summary of changes

Mbed OS build tool is wrongly printing uARM toolchain deprecation warning when building with GCC_ARM toolchain, so added the toolchain check before appending warning message.

Impact of changes

Migration actions required

Documentation

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

@evedon @hugueskamba

ciarmcom · 2020-01-07T16:00:39Z

@rajkan01, thank you for your changes.
@hugueskamba @evedon @ARMmbed/mbed-os-tools @ARMmbed/mbed-os-maintainers please review.

0xc0170 · 2020-01-08T10:18:46Z

CI started

mbed-ci · 2020-01-08T11:19:42Z

Test run: SUCCESS

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

0xc0170 · 2020-01-08T11:52:04Z

@ARMmbed/mbed-os-tools Please review. If approved, this is ready for integration

0xc0170 · 2020-01-13T09:41:34Z

@ARMmbed/mbed-os-tools Please review. If approved, this is ready for integration

Quick review here 👍

hugueskamba · 2020-01-13T11:22:52Z

tools/build_api.py

-                 end_warnings.append(UARM_TOOLCHAIN_WARNING)
+            if toolchain_name == "uARM":
+                end_warnings.append(UARM_TOOLCHAIN_WARNING)
+            elif toolchain_name == "ARMC6" and target.default_toolchain == "uARM":                


Why are you checking for target.default_toolchain?
Also isn't there an opportunity do so something like:

if ( toolchain_name in ["uARM", "ARMC5", "ARMC6"] and target.default_toolchain == "uARM" ): end_warnings.append(UARM_TOOLCHAIN_WARNING)

(check if the syntax is correct)

For me, this logic has become too complex to maintain. Please can you take the opportunity to refactor this into a sequence of 'if' statements at the same level, rather than the current complex arrangement.

Why are you checking for target.default_toolchain?
Also isn't there an opportunity do so something like:
if (
toolchain_name in ["uARM", "ARMC5", "ARMC6"]
and target.default_toolchain == "uARM"
):
end_warnings.append(UARM_TOOLCHAIN_WARNING)

(check if the syntax is correct)

The reason for checking target.default_toolchain is, if the target tries to build with ARM(ARMC5 or ARMC6) toolchain but the target.default_toolchain is configured to uARM then internally build tool use ARM with Small library.

So code snapshot will not work for all the target, such as LPC11C24 has uARM in the supported_toolchain but there is no target.default_toolchain.

Thanks.
Please refactor the code to avoid repetitions and add comment where an explanation is needed.

mark-edgeworth · 2020-01-13T14:05:21Z

tools/build_api.py

-                 end_warnings.append(UARM_TOOLCHAIN_WARNING)
+            if toolchain_name == "uARM":
+                end_warnings.append(UARM_TOOLCHAIN_WARNING)
+            elif toolchain_name == "ARMC6" and target.default_toolchain == "uARM":                


For me, this logic has become too complex to maintain. Please can you take the opportunity to refactor this into a sequence of 'if' statements at the same level, rather than the current complex arrangement.

mark-edgeworth · 2020-01-13T14:06:24Z

tools/toolchains/arm.py

@@ -44,6 +44,13 @@
    "please visit https://os.mbed.com/docs/mbed-os/latest/reference/using-small-c-libraries.html"
 )

+UARM_DEFAULT_TOOLCHAIN_WARNING = (
+    "Warning: We noticed that this target default_toolchain overrides --toolchain option with uARM Toolchain. "
+    "We are deprecating the use of uARM Toolchain. "


Suggested change

"We are deprecating the use of uARM Toolchain. "

"We are deprecating the use of the uARM Toolchain. "

mark-edgeworth · 2020-01-13T14:16:05Z

tools/toolchains/arm.py

@@ -44,6 +44,13 @@
    "please visit https://os.mbed.com/docs/mbed-os/latest/reference/using-small-c-libraries.html"
 )

+UARM_DEFAULT_TOOLCHAIN_WARNING = (
+    "Warning: We noticed that this target default_toolchain overrides --toolchain option with uARM Toolchain. "


Suggested change

"Warning: We noticed that this target default_toolchain overrides --toolchain option with uARM Toolchain. "

"Warning: We noticed that this target overrides default_toolchain with the uARM Toolchain (using --toolchain). "

0xc0170 · 2020-01-15T14:08:56Z

CI restarted

mbed-ci · 2020-01-15T15:02:21Z

Test run: SUCCESS

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

hugueskamba · 2020-01-15T15:34:13Z

tools/toolchains/arm.py

-    "Warning: We noticed that you are using uARM Toolchain. "
-    "We are deprecating the use of uARM Toolchain. "
+    "Warning: We noticed that you are using uARM Toolchain either via --toolchain command line or default_toolchain option. "
+    "We are deprecating the use of the uARM Toolchain. "
    "For more information on how to use the ARM toolchain with small C libraries, "
    "please visit https://os.mbed.com/docs/mbed-os/latest/reference/using-small-c-libraries.html"
 )


What happened to UARM_DEFAULT_TOOLCHAIN_WARNING that you had before?

I have combined into existing UARM_TOOLCHAIN_WARNING

hugueskamba · 2020-01-15T15:37:30Z

tools/build_api.py

        if not TOOLCHAIN_CLASSES[internal_tc_name].check_executable():
            search_path = TOOLCHAIN_PATHS[internal_tc_name] or "No path set"
            last_error = (
                "Could not find executable for {}.\n"
                "Currently set search path: {}"
            ).format(toolchain_name, search_path)
        else:
-            if toolchain_name == "uARM" or target.default_toolchain == "uARM":
-                 end_warnings.append(UARM_TOOLCHAIN_WARNING)
+            if toolchain_name in ["uARM", "ARMC5", "ARMC6"]:


You can use elif (condition): pass instead of else: if (condition): pass

The reason is, it will fail for GCC_ARM

It will not fail. It will work the exact same way. It is weird to have else and and single if.

until if toolchain_name in ["uARM", "ARMC5", "ARMC6", "GCC_ARM"]:, it will not work when I combined as if with else.

I was confused. I know understand that line 244 is only applicable to line 247 (To prevent adding the warning if a target has target.default_toolchain set to uARM even if say the GCC_ARM toolchain is used).
Perhaps line 244 should be moved to above 247 to be clear.

hugueskamba · 2020-01-15T15:44:48Z

tools/build_api.py

-            if toolchain_name == "uARM" or target.default_toolchain == "uARM":
-                 end_warnings.append(UARM_TOOLCHAIN_WARNING)
+            if toolchain_name in ["uARM", "ARMC5", "ARMC6"]:
+                if toolchain_name == "ARMC5":


Is the following more readable?

elif toolchain_name in ["uARM", "ARMC5", "ARMC6"]: (end_warnings.append(ARMC5_MIGRATION_WARNING) if toolchain_name == "ARMC5") (end_warnings.append(UARM_TOOLCHAIN_WARNING) if "uARM" in {toolchain_name, target.default_toolchain})

It's a bit difficult to readable "pass" followed if(condition)

You can at least do:
if "uARM" in {toolchain_name, target.default_toolchain}:

0xc0170 · 2020-01-15T17:58:11Z

CI started

mbed-ci · 2020-01-15T19:02:07Z

Test run: SUCCESS

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

hugueskamba

LGTM

0xc0170 · 2020-01-16T13:28:01Z

CI started

mbed-ci · 2020-01-16T14:27:29Z

Test run: SUCCESS

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

rajkan01 force-pushed the bug_fix_uARM_depreciate branch from 0619941 to 0c9d8b2 Compare January 7, 2020 15:25

ciarmcom requested review from evedon, hugueskamba and a team January 7, 2020 16:00

ciarmcom added the needs: review label Jan 7, 2020

rajkan01 changed the title ~~uARM: Fix depreciate warning printing wrongly for other toolchains~~ uARM: Fix deprecate warning printing wrongly for other toolchains Jan 7, 2020

uARM: Fix deprecate warning printing wrongly for other toolchains.

de3c737

rajkan01 force-pushed the bug_fix_uARM_depreciate branch from 0c9d8b2 to de3c737 Compare January 7, 2020 16:03

rajkan01 changed the title ~~uARM: Fix deprecate warning printing wrongly for other toolchains~~ uARM: Fix deprecate warning printing wrongly for GCC_ARM/IAR toolchain build Jan 7, 2020

0xc0170 added the release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 label Jan 8, 2020

0xc0170 approved these changes Jan 8, 2020

View reviewed changes

evedon approved these changes Jan 8, 2020

View reviewed changes

0xc0170 added needs: CI and removed needs: review labels Jan 8, 2020

0xc0170 added needs: review and removed needs: CI labels Jan 8, 2020

0xc0170 removed the release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 label Jan 10, 2020

hugueskamba reviewed Jan 13, 2020

View reviewed changes

mark-edgeworth suggested changes Jan 13, 2020

View reviewed changes

Refactor the code and incorporated the review comment

816516e

0xc0170 requested a review from mark-edgeworth January 15, 2020 14:02

0xc0170 added the release-version: 6.0.0-alpha-2 Second pre-release version of 6.0.0 label Jan 15, 2020

hugueskamba reviewed Jan 15, 2020

View reviewed changes

mark-edgeworth approved these changes Jan 15, 2020

View reviewed changes

Refactor condition check

f5230cd

Refactor the if condition check

434f50e

hugueskamba approved these changes Jan 16, 2020

View reviewed changes

0xc0170 added needs: CI and removed needs: review labels Jan 16, 2020

0xc0170 removed the needs: CI label Jan 16, 2020

0xc0170 merged commit cbaa9bd into ARMmbed:master Jan 16, 2020

	"We are deprecating the use of uARM Toolchain. "
	"We are deprecating the use of the uARM Toolchain. "

	"Warning: We noticed that this target default_toolchain overrides --toolchain option with uARM Toolchain. "
	"Warning: We noticed that this target overrides default_toolchain with the uARM Toolchain (using --toolchain). "

uARM: Fix deprecate warning printing wrongly for GCC_ARM/IAR toolchain build #12198

uARM: Fix deprecate warning printing wrongly for GCC_ARM/IAR toolchain build #12198

Uh oh!

Conversation

rajkan01 commented Jan 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

ciarmcom commented Jan 7, 2020

Uh oh!

0xc0170 commented Jan 8, 2020

Uh oh!

mbed-ci commented Jan 8, 2020

Uh oh!

0xc0170 commented Jan 8, 2020

Uh oh!

0xc0170 commented Jan 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Jan 15, 2020

Uh oh!

mbed-ci commented Jan 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Jan 15, 2020

Uh oh!

mbed-ci commented Jan 15, 2020

Uh oh!

hugueskamba left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Jan 16, 2020

Uh oh!

mbed-ci commented Jan 16, 2020

Uh oh!

Uh oh!

rajkan01 commented Jan 7, 2020 •

edited

Loading