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

Implement Linker command/response files in make export #9596

Merged
merged 2 commits into from Feb 7, 2019

Conversation

Projects
None yet
7 participants
@vmedcy
Copy link
Contributor

vmedcy commented Feb 2, 2019

Description

This is a rebased version of 387747f from #7583 that can be applied to the latest master. Two other commits from that PR are no longer needed.

The original PR was closed since "it's really hard to find make > 3.81 for windows."
Compatible version of make utility for windows is documented in ARMmbed/mbed-os-5-docs#935
Cygwin make and MSYS2 make (both at version 4.2.1) are confirmed to work fine with makefiles produced by mbed export -i eclipse_gcc_arm.

The fix allows to build the mbed projects exported under Windows to environments that use the generated makefile: make_gcc_arm, make_iar, make_armc5, make_armc6, eclipse_gcc_arm, eclipse_iar, eclipse_armc5, vscode_gcc_arm, vscode_iar, vscode_armc5.

The error happens due to the list of the object files passed to the linker process exceeding Windows limit for the command line length.
Error code:
make (e=87): The parameter is incorrect.

Fixes the following issues:

  • Exporting to Makefile fails to build on Windows #6335
  • Exporting to Makefile fails to build on Windows due to command line limit being exceeded #9140
  • [OoB_5.11.0-RC2]: Debugging with exporting eclipse_gcc_arm failed #9015
  • [OoB_5.11.0-RC2]: Debugging with exporting vscode_gcc_arm failed #9000
  • MBED-OS 5.10 OOB - export to make_armc5, error in linking phase #8101
  • GCC Windows export does not build as of #6577 #7112

Pull request type

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

Reviewers

@theotherjimmy (who is the author of the original commit on the PR)

@ciarmcom ciarmcom requested review from theotherjimmy and ARMmbed/mbed-os-maintainers Feb 2, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

ciarmcom commented Feb 2, 2019

@vmedcy, thank you for your changes.
@theotherjimmy @ARMmbed/mbed-os-tools @ARMmbed/mbed-os-maintainers please review.

makefile export: create .link_options.txt with echo
$(file > $@.in, $(filter %.o, $^)) is not supported in GNU Make 3.81.
Create the linker response file with pipe redirect from echo command.
This is tested with Cygwin make and make 3.8.1 shipped with macOS.

Also, change the linker responce file name to .link_options.txt.
This is an internal file, not a build artifact.
@vmedcy

This comment has been minimized.

Copy link
Contributor Author

vmedcy commented Feb 4, 2019

The original commit uses $(file > $@.in, $(filter %.o, $^)) syntax - this is not compatible with GNU Make 3.81 commonly found on macOS.

I reworked the rule in Makefile template to use the echo redirect instead:
@echo "$(filter %.o, $^)" > .link_options.txt

I believe .link_options.txt is more appropriate name for the temporary response file: $(PROJECT).in looks like a public artifact.

I tested this with GNU Make 4.21 on Cygwin and MSYS2, as well as with make 3.81 on macOS.
On Windows, I also tested eclipse_iar and eclipse_armc5 to check the syntax of response file option.

@vmedcy

This comment has been minimized.

Copy link
Contributor Author

vmedcy commented Feb 4, 2019

ARMmbed/mbed-os-5-docs#935 documents the findings on make utilities compatible with mbed generated makefiles.

Please prioritize this for the next release: Cypress heavily relies on the ability to build and debug Mbed projects in external environments supported by Makefile, throwing make (e=87): The parameter is incorrect at Windows users is not really an option. There are still lots of customers using legacy operating systems.

@theotherjimmy

This comment has been minimized.

Copy link
Contributor

theotherjimmy commented Feb 4, 2019

@vmedcy My understanding is that echo is interpenetrated by the terminal, so it will not get around the windows error 87. Do you have a test case that fails on master and passes with this branch?

@vmedcy

This comment has been minimized.

Copy link
Contributor Author

vmedcy commented Feb 5, 2019

@theotherjimmy:

echo is interpenetrated by the terminal, so it will not get around the windows error 87

echo "$(filter %.o, $^)" is executed by cygwin make, in cygwin shell, my guess is that standard Win32 interface for process execution is somehow bypassed. The same works with MSYS2. In contrary, linker process (arm-none-eabi-gcc.exe) is native Win32 executable, unable to handle the huge argument list.

Do you have a test case that fails on master and passes with this branch?

Yes, here is my test plan:

Prerequisites

Windows 10 x64, Mbed CLI 1.8.3, Cygwin x64, make 4.2.1, GNU Tools ARM Embedded 6 2017-q2-update, GNU MCU Eclipse.

Failing test case

mbed import mbed-os-example-blinky
cd mbed-os-example-blinky
mbed export -m K64F -i eclipse_gcc_arm

Open GNU MCU Eclipse and import mbed-os-example-blinky

Set Eclipse project PATH to C:/cygwin64/bin;C:/Program Files (x86)/GNU Tools ARM Embedded/6 2017-q2-update/bin
eclipsepath

Project -> Build project

...
link: mbed-os-example-blinky.elf
make[1]: execvp: arm-none-eabi-gcc: Argument list too long
make[1]: *** [/cygdrive/d/mbed-os-example-blinky/Makefile:1605: mbed-os-example-blinky.elf] Error 127

Attached build_fail.log

Passing test case

Switch back to Mbed CLI

cd mbed-os
git fetch origin pull/9596/head:make-cmd-files
git checkout make-cmd-files
cd ..
# re-export the project
mbed export -m K64F -i eclipse_gcc_arm

Switch back to GNU MCU Eclipse
Remove BUILD directory
Project -> Build project

...
link: mbed-os-example-blinky.elf
arm-none-eabi-objcopy -O binary mbed-os-example-blinky.elf mbed-os-example-blinky.bin

Attached build_pass.log

I also checked the build passes with GNU Make 3.81 on macOS High Sierra.

@theotherjimmy

This comment has been minimized.

Copy link
Contributor

theotherjimmy commented Feb 5, 2019

my guess is that standard Win32 interface for process execution is somehow bypassed.

I'm willing to bet that it's a bash builtin. Perhaps we could switch this behavior based on make version? that way it's maximally compatible?

@theotherjimmy

This comment has been minimized.

Copy link
Contributor

theotherjimmy commented Feb 5, 2019

@vmedcy I appreciate the detailed test case.

@theotherjimmy
Copy link
Contributor

theotherjimmy left a comment

This is an improvement: all places where make worked before will still work, and places where make failed before may begin to work.

@0xc0170

0xc0170 approved these changes Feb 5, 2019

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Feb 5, 2019

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Feb 5, 2019

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 1
Build artifacts

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

NirSonnenschein commented Feb 6, 2019

@vmedcy , please add to the description the actual issue this fixes (not just a link).

@vmedcy

This comment has been minimized.

Copy link
Contributor Author

vmedcy commented Feb 6, 2019

@NirSonnenschein:

please add to the description the actual issue this fixes (not just a link).

I updated the PR description. I also added references to other issues addressed by this PR.

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Feb 7, 2019

Fixes the following issues

Quite number of issues! @cmonr will be surprised once he's back

@0xc0170 0xc0170 merged commit 7e18cc5 into ARMmbed:master Feb 7, 2019

27 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-ARM Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR 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-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR 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 9092 cycles (-122 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

@0xc0170 0xc0170 removed the ready for merge label Feb 7, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Feb 7, 2019

@vmedcy @theotherjimmy This is labeled as fix for 5.11.4. However, some windows users might be affected (as mentioned in ARMmbed/mbed-os-5-docs#935, gnuwin32 is not compatible. I have this installed locally as well). Thus this can be viewed as breaking change and should be in 5.12 rather with release notes mentioning this change?

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Feb 7, 2019

I've moved the release label to 5.12 , will wait for the confirmation and if this might affect users, we should add section to the release notes (following this doc update ARMmbed/mbed-os-5-docs#933)

@theotherjimmy

This comment has been minimized.

Copy link
Contributor

theotherjimmy commented Feb 7, 2019

@0xc0170 Please move this back to a patch release. As I said before:

all places where make worked before will still work, and places where make failed before may begin to work.

This does not require make version >=4, as it will work with make 3.81.

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Feb 7, 2019

Done, 5.11.4

@vidavidorra

This comment has been minimized.

Copy link
Contributor

vidavidorra commented Feb 18, 2019

@vmedcy @theotherjimmy Appologies to be posting in a closed PR, but will this also work for cmake (cmake_gcc_arm) exports?

@vmedcy

This comment has been minimized.

Copy link
Contributor Author

vmedcy commented Feb 18, 2019

@vidavidorra:

will this also work for cmake (cmake_gcc_arm) exports

No. This change is only applicable to makefile based exporters. CMake export produces the projects that can be compiled and linked on Windows without path length issues. I tested with Ninja generator:

# prerequisite: arm-none-eabi-gcc, cmake and ninja in PATH
mbed export -i cmake_gcc_arm -m K64F
cmake . -G Ninja
ninja
...
[730/730] Linking CXX executable mbed-os-example-blinky
-- built: C:/mbed/mbed-os-example-blinky/mbed-os-example-blinky.hex

By the way, ninja build is much faster than makefile build.

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.