Skip to content

Allow the use of Mbed Studio's version of ARMC6 #10114

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

Merged
merged 7 commits into from
Mar 23, 2019
Merged

Allow the use of Mbed Studio's version of ARMC6 #10114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9ac08e6
Whitespace clean up
bridadan Mar 19, 2019
bc06c53
Conditionally enable --ide=mbed from ARMC6 based on compiler version.
bridadan Mar 19, 2019
b135709
Remove stray prints and whitespace
bridadan Mar 19, 2019
c4a9fb3
Test detection of Mbed Studio version of ARMC6
bridadan Mar 19, 2019
9824c3c
Clean up some whitespace
bridadan Mar 19, 2019
6e629de
Version check the compiler in all build functions
bridadan Mar 19, 2019
e382b50
Add comments about the proper use of specific ARMC6 arguments
bridadan Mar 21, 2019
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
17 changes: 10 additions & 7 deletions tools/build_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ def transform_release_toolchains(target, version):
if version == '5':
if 'ARMC5' in target.supported_toolchains:
return ['ARMC5', 'GCC_ARM', 'IAR']
else:
else:
return ['ARM', 'ARMC6', 'GCC_ARM', 'IAR']
else:
return target.supported_toolchains
Expand Down Expand Up @@ -356,19 +356,19 @@ def prepare_toolchain(src_paths, build_dir, target, toolchain_name,
# If the configuration object was not yet created, create it now
config = config or Config(target, src_paths, app_config=app_config)
target = config.target

if not target_supports_toolchain(target, toolchain_name):
raise NotSupportedException(
"Target {} is not supported by toolchain {}".format(
target.name, toolchain_name))

selected_toolchain_name = get_toolchain_name(target, toolchain_name)

#If a target supports ARMC6 and we want to build UARM with it,
#If a target supports ARMC6 and we want to build UARM with it,
#then set the default_toolchain to uARM to link AC6 microlib.
if(selected_toolchain_name == "ARMC6" and toolchain_name == "uARM"):
target.default_toolchain = "uARM"
toolchain_name = selected_toolchain_name
toolchain_name = selected_toolchain_name

try:
cur_tc = TOOLCHAIN_CLASSES[toolchain_name]
Expand Down Expand Up @@ -607,6 +607,7 @@ def build_library(src_paths, build_path, target, toolchain_name,
src_paths, build_path, target, toolchain_name, macros=macros,
clean=clean, jobs=jobs, notify=notify, app_config=app_config,
build_profile=build_profile, ignore=ignore)
toolchain.version_check()

# The first path will give the name to the library
if name is None:
Expand Down Expand Up @@ -781,6 +782,7 @@ def build_lib(lib_id, target, toolchain_name, clean=False, macros=None,
src_paths, tmp_path, target, toolchain_name, macros=macros,
notify=notify, build_profile=build_profile, jobs=jobs, clean=clean,
ignore=ignore)
toolchain.version_check()

notify.info("Building library %s (%s, %s)" %
(name.upper(), target.name, toolchain_name))
Expand Down Expand Up @@ -871,7 +873,7 @@ def build_mbed_libs(target, toolchain_name, clean=False, macros=None,

selected_toolchain_name = get_toolchain_name(target, toolchain_name)

#If a target supports ARMC6 and we want to build UARM with it,
#If a target supports ARMC6 and we want to build UARM with it,
#then set the default_toolchain to uARM to link AC6 microlib.
if(selected_toolchain_name == "ARMC6" and toolchain_name == "uARM"):
target.default_toolchain = "uARM"
Expand Down Expand Up @@ -925,6 +927,7 @@ def build_mbed_libs(target, toolchain_name, clean=False, macros=None,
toolchain = prepare_toolchain(
[""], tmp_path, target, toolchain_name, macros=macros, notify=notify,
build_profile=build_profile, jobs=jobs, clean=clean, ignore=ignore)
toolchain.version_check()

config = toolchain.config
config.add_config_files([MBED_CONFIG_FILE])
Expand Down Expand Up @@ -1118,10 +1121,10 @@ def mcu_toolchain_matrix(verbose_html=False, platform_filter=None,
unique_supported_toolchains = get_unique_supported_toolchains(
release_targets)
#Add ARMC5 column as well to the matrix to help with showing which targets are in ARMC5
#ARMC5 is not a toolchain class but yet we use that as a toolchain id in supported_toolchains in targets.json
#ARMC5 is not a toolchain class but yet we use that as a toolchain id in supported_toolchains in targets.json
#capture that info in a separate column
unique_supported_toolchains.append('ARMC5')

prepend_columns = ["Target"] + ["mbed OS %s" % x for x in RELEASE_VERSIONS]

# All tests status table print
Expand Down
33 changes: 29 additions & 4 deletions tools/test/toolchains/api_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations
limitations
"""

import sys
Expand Down Expand Up @@ -84,16 +84,41 @@ def test_armc5_version_check(_run_cmd):
def test_armc6_version_check(_run_cmd):
set_targets_json_location()
notifier = MockNotifier()
print(TARGET_MAP["K64F"])
toolchain = TOOLCHAIN_CLASSES["ARMC6"](TARGET_MAP["K64F"], notify=notifier)
print(toolchain)
_run_cmd.return_value = ("""
Product: ARM Compiler 6.11 Professional
Component: ARM Compiler 6.11
Tool: armclang [5d3b4200]
""", "", 0)

toolchain.version_check()
assert notifier.messages == []
assert notifier.messages == []
assert not toolchain.is_mbed_studio_armc6

_run_cmd.return_value = ("""
armclang: error: Failed to check out a license.
The provided license does not enable these tools.
Information about this error is available at: http://ds.arm.com/support/lic56/m5
General licensing information is available at: http://ds.arm.com/support/licensing/
If you need further help, provide this complete error report to your supplier or [email protected].
- ARMLMD_LICENSE_FILE: unset
- LM_LICENSE_FILE: unset
- ARM_TOOL_VARIANT: unset
- ARM_PRODUCT_PATH: unset
- Product location: C:\MbedStudio\tools\ac6\sw\mappings
- Toolchain location: C:\MbedStudio\tools\ac6\bin
- Selected tool variant: product
- Checkout feature: mbed_armcompiler
- Feature version: 5.0201810
- Flex error code: -5
Product: ARM Compiler 6.11 for Mbed Studio
Component: ARM Compiler 6.11
Tool: armclang [5d3b3c00]
""", "", 0)

toolchain.version_check()
assert notifier.messages == []
assert toolchain.is_mbed_studio_armc6

@patch('tools.toolchains.iar.run_cmd')
def test_iar_version_check(_run_cmd):
Expand Down
90 changes: 79 additions & 11 deletions tools/toolchains/arm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,17 @@
from builtins import str # noqa: F401

import re
import os
from copy import copy
from os.path import join, dirname, splitext, basename, exists, isfile
from os.path import join, dirname, splitext, basename, exists, isfile, split
from os import makedirs, write, remove
from tempfile import mkstemp
from shutil import rmtree
from distutils.version import LooseVersion

from tools.targets import CORE_ARCH
from tools.toolchains.mbed_toolchain import mbedToolchain, TOOLCHAIN_PATHS
from tools.utils import mkdir, NotSupportedException, run_cmd
from tools.utils import mkdir, NotSupportedException, ToolException, run_cmd


class ARM(mbedToolchain):
Expand All @@ -44,6 +45,7 @@ class ARM(mbedToolchain):
"Cortex-M7", "Cortex-M7F", "Cortex-M7FD", "Cortex-A9"
]
ARMCC_RANGE = (LooseVersion("5.06"), LooseVersion("5.07"))
ARMCC_PRODUCT_RE = re.compile(b"Product: (.*)")
ARMCC_VERSION_RE = re.compile(b"Component: ARM Compiler (\d+\.\d+)")

@staticmethod
Expand Down Expand Up @@ -103,11 +105,20 @@ def __init__(self, target, notify=None, macros=None,

self.SHEBANG += " --cpu=%s" % cpu

self.product_name = None

def version_check(self):
stdout, _, retcode = run_cmd([self.cc[0], "--vsn"], redirect=True)
# The --ide=mbed removes an instability with checking the version of
# the ARMC6 binary that comes with Mbed Studio.
# NOTE: the --ide=mbed argument is only for use with Mbed OS
stdout, _, retcode = run_cmd(
[self.cc[0], "--vsn", "--ide=mbed"],
redirect=True
)
msg = None
min_ver, max_ver = self.ARMCC_RANGE
match = self.ARMCC_VERSION_RE.search(stdout.encode("utf-8"))
output = stdout.encode("utf-8")
match = self.ARMCC_VERSION_RE.search(output)
if match:
found_version = LooseVersion(match.group(1).decode("utf-8"))
else:
Expand All @@ -132,6 +143,19 @@ def version_check(self):
"severity": "WARNING",
})

msg = None
match = self.ARMCC_PRODUCT_RE.search(output)
if match:
self.product_name = match.group(1).decode("utf-8")
else:
self.product_name = None

if not match or len(match.groups()) != 1:
msg = (
"Could not detect product name: defaulting to professional "
"version of ARMC6"
)

def _get_toolchain_labels(self):
if getattr(self.target, "default_toolchain", "ARM") == "uARM":
return ["ARM", "ARM_MICRO"]
Expand Down Expand Up @@ -275,7 +299,7 @@ def correct_scatter_shebang(self, scatter_file, cur_dir_name=None):

return new_scatter

def link(self, output, objects, libraries, lib_dirs, scatter_file):
def get_link_command(self, output, objects, libraries, lib_dirs, scatter_file):
base, _ = splitext(output)
map_file = base + ".map"
args = ["-o", output, "--info=totals", "--map", "--list=%s" % map_file]
Expand All @@ -294,6 +318,13 @@ def link(self, output, objects, libraries, lib_dirs, scatter_file):
link_files = self.get_link_file(cmd[1:])
cmd = [cmd_linker, '--via', link_files]

return cmd

def link(self, output, objects, libraries, lib_dirs, scatter_file):
cmd = self.get_link_command(
output, objects, libraries, lib_dirs, scatter_file
)

self.notify.cc_verbose("Link: %s" % ' '.join(cmd))
self.default_cmd(cmd)

Expand All @@ -304,12 +335,15 @@ def archive(self, objects, lib_path):
param = objects
self.default_cmd([self.ar, '-r', lib_path] + param)

def get_binary_commands(self, bin_arg, bin, elf):
return [self.elf2bin, bin_arg, '-o', bin, elf]

def binary(self, resources, elf, bin):
_, fmt = splitext(bin)
# On .hex format, combine multiple .hex files (for multiple load
# regions) into one
bin_arg = {".bin": "--bin", ".hex": "--i32combined"}[fmt]
cmd = [self.elf2bin, bin_arg, '-o', bin, elf]
cmd = self.get_binary_commands(bin_arg, bin, elf)

# remove target binary file/path
if exists(bin):
Expand Down Expand Up @@ -337,7 +371,6 @@ def redirect_symbol(source, sync, build_dir):
write(handle, "RESOLVE %s AS %s\n" % (source, sync))
return "--edit=%s" % filename


class ARM_STD(ARM):

OFFICIALLY_SUPPORTED = True
Expand All @@ -359,7 +392,7 @@ def __init__(
build_profile=build_profile
)
if int(target.build_tools_metadata["version"]) > 0:
#check only for ARMC5 because ARM_STD means using ARMC5, and thus
#check only for ARMC5 because ARM_STD means using ARMC5, and thus
# supported_toolchains must include ARMC5
if "ARMC5" not in target.supported_toolchains:
raise NotSupportedException(
Expand Down Expand Up @@ -546,12 +579,21 @@ def __init__(self, target, *args, **kwargs):
self.ar = join(TOOLCHAIN_PATHS["ARMC6"], "armar")
self.elf2bin = join(TOOLCHAIN_PATHS["ARMC6"], "fromelf")

# Adding this for safety since this inherits the `version_check` function
# but does not call the constructor of ARM_STD, so the `product_name` variable
# is not initialized.
self.product_name = None

def _get_toolchain_labels(self):
if getattr(self.target, "default_toolchain", "ARM") == "uARM":
return ["ARM", "ARM_MICRO", "ARMC6"]
else:
return ["ARM", "ARM_STD", "ARMC6"]

@property
def is_mbed_studio_armc6(self):
return self.product_name and "Mbed Studio" in self.product_name
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not convinced that mbed os tools should have special logic based on a single product. Not sure if there is a better way of fixing it but it should be worth double checking before merging it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It also limit options to make any changes in product name if needed. As old mbed os versions will be already released.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not convinced that mbed os tools should have special logic based on a single product.

Considering the compiler has special behavior in this single product, I don't know if we have much of a choice 😄. The license shipped with Mbed Studio must be supplied to the compiler shipped with Mbed Studio in order to build anything. However if you have any alternatives/suggestions do let me know.

It also limit options to make any changes in product name if needed. As old mbed os versions will be already released.

This is the only way I know of to identify this build of the compiler. As you can see, the output of armclang --vsn is quite similar between the different builds:

ARMC6 Professional:

Product: ARM Compiler 6.11 Professional
Component: ARM Compiler 6.11
Tool: armclang [5d3b4200]

ARMC6 from Mbed Studio:

<removed license error for brevity>
Product: ARM Compiler 6.11 for Mbed Studio
Component: ARM Compiler 6.11
Tool: armclang [5d3b3c00]

And we shouldn't be switching this behavoir based on the commit since that will change with new releases of the compiler.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @arekzaluski, managing specific cases like this is problematic and prone to breaking. For example, we plan to move the installation directory of the tools in the next release of Mbed Studio.
At a higher level, requiring Mbed Studio to be installed for this functionality to work makes little sense IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thegecko I understand your concern, it definitely reduces the flexibility of how Mbed Studio is internally organized.

For example, we plan to move the installation directory of the tools in the next release of Mbed Studio.

This isn't immediately a problem for this implementation, as it only requires the ac6-license.dat file to be placed in any parent directory of the compiler binary.

At a higher level, requiring Mbed Studio to be installed for this functionality to work makes little sense IMO.

Mbed Studio is only required to be installed if you wish to take advantage of the development license and corresponding compiler provided by Mbed Studio. You can of course still use the professional versions of the Arm Compiler 6 and license.

Another option is that I completely remove all of the "license detection" features at the expense of user experience. This would mean the user would now be responsible for the following:

  1. Find the ac6-license.dat file in the Mbed Studio installation
  2. Add the ARMLMD_LICENSE_FILE environment variable to their shell and set it to the path to the ac6-license.dat file.

We can of course document all of this, but that also means we need to keep this documentation up to date whenever the structure of Mbed Studio changes.

If the license detection feature is something that we need to ship though, I really need an alternative suggestion from your team. I'll happily parse a file to get the exact location. Maybe you could place a dedicated file next to the armc6 binaries? That file could point to the correct location of the license.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking about it and I think that the best way would be to remove license detection feature from mbed-os tools. Instead, it should be applications (Mbed Studio, mbed-cli, possible other in the future) that controls specific compilation behavior. Only they know location of all of the files.

At the moment Mbed Studio handles cases when ARMLMD_LICENSE_FILE env variable is set or/and AC6 compiler is installed on user's system. It simply overrides it and set its own path for both license and compiler location. It overrides it only for a single build process so user's environment variables are not changed.

I believe that the best way forward is to add similar logic in mbed-cli. For example installers of mbed-cli can be shipped with ARMC6 and valid license. Installer knows installation location so it can ask user at the end of installation if env variables should be updated or not.
Of course it should be also well documented for users that do not use installers to install mbed-cli.

@bridadan What do you think about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like the least contentious way forward, so I'll go with that. Moving this functionality to the installers doesn't sound like a bad idea either 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

@bridadan Checking since this PR has passed CI checks.

I'm assuming the above will be planned for 5.13, and not squeezed into 5.12.0, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on the feedback from the Mbed Studio folks it sounds like they are not comfortable with this PR going in as-is for 5.12, so I think I have to make the tool changes now. The installers will not be ready though for sometime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For what its worth I have the changes almost ready. I don't have access to a mac to test though.


def parse_dependencies(self, dep_path):
return mbedToolchain.parse_dependencies(self, dep_path)

Expand All @@ -565,21 +607,27 @@ def get_config_option(self, config_header):
return ["-include", config_header]

def get_compile_options(self, defines, includes, for_asm=False):

opts = ['-D%s' % d for d in defines]

if self.RESPONSE_FILES:
opts += ['@{}'.format(self.get_inc_file(includes))]
else:
opts += ["-I%s" % i for i in includes if i]

config_header = self.get_config_header()
if config_header:
opts.extend(self.get_config_option(config_header))
if for_asm:
return [
opts = [
"--cpreproc",
"--cpreproc_opts=%s" % ",".join(self.flags['common'] + opts)
]

if self.is_mbed_studio_armc6:
# NOTE: the --ide=mbed argument is only for use with Mbed OS
opts.insert(0, "--ide=mbed")

return opts

def assemble(self, source, object, includes):
Expand All @@ -594,3 +642,23 @@ def compile(self, cc, source, object, includes):
cmd.extend(self.get_compile_options(self.get_symbols(), includes))
cmd.extend(["-o", object, source])
return [cmd]

def get_link_command(self, output, objects, libraries, lib_dirs, scatter_file):
cmd = ARM.get_link_command(
self, output, objects, libraries, lib_dirs, scatter_file
)

if self.is_mbed_studio_armc6:
# NOTE: the --ide=mbed argument is only for use with Mbed OS
cmd.insert(1, "--ide=mbed")

return cmd

def get_binary_commands(self, bin_arg, bin, elf):
cmd = ARM.get_binary_commands(self, bin_arg, bin, elf)

if self.is_mbed_studio_armc6:
# NOTE: the --ide=mbed argument is only for use with Mbed OS
cmd.insert(1, "--ide=mbed")

return cmd