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

Add quotes to preprocessor path on export #5199

Merged
merged 2 commits into from Oct 5, 2017

Conversation

Projects
None yet
5 participants
@mgiaco
Contributor

mgiaco commented Sep 26, 2017

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 26, 2017

@theotherjimmy

Please reformat.

@@ -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:])

This comment has been minimized.

@theotherjimmy

theotherjimmy Sep 26, 2017

Contributor

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:]))

This comment has been minimized.

@theotherjimmy

theotherjimmy Sep 26, 2017

Contributor

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:])

This comment has been minimized.

@mgiaco

mgiaco Sep 27, 2017

Contributor

Okay i have removed the first join that was not okay. But sorry your changes are not correct.

  1. basename is the filename only so that did not solve the issue at all.
  2. 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

This comment has been minimized.

@theotherjimmy

theotherjimmy Sep 27, 2017

Contributor

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.

This comment has been minimized.

@theotherjimmy

theotherjimmy Sep 27, 2017

Contributor

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 theotherjimmy changed the title from add quotation marks for compiler path to Add quotes to preprocessor path on export Sep 26, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 26, 2017

@mgiaco Thanks so much for making this patch.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 26, 2017

FYI, this fix applies to any OS, so long as they have a space in the path.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 27, 2017

@theotherjimmy please review again

@theotherjimmy

My only qualm now is formatting related.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 27, 2017

@mgiaco Do you have arm-none-eabi-cpp accessible from your PATH environment variable? If you don't that might have been the cause of the basename patch not working.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 27, 2017

Moved from a closed diff thread:

basename is the filename only so that did not solve the issue at all.

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.

you removed the '"{}"'.form... this is the important part of the fix. ...

Not if you invoke the command without a preceding path. There should be no spaces in the path if it's only arm-none-eabi-cpp.

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.

@@ -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:])

This comment has been minimized.

@theotherjimmy

theotherjimmy Sep 27, 2017

Contributor

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.

@mgiaco

This comment has been minimized.

Contributor

mgiaco commented Sep 27, 2017

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.

@mgiaco

This comment has been minimized.

Contributor

mgiaco commented Sep 27, 2017

I would say and that is that behavior... Sorry

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 27, 2017

@mgiaco You are correct in that:

So the error comes from the path only.

If you look at what I said about the export from the website:

When we run this exporter on the website, you have no path information to use

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

I am wondering why I am the only one who has this issue

Me too!

Maybe GNU ARM Eclipse normally sets the PATH before invoking make, and something is going wrong with you setup. I'd have to do more research.

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] would like to control the exact version [of GCC].

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) .

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 27, 2017

/morph export-build

@mgiaco

This comment has been minimized.

Contributor

mgiaco commented Sep 28, 2017

@theotherjimmy okay super.
Concerning the compiler ... i always switch the compiler but I would like to be sure which compiler is used - that is all.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Oct 2, 2017

/morph export-build

@mbed-bot

This comment has been minimized.

mbed-bot commented Oct 3, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph export-build

Output

mbed Build Number: 164

All exports and builds passed!

@adbridge

This comment has been minimized.

Contributor

adbridge commented Oct 3, 2017

@theotherjimmy we should still run a morph test on this ?

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Oct 3, 2017

Nope

@theotherjimmy theotherjimmy merged commit 9b62dce into ARMmbed:master Oct 5, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-export-build Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment