Skip to content

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

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.

Sign up for GitHub

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
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions tools/build.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
from tools.build_api import print_build_results
from tools.build_api import target_supports_toolchain
from tools.build_api import find_valid_toolchain
from tools.build_api import check_small_c_lib_support
from tools.notifier.term import TerminalNotifier
from tools.utils import argparse_filestring_type, args_error, argparse_many
from tools.utils import argparse_dir_not_parent
Expand Down Expand Up @@ -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_swap(target, toolchain_name)
end_warnings += [warning] if warning is not None else []

tt_id = "%s::%s" % (internal_tc_name, target_name)
if not target_supports_toolchain(target, toolchain_name):
# Log this later
Expand Down
27 changes: 27 additions & 0 deletions tools/build_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,11 @@

RELEASE_VERSIONS = ['2', '5']

SMALL_C_LIB_CHANGE_WARNING = (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"Warning: We noticed that you are using target.c_lib set to small. "
"As the {} target does not support a small C library for the {} toolchain, we are using the standard C library instead. "
)

def prep_report(report, target_name, toolchain_name, id_name):
"""Setup report keys

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

return toolchain_name


def check_small_c_lib_support(target, toolchain_name):
Copy link
Contributor

@mark-edgeworth mark-edgeworth Mar 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"""
Check if the small C lib is supported.
If it is not supported and the standard C library is supported, the later will be used instead.
Return a warning if the standard C lib will be used instead of the small C lib.
"""
if (
hasattr(target, "c_lib")
and target.c_lib.lower() == "small"
and hasattr(target, "supported_c_libs")
and toolchain_name not in target.supported_c_libs
):
if (
"small" not in target.supported_c_libs[toolchain_name]
and "std" in target.supported_c_libs[toolchain_name]
):
return SMALL_C_LIB_CHANGE_WARNING.format(target.name, toolchain_name)



def find_valid_toolchain(target, toolchain):
"""Given a target and toolchain, get the names for the appropriate
toolchain to use. The environment is also checked to see if the corresponding
Expand Down Expand Up @@ -250,6 +276,7 @@ def find_valid_toolchain(target, toolchain):
and "uARM" in {toolchain_name, target.default_toolchain}
):
end_warnings.append(UARM_TOOLCHAIN_WARNING)

return toolchain_name, internal_tc_name, end_warnings
else:
if last_error:
Expand Down
10 changes: 10 additions & 0 deletions tools/toolchains/mbed_toolchain.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1111,7 +1111,17 @@ def check_c_lib_supported(self, target, toolchain):
"target.default_lib is no longer supported, please use target.c_lib for C library selection."
)
if hasattr(target, "c_lib"):

target.c_lib = target.c_lib.lower()

if (
target.c_lib == "small"
and hasattr(target, "supported_c_libs")
and "small" not in target.supported_c_libs[toolchain]
and "std" in target.supported_c_libs[toolchain]
):
target.c_lib = "std"

if (
hasattr(target, "supported_c_libs") == False
or toolchain not in target.supported_c_libs
Expand Down