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

Double escape defines in gnuarmeclipse export #4636

Merged
merged 1 commit into from Jul 7, 2017

Conversation

Projects
None yet
6 participants
@theotherjimmy
Contributor

theotherjimmy commented Jun 26, 2017

Fixes a bug where quoting gets stripped by the shell used in the makefile
and another bug where the lack of escaping would cause parser errors in
eclipse.

Resolves #4622

@JojoS62

This comment has been minimized.

Contributor

JojoS62 commented Jun 26, 2017

@JojoS62

This comment has been minimized.

Contributor

JojoS62 commented Jun 26, 2017

this fix works, but it leaves backslashes that can be removed?

grafik

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 26, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 26, 2017

Oh joy. A travis bug on master.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 26, 2017

@JojoS62 Let me explain why those extra escapes are required. First, we are using this define in place of an include file. We cannot quote the argument to #include as it will not expand the macro in that instance. Instead, we are left with adding the quotes to the macro. Next, we have to pass this quoted value with the quotes to the compiler on the command line. This reqires a

str.replace("\"", "\\\"")

Which will prevent the shell from expanding the quotes itself. Now we need to pass that value, backslashes, quotes and all, to eclipse. That is done by escaping the value for HTML, a function built into Jinja2.

TL;DR: backslashes are needed to build correctly.

@theotherjimmy theotherjimmy changed the title from Double escape defines to Double escape defines in gnuarmeclipse export Jun 26, 2017

@JojoS62

This comment has been minimized.

Contributor

JojoS62 commented Jun 26, 2017

thanks, and you're right, without backslashes the compiler complains.

@JojoS62

This comment has been minimized.

Contributor

JojoS62 commented Jun 27, 2017

just one more thing for my curiosity:
I've checked it also in SW4STM32 and there the leading backslashes are not necessary. The reason should be the different ways how the different build systems work.
As a side effect, the eclipse C++ indexer gets confused by the backslahes in the macro value. It compiles, but wouldn't it be a cleaner solution when the gnuarmeclipse plugin modifies the symbol value as needed? @ilg : what do you think?

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 27, 2017

@JojoS62 I agree that It would be cleaner to not escape the macro for the makefile under eclipse.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 29, 2017

/morph export-build

@mbed-bot

This comment has been minimized.

mbed-bot commented Jun 29, 2017

Result: FAILURE

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

/morph export-build

Output

mbed Build Number: 75

Exporter Build failed!

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 29, 2017

sigh these errors are probably on master...

Double escape defines
Fixes a bug where quoting gets stripped by the shell used in the makefile
and another bug where the lack of escaping would cause parser errors in
eclipse.

@theotherjimmy theotherjimmy force-pushed the theotherjimmy:quote-cflags branch to a5a5fa3 Jun 29, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 29, 2017

/morph export-build

@mbed-bot

This comment has been minimized.

mbed-bot commented Jun 30, 2017

Result: FAILURE

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

/morph export-build

Output

mbed Build Number: 76

Exporter Build failed!

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Jul 3, 2017

/morph export-build

@mbed-bot

This comment has been minimized.

mbed-bot commented Jul 3, 2017

Result: FAILURE

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

/morph export-build

Output

mbed Build Number: 78

Exporter Build failed!

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Jul 5, 2017

/morph export-build

@mbed-bot

This comment has been minimized.

mbed-bot commented Jul 5, 2017

Result: SUCCESS

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

/morph export-build

Output

mbed Build Number: 82

All exports and builds passed!

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jul 5, 2017

Wohooo!

LGTM!!

@adbridge adbridge merged commit 292b408 into ARMmbed:master Jul 7, 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

@sg- sg- removed the ready for merge label Jul 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment