Skip to content
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

Jump to bottom

Update gnuarmeclipse to preprocess linker scripts #4069

Merged
merged 1 commit into from Apr 10, 2017
Merged

Update gnuarmeclipse to preprocess linker scripts #4069

merged 1 commit into from Apr 10, 2017

Conversation

theotherjimmy
Copy link
Contributor

@theotherjimmy theotherjimmy commented Mar 29, 2017 •
edited by bridadan

PR #3706 changed the behavior of .ld files in mbed-os. After that PR, all .ld files are passed through the C pre-processor before handing them off to the linker. This PR changes the GNU ARM Eclipse exporter to do that pre-processing as well.

Todos

  • /morph export-build (with gnuarmeclipse enabled please)
  • Review by @bridadan
  • Review by @ilg-ul

Notes

At the moment, the implementation pre-processes the linker script for all profiles, when it only needs one. This is because I was not able to find a way to get the linker other flags as a make variable. @ilg-ul is it possible to get the linker other flags in a Makefile variable for the makefile.targets extension?

@ilg-ul
Copy link
Contributor

ilg-ul commented Mar 29, 2017

I'm afraid I can no longer follow the details of this issue... :-(

@theotherjimmy
Copy link
Contributor Author

Sorry about that. Is there any clarification I can provide?

@theotherjimmy theotherjimmy mentioned this pull request Mar 29, 2017
Closed
ilg-ul
ilg-ul reviewed Mar 29, 2017
View reviewed changes
Copy link
Contributor

@ilg-ul ilg-ul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as you like.
FYI, in Eclipse the common flags are applied to all tools, including the linker.

tools/export/gnuarmeclipse/__init__.py Outdated
@@ -1014,8 +1014,7 @@ def process_options(self, opts, flags_in):
opts['cpp']['other'] += ' ' + \
' '.join(flags['common_flags']) + ' ' + \
' '.join(flags['cxx_flags'])
opts['ld']['other'] += ' ' + \
' '.join(flags['common_flags']) + ' ' + ' '.join(flags['ld_flags'])
opts['ld']['other'] += ' ' + ' '.join(flags['ld_flags'])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not how the mbed-os tools do it. I take it that this is also non-standard for eclipse.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll just take that commit off the top then. There does not seem to be a problem with adding the common flags. I'm just worried that any differences in the future will create failing exports if we are not careful.

@theotherjimmy
Copy link
Contributor Author

theotherjimmy commented Mar 29, 2017 •
edited

Thanks for the clarification. I removed the commit that you commented on. There should not be any need to change the default eclipse behavior on this one.

@sg- sg- added the needs: CI label Mar 29, 2017
@0xc0170
Copy link
Contributor

0xc0170 commented Mar 30, 2017

retest uvisor

bridadan
bridadan approved these changes Mar 30, 2017
View reviewed changes
Copy link
Contributor

@bridadan bridadan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for my late review, changes look good 👍

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 3, 2017

retest uvisor

@sg-
Copy link
Contributor

sg- commented Apr 6, 2017

/ m o r p h e x p o r t - b u i l d (with gnuarmeclipse enabled please)

@bridadan @studavekar Are these enabled and ready to run?

@bridadan
Copy link
Contributor

bridadan commented Apr 6, 2017

They are now! Yee haw 🏇

/morph export-build

@bridadan bridadan mentioned this pull request Apr 6, 2017
Merged
@mbed-bot
Copy link

mbed-bot commented Apr 7, 2017

Result: SUCCESS

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

/morph export-build

Output

mbed Build Number: 152

All exports and builds passed!

@ilg-ul
Copy link
Contributor

ilg-ul commented Apr 7, 2017 •
edited

Result: SUCCESS

great! this means that until someone else will push another incompatible patch, we're good. :-(

as an 'early warning' system, since the entire export-build test is too expensive to run always, I suggest you add to the tests executed always, a single gnuarmeclipse export + build for the blinky project.

@bridadan
Copy link
Contributor

bridadan commented Apr 7, 2017

@ilg-ul we're definitely looking at this. Believe us, we don't like this stuff breaking any more than you do! Thanks again for all of your help.

Also, great work @theotherjimmy for patching this!

@bridadan
Copy link
Contributor

bridadan commented Apr 7, 2017

@0xc0170 Can we get this one in fairly quickly? It's blocking this PR: #4115

And the sooner we get this in the sooner we can start enforcing it.

@sg- sg- merged commit 0b11177 into ARMmbed:master Apr 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ilg-ul ilg-ul ilg-ul left review comments

@bridadan bridadan bridadan approved these changes

Assignees
No one assigned
Labels
release-version: 5.4.4
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@theotherjimmy @ilg-ul @0xc0170 @sg- @bridadan @mbed-bot