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

compiler.libraries.ldflags= was missing, generating an error when trying to link precompiled libraries #8392

Merged
merged 4 commits into from
Dec 2, 2021

Conversation

marcel-licence
Copy link
Contributor

This line is required to be compatible with the Arduino library specification

…ing to link precompiled libraries.

This line is required to be compatible with the Arduino library specification
Copy link
Collaborator

@mcspr mcspr left a comment

Choose a reason for hiding this comment

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

Should this also be added to the combine pattern here? I have noticed esp32 platform.txt using this property
https://github.com/espressif/arduino-esp32/blob/399f4ecbb3a4cef21e2bffa37adb6190356dfb76/platform.txt#L151

recipe.c.combine.pattern="{compiler.path}{compiler.c.elf.cmd}" {build.exception_flags} -Wl,-Map "-Wl,{build.path}/{build.project_name}.map" {compiler.c.elf.flags} {compiler.c.elf.extra_flags} -o "{build.path}/{build.project_name}.elf" -Wl,--start-group {object_files} "{archive_file_path}" {compiler.c.elf.libs} -Wl,--end-group "-L{build.path}"

@mcspr
Copy link
Collaborator

mcspr commented Nov 28, 2021

Also, some references
https://arduino.github.io/arduino-cli/0.20/platform-specification/#recipes-for-linking

Turns out this was reported to the Arduino repo last year
arduino/Arduino#10473
arduino/arduino-cli#840

@marcel-licence
Copy link
Contributor Author

I compiled with and without the requested change and couldn't spot a difference. I am not so familar with arduino to understand what this line should do.
If you like I can just add this to the push request but it was not required to link the library.

@mcspr
Copy link
Collaborator

mcspr commented Nov 29, 2021

I compiled with and without the requested change and couldn't spot a difference. I am not so familar with arduino to understand what this line should do.
If you like I can just add this to the push request but it was not required to link the library.

Please do. We should also fix the comment to reference the reason why we need this?


tldr; this function of the arduino-cli builder uses the combine pattern:
https://github.com/arduino/arduino-cli/blob/e31a717ebc34fff80892dbbe25a9d4581906e13a/legacy/builder/phases/linker.go#L66-L123

grabbing https://github.com/BoschSensortec/BSEC-Arduino-library to reproduce this, it does not build without adding {compiler.libraries.ldflags} to the pattern group

@marcel-licence
Copy link
Contributor Author

Okay, I've added this to the pattern group

@mcspr mcspr merged commit 7fc43b6 into esp8266:master Dec 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants