Add quotes to preprocessor path on export #5199
Conversation
| @@ -244,7 +244,7 @@ def create_jinja_ctx(self): | |||
| opts['ld']['system_libraries'] = self.system_libraries | |||
| opts['ld']['script'] = join(id.capitalize(), | |||
| "linker-script-%s.ld" % id) | |||
| opts['cpp_cmd'] = " ".join(toolchain.preproc) | |||
| opts['cpp_cmd'] = "".join('"{}"'.format(toolchain.preproc[0])) + " " + " ".join(toolchain.preproc[1:]) | |||
There was a problem hiding this comment.
Please remove the join call to something that is not a list. Please limit lines to 80 characters.
This should be:
opts['cpp_cmd'] = '"{}" {}'.format(toolchain.preproc[0],
" ".join(toolchain.preproc[1:]))There was a problem hiding this comment.
Upon further thinking, I think we should just use the filename. So:
opts['cpp_cmd'] = '{} {}'.format(basename(toolchain.preproc[0]),
" ".join(toolchain.preproc[1:]))or
opts['cpp_cmd'] = " ".join(basename(toolchain.preproc[0]) +
toolchain.preproc[1:])There was a problem hiding this comment.
Okay i have removed the first join that was not okay. But sorry your changes are not correct.
- basename is the filename only so that did not solve the issue at all.
- you removed the '"{}"'.form... this is the important part of the fix. ...
So I have updated my PR I do not see any easier Solution. Also I do not understand why 80 chars only because have you looked on line 429 as an example the line has 111 chars :-)
cheers
There was a problem hiding this comment.
basename is the filename only so that did not solve the issue at all.
That's the point. When we run this exporter on the website, you have no path information to use, so we have to make an assumption. I'm suggesting that we unify these behaviors.
you removed the '"{}"'.form... this is the important part of the fix. ...
Not if you invoke the command with not preceding path. There should be no spaces in the path if it's only arm-none-eabi-cpp.
There was a problem hiding this comment.
Also I do not understand why 80 chars only because have you looked on line 429 as an example the line has 111 chars :-)
I'm trying not to add formatting work for later.
theotherjimmy
changed the title
|
@mgiaco Thanks so much for making this patch. |
|
FYI, this fix applies to any OS, so long as they have a space in the path. |
|
@theotherjimmy please review again |
|
@mgiaco Do you have |
|
Moved from a closed diff thread:
That's the point, and it did solve the issue mentioned, while imposing another restriction. When we run this exporter on the website, you have no path information to use, so we have to make an assumption. I'm suggesting that we unify these behaviors.
Not if you invoke the command without a preceding path. There should be no spaces in the path if it's only
I'm trying not to add formatting work for later. |
| @@ -244,7 +244,7 @@ def create_jinja_ctx(self): | |||
| opts['ld']['system_libraries'] = self.system_libraries | |||
| opts['ld']['script'] = join(id.capitalize(), | |||
| "linker-script-%s.ld" % id) | |||
| opts['cpp_cmd'] = " ".join(toolchain.preproc) | |||
| opts['cpp_cmd'] = '"{}"'.format(toolchain.preproc[0]) + " " + " ".join(toolchain.preproc[1:]) | |||
There was a problem hiding this comment.
This might be more clear:
opts['cpp_cmd'] = '"{}" {}'.format(toolchain.preproc[0]), " ".join(toolchain.preproc[1:])Don't worry, it's semantically the same thing.
|
Okay.. no I do not have the compiler in the path, and I also think that this isn't a good way to go for embedded development. But that's my opinion. I need more different versions of gcc and would like to control the exact version. So if you do not have any path then you do not need any patch. So the error comes from the path only. But as I said for me this is a bad behavior because I have to set the path for mbed and therefore the exporter should also use that compiler and that's no the behavior but with that error I reported. I am wondering why I am the only one who has this issue. |
|
I would say and that is that behavior... Sorry |
|
@mgiaco You are correct in that:
If you look at what I said about the export from the website:
What should we put here as the path to the preprocesser when exporting from the website? If we have a Windows default path, we risk making the export fail mysteriously for those who have installed in a non-default location, Linux users and Mac users. If we use an Ubuntu path we make the export unusable for users of other distributions, Windows users and Mac users. All that said, I'm suspicious that something is not how GNU ARM Eclipse expects it
Me too! Maybe GNU ARM Eclipse normally sets the I'll look into this in further detail tomorrow. With all of that said, I have approved this PR, and I stand by that approval. It is the solution to the issue you have raised and a clear improvement over not quoting the path. Off topic, not intended as a critique, just curious:
I see this pretty often, and I always wonder what causes people to have this opinion. I don't see embedded development this way, but my experience may be a very biased sample. I have found that relying on compiler minor or patch version is almost always a bug. Now, relying on major version, for example Arm Compiler 5 vs Arm Compiler 6, can be very reasonable (look at the porting guide from Arm Compiler 5 to Arm Compiler 6, it's huge). That being said, GCC has been pretty stable WRT the C standard, and a conservative set of intrinsics (simple inline assembly syntax, few attributes like packed, inline, noinline, used and a few more things) . |
|
/morph export-build |
|
@theotherjimmy okay super. |
|
/morph export-build |
Result: SUCCESSYour command has finished executing! Here's what you wrote!
Outputmbed Build Number: 164 All exports and builds passed! |
|
@theotherjimmy we should still run a morph test on this ? |
|
Nope |
Metadata
Type: Bugfix
Resolves: #5186
Ready
Description
Hi,
So this is a fix for #5186 tested on windows only. So can someone test this on the other systems mbed and the exporter is running.
cheers
mathias