Enable build with IAR when c_lib is set to small #12665

hugueskamba · 2020-03-20T15:32:50Z

Summary of changes

If "c_lib" is set to "small" and "targets.supported_c_libs.iar" only has "std" it means that it is not possible to build with the IAR toolchain.

This commits modifies the tools to allow building with the IAR toolchain even when "small" is specified by reverting to the standard C library and displaying a message to warn that the standard library is used instead.

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)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[X] Tests / results supplied as part of this PR

Reviewers

@evedon @rajkan01

ciarmcom · 2020-03-20T16:00:37Z

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

evedon · 2020-03-20T16:25:37Z

tools/toolchains/iar.py

-        self.check_c_lib_supported(target, "iar")
+        # Build with the standard C lib if c_lib is set to small as there
+        # is no small lib for the IAR toolchain
+        if target.c_lib.lower() != "small":


If c_lib is set to "small" for IAR, we want to check that "std" is supported and if so revert to using standard.

evedon · 2020-03-23T14:37:51Z

We should make this a generic feature.
If c_lib is set to small :

if small is present in supported_c_libs for the used toolchain then use it
else use standard

mark-edgeworth · 2020-03-24T09:26:30Z

tools/toolchains/iar.py

+        # the value of the c_lib attribute is used to print a warning if std C lib
+        # ends up being used instead of a small C lib as potentially requested.
+        target_c_lib_check = copy.copy(target)
+        if hasattr(target, "c_lib") and target.c_lib == "small":


Could use getattr() here to simplify this.

mark-edgeworth · 2020-03-24T11:39:10Z

tools/build_api.py

+
+            if (
+                toolchain_name == "IAR"
+                and target.c_lib.lower() == "small"


Since c_lib appears to be an optional attribute, need to use getattr() here to ensure that an exception is not thrown when it is not specified.

Pull request has been modified.

mark-edgeworth · 2020-03-24T14:30:08Z

tools/build_api.py

@@ -205,6 +210,27 @@ def get_toolchain_name(target, toolchain_name):

    return toolchain_name

+
+def check_small_c_lib_support(target, toolchain_name):


There are two issues with this function: 1. The 'target' argument is potentially modified inside the function (ie the function has a side-effect that is not easily seen from outside). The name of the function 'check...' implies that it simply checks something. When the function is called, the user does not know that the target arg will be modified (without studying the internals of the function). This kind of thing is really hard to maintain in future. 2. The function mixes up the concepts of checking for some condition, and reporting on that condition. This makes it hard to test (and there are no tests for this). The reporting should be done by the caller IMO.
To fix this: return a boolean to indicate if the check passes or fails. Move the target modification and the error message formation up to the caller. Rename the function (suggest 'is_small_c_lib_supported' or something like that).
It might be that fixing these issues makes this whole function redundant (the code could be in-line since it is only called once anyway.

mark-edgeworth · 2020-03-24T14:31:50Z

tools/build_api.py

@@ -52,6 +52,11 @@

 RELEASE_VERSIONS = ['2', '5']

+SMALL_C_LIB_CHANGE_WARNING = (


Separating the message format from its arguments doesn't really make sense here. Since this is used only once, at the print, it makes more sense to put the text template there.

mark-edgeworth · 2020-03-24T14:32:33Z

tools/build.py

@@ -177,6 +178,10 @@ def main():
            except NoValidToolchainException as e:
                print_end_warnings(e.end_warnings)
                args_error(parser, str(e))
+
+            warning = check_small_c_lib_support(target, toolchain_name)


Consider not separating this out into a function - see below.

Pull request has been modified.

madchutney · 2020-03-24T14:58:36Z

@hugueskamba Please do not force push, it makes it difficult to understand past comments without context.

mark-edgeworth

There are may existing review comments that have been lost due to force-push. Please look again at the comments and reimplement accordingly.
The tests that were originally here have been removed. Please reinstate.

hugueskamba · 2020-03-24T15:25:29Z

Closing this as the scope has changed.

hugueskamba · 2020-03-24T18:17:20Z

The following PR replaces the present PR:
#12691

hugueskamba mentioned this pull request Mar 20, 2020

Migrate NUCLEO_F303K8 to Mbed OS 5 baremetal #12663

Merged

ciarmcom requested review from evedon, rajkan01 and a team March 20, 2020 16:00

ciarmcom added the needs: review label Mar 20, 2020

evedon reviewed Mar 20, 2020

View reviewed changes

hugueskamba force-pushed the hk_iar_enable_c_lib_small_build branch from f4d874d to bc8aeb6 Compare March 21, 2020 00:14

mark-edgeworth previously requested changes Mar 24, 2020

View reviewed changes

mergify bot added needs: work and removed needs: review labels Mar 24, 2020

hugueskamba force-pushed the hk_iar_enable_c_lib_small_build branch from bc8aeb6 to a9445c9 Compare March 24, 2020 14:19

mark-edgeworth previously requested changes Mar 24, 2020

View reviewed changes

hugueskamba force-pushed the hk_iar_enable_c_lib_small_build branch from a9445c9 to 9e2d6c7 Compare March 24, 2020 14:43

hugueskamba force-pushed the hk_iar_enable_c_lib_small_build branch from 9e2d6c7 to e38be3a Compare March 24, 2020 15:13

Replace small C lib with std C lib if not supported for a given target

02bf3ca

hugueskamba force-pushed the hk_iar_enable_c_lib_small_build branch from e38be3a to 02bf3ca Compare March 24, 2020 15:15

mark-edgeworth suggested changes Mar 24, 2020

View reviewed changes

hugueskamba closed this Mar 24, 2020

mergify bot removed the needs: work label Mar 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Enable build with IAR when c_lib is set to small #12665

Enable build with IAR when c_lib is set to small #12665

Uh oh!

hugueskamba commented Mar 20, 2020

Uh oh!

ciarmcom commented Mar 20, 2020

Uh oh!

evedon Mar 20, 2020

Uh oh!

hugueskamba Mar 21, 2020

Uh oh!

evedon commented Mar 23, 2020

Uh oh!

mark-edgeworth Mar 24, 2020

Uh oh!

mark-edgeworth Mar 24, 2020

Uh oh!

mark-edgeworth Mar 24, 2020 •

edited

Loading

Uh oh!

mark-edgeworth Mar 24, 2020

Uh oh!

mark-edgeworth Mar 24, 2020

Uh oh!

madchutney commented Mar 24, 2020

Uh oh!

mark-edgeworth left a comment •

edited

Loading

Uh oh!

hugueskamba commented Mar 24, 2020

Uh oh!

hugueskamba commented Mar 24, 2020 •

edited

Loading

Uh oh!

Uh oh!

		@@ -205,6 +210,27 @@ def get_toolchain_name(target, toolchain_name):

		return toolchain_name


		def check_small_c_lib_support(target, toolchain_name):

		@@ -52,6 +52,11 @@

		RELEASE_VERSIONS = ['2', '5']

		SMALL_C_LIB_CHANGE_WARNING = (

Enable build with IAR when c_lib is set to small #12665

Enable build with IAR when c_lib is set to small #12665

Uh oh!

Conversation

hugueskamba commented Mar 20, 2020

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

ciarmcom commented Mar 20, 2020

Uh oh!

evedon Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

hugueskamba Mar 21, 2020

Choose a reason for hiding this comment

Uh oh!

evedon commented Mar 23, 2020

Uh oh!

mark-edgeworth Mar 24, 2020

Choose a reason for hiding this comment

Uh oh!

mark-edgeworth Mar 24, 2020

Choose a reason for hiding this comment

Uh oh!

mark-edgeworth Mar 24, 2020 • 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!

mark-edgeworth Mar 24, 2020

Choose a reason for hiding this comment

Uh oh!

mark-edgeworth Mar 24, 2020

Choose a reason for hiding this comment

Uh oh!

madchutney commented Mar 24, 2020

Uh oh!

mark-edgeworth 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!

hugueskamba commented Mar 24, 2020

Uh oh!

hugueskamba commented Mar 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mark-edgeworth Mar 24, 2020 •

edited

Loading

mark-edgeworth left a comment •

edited

Loading

hugueskamba commented Mar 24, 2020 •

edited

Loading