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

[sarahmarshy]

Fixes #2360.

New error:
[ERROR] Could not find executable for IAR.
Currently set search path: /default/path/that/doesnt/exist
(System exit before any build system calls)

@theotherjimmy @0xc0170 @bridadan

[theotherjimmy]

Can we exit with non-zero? It's an error after all.

[theotherjimmy]

After exiting non-zero, LGTM.

[jupe]

do we have any documentation about exit codes like this? would be useful in some cases if all possible exit codes are well known&documented..

[bridadan]

@jupe I don't think anyone has looked at standardizing the return codes across the tools yet, that needs to be revisited in another PR

[theotherjimmy]

Actually, we return 2 on all argument errors after #2393 gets merged.

[bridadan]

Well that's a step in the right direction! I guess my point was we haven't specified a full list of return codes for the tools yet. I think it make sense to look at that collectively across all scripts and make that change in one PR.

[theotherjimmy]

Agreed, and we should add it to the documentation somewhere.

[theotherjimmy]

Should we open an issue, so we don't forget?

[jupe]

Thats good starting point 👍 . Somebody should look about existing return codes, standardize all of them and prepare proper documentation..

[bridadan]

Copying this over from #2416 (@sarahmarshy can this PR just be updated from now on instead of moving it?)

@sarahmarshy It does exit gracefully with an error, but I think we typically try to stay away from calling exit within API functions (like toolchains or build_api). Usually I think we try to return error codes or throw exceptions in API functions and then let the top level CLI script that's calling them handle the exception and exit. Thoughts on this @theotherjimmy ?

@mazimkhan You're right I think this might cause issues with GCC_ARM because the toolchains can find it in the system PATH. I think this is only for GCC_ARM though unfortunately :/ @sarahmarshy sounds like another corner case.

[theotherjimmy]

@sarahmarshy It does exit gracefully with an error, but I think we typically try to stay away from calling exit within API functions (like toolchains or build_api). Usually I think we try to return error codes or throw exceptions in API functions and then let the top level CLI script that's calling them handle the exception and exit. Thoughts on this @theotherjimmy ?

I agree, the toolchains should just throw an exception. This helps out with online build system compatibly.

@mazimkhan You're right I think this might cause issues with GCC_ARM because the toolchains can find it in the system PATH. I think this is only for GCC_ARM though unfortunately :/ @sarahmarshy sounds like another corner case.

I can set the ARM_PATH to '' and force it to find the armcc executable in my path (I might already do that...). This removes that use case.

[bridadan]

I can set the ARM_PATH to '' and force it to find the armcc executable in my path (I might already do that...). This removes that use case.

Not sure exactly what you mean @theotherjimmy ?

[theotherjimmy]

Setting the path to '' would force the subprocess.Popen to search the $PATH for the executable. It's something I'll work around when this PR gets merged. It's NBD.

[bridadan]

Does it do this for all toolchains now? Including IAR? It was my impression that this only worked for GCC_ARM

[sarahmarshy]

@bridadan yes, we can just update this. I think it is at the point where I can make commits instead of new branches to implement the changes listed. Let me look into checking $PATH if the set path is empty.

[theotherjimmy]

Instead of branches and duplicate PR's, you could force push to overwrite the old branch.

[sarahmarshy]

But that would cause a lot of the conversation to look out of context for posterity,

[theotherjimmy]

I think it would show all of the related conversation in the same thread, which might be preferred. This is off topic, if you want to continue this discussion we should do it 'offline'.

[sarahmarshy]

Back to on topic: looks like https://docs.python.org/dev/library/shutil.html#shutil.which would be a good solution for executables in the path that aren't assigned an environment variable. It is only supported in python 3.3, but I will try to find a 2.7 alternative.

[bridadan]

@sarahmarshy I did an implementation of this here: https://github.com/ARMmbed/mbed-os/pull/2057/files#diff-1832807e55f923dee42eee74919441faR69

The PR itself wasn't ready to merge but there might be some code there you can salvage!

[sarahmarshy]

@bridadan going to try this https://docs.python.org/release/2.4/dist/module-distutils.spawn.html first!

[bridadan]

I remember looking at that function and kind of straying away from it due to some of the comments in this Stack Overflow post: http://stackoverflow.com/questions/377017/test-if-executable-exists-in-python

But I never actually tried it, so it may be just fine!

[sarahmarshy]

It is working alright in gcc, it is also working for arm, but there are issues with arm because we manually append the bin path like:
ARM_BIN = join(TOOLCHAIN_PATHS['ARM'], "bin")
which is used:

main_cc = join(ARM_BIN, "armcc")

so, if you leave the path blank the command becomes: bin\armcc

I could modify this to have different behavior when the path is blank.

@theotherjimmy

I can set the ARM_PATH to '' and force it to find the armcc executable in my path (I might already do that...)

Are you sure you do that? Can you check? I am not able to do this successfully.

[theotherjimmy]

I can check tomorrow. Right now I don't have armc5 installed. But, I don't consider it blocking.

[bridadan]

Did we come to a decision about calling exit within toolchains? I'd still vote no on this and throw an exception instead

[bridadan]

If you move this message into the exception you raise below, it will be passed to anything that catches it, which can then print it or ignore.

raise Exception("[ERROR] Could not find executable...")

[screamerbg]

+1 on the idea (haven't reviewed the whole implementation yet)

[bridadan]

@sg- I'm not sure why ARM and IAR have to know the paths to the bin and the include stuff separately? Aren't they aware of these paths themselves? Or is this needed for ulib support for ARM?

[sarahmarshy]

@sg- This implementation uses a decorator for the compile, build, and link functions. It will check whatever would be used as the path to the command. Assuming that config makes its way to TOOLCHAIN_PATHS in settings.py (the variable used in the toolchains to get those paths), then it should be checked.

I can try to look into why we need those includes separately.

[mazimkhan]

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.

[sarahmarshy]

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.

[mazimkhan]

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', '')

[bogdanm]

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.

[mazimkhan]

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

[adbridge]

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

[sarahmarshy]

@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++.

[bogdanm]

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.

[theotherjimmy]

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.

[bogdanm]

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

[bogdanm]

PR looks good overall (with the above comments). I'd also add another change that sets all the paths in tools/settings.py to the empty string (which is the only sensible default I can think of).

[sarahmarshy]

@bogdanm I think that is a good solution. Then, if mbed_settings variables are not set, the value on the PATH will be used.

[adbridge]

I agree with Bogdan about empty strings. It forces the user to think about whether they have configured paths or not. With the non-empty defaults you can get some weird error messages that are not always quick to work out

[sarahmarshy]

@adbridge Yes, the [OS ERROR] was the original motivation for this PR.

[sg-]

Tested on Mac.

>>> from distutils.spawn import find_executable
>>> print find_executable('armcc')
None
>>> print find_executable('arm-none-eabi-gcc')
/Applications/yotta.app/Contents/Resources/prerequisites/gcc-arm-none-eabi-4_9-2015q3/bin/arm-none-eabi-gcc
>>> quit()

I dont have armcc installed so that is why its not found

[sarahmarshy]

@sg- great! A note on the empty string thing, @bogdanm, I think that should come as a separate PR.