-
Notifications
You must be signed in to change notification settings - Fork 3k
Readable error when toolchain paths not set. #2416
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
Conversation
Fixes ARMmbed#2360. New error: [Error] Toolchain path does not exist for IAR. Current value: /default/path/that/doesnt/exist (System exit before any build system calls)
I disagree with adding it to the toolchains' constructor for several reasons:
Maybe we should check for the binaries at the moment we know that the Further, could we throw an exception, or maybe do |
@theotherjimmy one consideration: at the time of those functions, resource scan has already happened. It would just take some extra time to fail, but it is marginal. I think the things mentioned should supersede the timing consideration. |
I agree that it would delay the failure until after the toolchains perform the scan, but the current implementation is simply incorrect for the aforementioned reasons. |
|
||
No return value but causes a system exit of execution if the path | ||
does not exist. | ||
""" |
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.
❤️ the docstring!
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 have seen the light! Going to try to add them to everything I modify in the future.
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.
Do we really want to exit inside of this function? It seems like we'd just want to "check_toolchain_path" here, return True or False, and delegate the appropriate action farther up the call stack. For instance, the toolchains could call this, check the return value, and throw an exception. That could be caught by the CLI scripts (build.py
, make.py
, test.py
, etc) and they could exit gracefully.
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 should exit gracefully with the error? Unless you need some exit value. Check the new PR.
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.
@bridadan do you think it is breaking change. I see that the build has failed while trying to get GCC_ARM toolchain path. I see
17:13:53 + mbed-cli test --compile --test-spec test_spec.json -vv -t GCC_ARM -m K64F
17:14:19 [ERROR] Toolchain path does not exist for GCC_ARM.
in http://e108747.cambridge.arm.com:8080/job/mbed-os/job/mbed-os-pr-build-parameterized/612/console
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.
@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.
Let's try this again. Moved to #2418 |
Fixes #2360.
New error:
[Error] Toolchain path does not exist for IAR.
Current value: /default/path/that/doesnt/exist
(System exit before any build system calls)
@0xc0170 @bridadan