Skip to content

Readable error when toolchain paths not set. Allow IAR and ARM toolchains to be set in user's PATH. #2418

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.

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 3 commits into from
Aug 26, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
20 changes: 20 additions & 0 deletions tools/toolchains/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,23 @@ def __str__(self):
}


def check_toolchain_path(function):
"""Check if the path to toolchain is valid. Exit if not.
Use this function as a decorator. Causes a system exit if the path does
not exist. Execute the function as normal if the path does exist.

Positional arguments:
function -- the function to decorate
"""
def perform_check(self, *args, **kwargs):
if not exists(self.toolchain_path):
error_string = 'Could not find executable for %s.\n Currently ' \
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm reading this right, the error string is a bit misleading. You're not checking for an executable, but rather for the top directory of the whole toolchain, is that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It depends on the toolchain, unfortunately. GCC uses the executable. ARM needs the root because it uses both /bin/ and /include/ directories.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then maybe this test should be toolchain-specific rather than generic.

Copy link
Contributor Author

@sarahmarshy sarahmarshy Aug 19, 2016

Choose a reason for hiding this comment

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

I could also force self.toolchain_path to be the path to the executable. I created that class variable for this purpose.

'set search path: %s'% (self.name, self.toolchain_path)
raise Exception(error_string)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mazimkhan, here is the exception

Choose a reason for hiding this comment

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

This is not what I was asking. Please see the comment below.

return function(self, *args, **kwargs)
return perform_check


class mbedToolchain:
# Verbose logging
VERBOSE = True
Expand Down Expand Up @@ -702,6 +719,7 @@ def get_arch_file(self, objects):

# THIS METHOD IS BEING CALLED BY THE MBED ONLINE BUILD SYSTEM
# ANY CHANGE OF PARAMETERS OR RETURN VALUES WILL BREAK COMPATIBILITY
@check_toolchain_path
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps this could be named more general way because it might be that you want to check inc_dirs and other thing here as well before start compiling in future,,,,

Copy link
Contributor

Choose a reason for hiding this comment

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

Then it would have to take in a parameter to check the existence of. At the Class construction time (when this gets called) we don't know what the directory we need to check should be, so we can't pass it.

def compile_sources(self, resources, build_path, inc_dirs=None):
# Web IDE progress bar for project build
files_to_compile = resources.s_sources + resources.c_sources + resources.cpp_sources
Expand Down Expand Up @@ -900,6 +918,7 @@ def compile_output(self, output=[]):
else:
raise ToolException(_stderr)

@check_toolchain_path
Copy link
Contributor

Choose a reason for hiding this comment

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

That decorator is sliiiiiick :)

def build_library(self, objects, dir, name):
needed_update = False
lib = self.STD_LIB_NAME % name
Expand All @@ -911,6 +930,7 @@ def build_library(self, objects, dir, name):

return needed_update

@check_toolchain_path
Copy link
Contributor Author

@sarahmarshy sarahmarshy Aug 19, 2016

Choose a reason for hiding this comment

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

@mazimkhan here is the decorator in use. This will cause an exception to be thrown before calling link_program if the path to the toolchain executable doesn't exist.

Copy link
Contributor

@bogdanm bogdanm Aug 19, 2016

Choose a reason for hiding this comment

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

We need to test how this decorator works with the existsing hook_tool decorator (for example here). If they don't work together properly, this might break compilation on targets that have a post-link step (for example LPC4088).

def link_program(self, r, tmp_path, name):
needed_update = False
ext = 'bin'
Expand Down
11 changes: 9 additions & 2 deletions tools/toolchains/arm.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,12 @@
limitations under the License.
"""
import re
from os.path import join, dirname, splitext, basename, exists
from os.path import join, dirname, splitext, basename
from distutils.spawn import find_executable
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this add a new dependency of our tools on distutils?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is part of the standard lib.


from tools.toolchains import mbedToolchain, TOOLCHAIN_PATHS
from tools.hooks import hook_tool
from tools.utils import mkdir
import copy

class ARM(mbedToolchain):
LINKER_EXT = '.sct'
Expand Down Expand Up @@ -56,6 +56,11 @@ def __init__(self, target, options=None, notify=None, macros=None, silent=False,
else:
cpu = target.core

if not TOOLCHAIN_PATHS['ARM']:
exe = find_executable('armcc')
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this do the right thing on Linux/OSX and Windows (where the executable is called armcc.exe) ?

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 tested this function on Linux, but I am not sure about OSX :(

Copy link
Contributor

Choose a reason for hiding this comment

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

Or without a .exe suffix ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have OSX? Would you mind trying this in a Python terminal if so?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have it - @bogdanm ??

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have OSX, but it runs fine on Windows:

>>> from distutils.spawn import find_executable
>>> print find_executable('armcc')
C:\software\armcc5.05\bin\armcc.exe
>>> print find_executable('armcc.exe')
C:\software\armcc5.05\bin\armcc.exe

if exe:
Copy link

@mazimkhan mazimkhan Aug 19, 2016

Choose a reason for hiding this comment

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

What if this condition fails. line 64 assumes ARM path in dict TOOLCHAIN_PATHS. Perhaps raise an exception. Though not sure if this class is instantiated on selection of -t ARM.

Copy link
Contributor Author

@sarahmarshy sarahmarshy Aug 19, 2016

Choose a reason for hiding this comment

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

I did not raise an exception here because I did not want the constructor to fail. The exception will be raised before compile_sources, link_program, or build_library. Nothing in this scope actually executes the command.

Copy link

@mazimkhan mazimkhan Aug 19, 2016

Choose a reason for hiding this comment

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

My question was actually local. If if exe fails then at line 64 TOOLCHAIN_PATHS['ARM'] will give KeyError exception.
If you don't care about actual path being constructed then you can do TOOLCHAIN_PATHS.get('ARM', '')

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why the constructor shouldn't fail? It looks like bailing out as soon as the error is found would be a good thing. Sorry if I'm missing something.

Copy link

@mazimkhan mazimkhan Aug 19, 2016

Choose a reason for hiding this comment

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

please ignore my comment. I can see that TOOLCHAIN_PATHS['ARM'] is already initialized tools.toolchains.__init__.py

Copy link
Contributor

Choose a reason for hiding this comment

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

Has this been checked for all the o/s's we may come across?

Copy link
Contributor Author

@sarahmarshy sarahmarshy Aug 19, 2016

Choose a reason for hiding this comment

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

@bogdanm @theotherjimmy has strong opinions here. I agree that we are delaying failure. But I think it is generally assumed that when you are constructing an object, it will succeed. Though I think __init__ is not a constructor in the same way as C++.

Copy link
Contributor

Choose a reason for hiding this comment

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

init is definitely not a constructor in the same sense as a C++ constructor. Exceptions inside constructors in C++ are bad because they might leave the object in an inconsistent state. That's hardly a concern here, when the outcome is that your program terminates with an error message. But in any case, this isn't a huge deal.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, my beef with the constructor failing has nothing to do with any of that. It's pretty simple: A constructed toolchain is required for an exporter. The online tools have only armc5. We need to be able to export to non-armc5 IDE's. So we must not fail to construct a toolchain when it's executable is not present.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I misunderstood. That makes a lot of sense.

TOOLCHAIN_PATHS['ARM'] = dirname(dirname(exe))

ARM_BIN = join(TOOLCHAIN_PATHS['ARM'], "bin")
ARM_INC = join(TOOLCHAIN_PATHS['ARM'], "include")

Expand All @@ -81,6 +86,8 @@ def __init__(self, target, options=None, notify=None, macros=None, silent=False,
self.ar = join(ARM_BIN, "armar")
self.elf2bin = join(ARM_BIN, "fromelf")

self.toolchain_path = TOOLCHAIN_PATHS['ARM']

def parse_dependencies(self, dep_path):
dependencies = []
for line in open(dep_path).readlines():
Expand Down
6 changes: 6 additions & 0 deletions tools/toolchains/gcc.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
"""
import re
from os.path import join, basename, splitext, dirname, exists
from distutils.spawn import find_executable

from tools.toolchains import mbedToolchain, TOOLCHAIN_PATHS
from tools.hooks import hook_tool
Expand Down Expand Up @@ -110,6 +111,11 @@ def __init__(self, target, options=None, notify=None, macros=None, silent=False,
self.ar = join(tool_path, "arm-none-eabi-ar")
self.elf2bin = join(tool_path, "arm-none-eabi-objcopy")

if tool_path:
self.toolchain_path = main_cc
else:
self.toolchain_path = find_executable("arm-none-eabi-gcc") or ''

def parse_dependencies(self, dep_path):
dependencies = []
buff = open(dep_path).readlines()
Expand Down
9 changes: 9 additions & 0 deletions tools/toolchains/iar.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import re
from os import remove
from os.path import join, exists, dirname, splitext, exists
from distutils.spawn import find_executable

from tools.toolchains import mbedToolchain, TOOLCHAIN_PATHS
from tools.hooks import hook_tool
Expand Down Expand Up @@ -50,6 +51,12 @@ def __init__(self, target, options=None, notify=None, macros=None, silent=False,
cpuchoice = "Cortex-M7"
else:
cpuchoice = target.core

if not TOOLCHAIN_PATHS['IAR']:
exe = find_executable('iccarm')
if exe:
TOOLCHAIN_PATHS['IAR'] = dirname(dirname(exe))

# flags_cmd are used only by our scripts, the project files have them already defined,
# using this flags results in the errors (duplication)
# asm accepts --cpu Core or --fpu FPU, not like c/c++ --cpu=Core
Expand Down Expand Up @@ -101,6 +108,8 @@ def __init__(self, target, options=None, notify=None, macros=None, silent=False,
self.ar = join(IAR_BIN, "iarchive")
self.elf2bin = join(IAR_BIN, "ielftool")

self.toolchain_path = TOOLCHAIN_PATHS['IAR']

def parse_dependencies(self, dep_path):
return [(self.CHROOT if self.CHROOT else '')+path.strip() for path in open(dep_path).readlines()
if (path and not path.isspace())]
Expand Down