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

Fix include paths for the assembler for makefiles #9946

Merged
merged 1 commit into from Mar 6, 2019

Conversation

Projects
None yet
5 participants
@bridadan
Copy link
Contributor

commented Mar 5, 2019

Description

Should fix #9938.

Seems to work fine with ARMC5, ARMC6, and GCC_ARM.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@0xc0170 - I tried my darndest to get git to ignore the whitespace changes, but I couldn't figure it out. If you feel really strongly about it let me know.

Release Notes

@0xc0170

0xc0170 approved these changes Mar 5, 2019

Copy link
Member

left a comment

Would be nice to have spaces out of the picture, lets get this in CI asap

new_sub_flags = []
for sub_flag in sub_flags:
new_sub_flags.append(_fix_include_asm_flag(sub_flag, ctx))
new_asm_flags.append(','.join(new_sub_flags))

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Mar 5, 2019

Member

one question - how does this fixes the issue - why paths for asm preprocessed files were not correct? I can see magic above based on -I or preinclude - shall we add some details to the commit msg?

Fix include paths for the assembler for makefiles.
The Makefile is run from the build directory. The source files were
properly prefixed with "../", however the paths provided to the
assmebler were not. This ensures the assembler include paths are
prefixed properly.
@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

CI started

@cmonr

cmonr approved these changes Mar 5, 2019

@cmonr

This comment has been minimized.

@bridadan bridadan force-pushed the bridadan:fix_make_armc6 branch 2 times, most recently to c129336 Mar 5, 2019

@bridadan

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

I fixed my dumb syntax error and updated the commit message. I will run further tests locally in the meantime.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

CI restarted

@bridadan

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

Ug, think there might be more issues, still debugging.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

Ug, think there might be more issues, still debugging.

Kk. Will let CI still run in the meanwhile.

Out of curiosity, do we have a way to test against the online compiler, or would we need to locally test somehow?

@bridadan

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

@cmonr pretty sure the issue I was seeing locally was a problem with my path config, not the implementation. Trying now.

Regarding the online compiler, we would need to test this when we make a tools release as this change wouldn't go live until then.

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 6, 2019

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 2
Build artifacts

@cmonr cmonr merged commit b35b37d into ARMmbed:master Mar 6, 2019

28 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARMC5 Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR8 Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARMC5 Success
Details
jenkins-ci/mbed2-build-ARMC6 Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR8 Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9296 cycles (-866 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@cmonr cmonr removed the needs: CI label Mar 6, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

Merging to see if this unblocks the other 5.12 PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.