-
Notifications
You must be signed in to change notification settings - Fork 3k
tools: fix toolchain extend inc paths #4749
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
Without this patch, we would get
@bridadan This looks similar to what we seen with CI in the last 2 days, this might fix it ! However I wonder why this problem has not been seen earlier. |
Verified |
tools/toolchains/__init__.py
Outdated
@@ -863,7 +863,10 @@ def compile_sources(self, resources, inc_dirs=None): | |||
|
|||
inc_paths = resources.inc_dirs | |||
if inc_dirs is not None: | |||
inc_paths.extend(inc_dirs) | |||
if type(inc_dirs) == ListType: |
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.
A more current pythonic way to do this is:
if isinstance(inc_dirs, list):
Please use that form.
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.
will update
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.
Thanks!
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.
Meant to request changes. :(
I think that the confusion came from the fact that the argument is plural, implying an unordered set of include directories. |
inc paths might be a list or might not be (just single string). If they don't, we are ending up with non valid include paths (one letter include paths). This as result would not compile.
Fixed, rebased |
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.
Awesome. Thanks!
/morph test |
/morph export-build |
Result: SUCCESSYour command has finished executing! Here's what you wrote!
Outputmbed Build Number: 86 All exports and builds passed! |
Result: FAILUREYour command has finished executing! Here's what you wrote!
OutputTest failed! |
/morph test |
Result: SUCCESSYour command has finished executing! Here's what you wrote!
OutputAll builds and test passed! |
inc paths might be a list or might not be (just single string). If they don't, we are ending up with non valid include paths (one letter include paths).
This as result would not compile.
As inc_dirs might be just
D:/SomePath
, extend this would lead to producing [D
,:
,S
..........] thus we were not able to compile with tools/build.py for instance. This is an example I was seeing[DEBUG] INC DIRS: ['C', ':', '\\', 'C', 'o', 'd', 'e', '\\', 'm', 'b', 'e', 'd', '-', 'o', 's', '\\', 'B', 'U', 'I', 'L', 'D', '\\', 'm', 'b', 'e', 'd', '\\', '.', 't', 'e', 'm', 'p', '\\', 'T', 'A', 'R', 'G', 'E', 'T', '_', 'N', 'U', 'C', 'L', 'E', 'O', '_', 'F', '4', '1', '1', 'R', 'E', '\\', 'T', 'O', 'O', 'L', 'C', 'H', 'A', 'I', 'N', '_', 'I', 'A', 'R']
@jeromecoutant This should fix the issue you were seeing today with IAR
@theotherjimmy please review, we handle inc dirs in scan resources similarly, might be better to always consider this as a list, or hack like this. You can take this patch and just push to my branch.