-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Readable error when toolchain paths not set. Allow IAR and ARM toolchains to be set in user's PATH. #2418
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 ' \ | ||
'set search path: %s'% (self.name, self.toolchain_path) | ||
raise Exception(error_string) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mazimkhan, here is the exception There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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,,,, There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -900,6 +918,7 @@ def compile_output(self, output=[]): | |
else: | ||
raise ToolException(_stderr) | ||
|
||
@check_toolchain_path | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -911,6 +930,7 @@ def build_library(self, objects, dir, name): | |
|
||
return needed_update | ||
|
||
@check_toolchain_path | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to test how this decorator works with the existsing |
||
def link_program(self, r, tmp_path, name): | ||
needed_update = False | ||
ext = 'bin' | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this add a new dependency of our tools on There was a problem hiding this comment. Choose a reason for hiding this commentThe 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' | ||
|
@@ -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') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 :( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or without a .exe suffix ... There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have it - @bogdanm ?? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have OSX, but it runs fine on Windows:
|
||
if exe: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My question was actually local. If There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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") | ||
|
||
|
@@ -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(): | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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.