Skip to content

using multiple linking flags when overriding a build property #532

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

Open
artynet opened this issue Dec 27, 2019 · 8 comments
Open

using multiple linking flags when overriding a build property #532

artynet opened this issue Dec 27, 2019 · 8 comments
Labels
topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project

Comments

@artynet
Copy link

artynet commented Dec 27, 2019

As described in the object I'd like to override the build properties of my custom boards.txt file leveraging all the benefits of the FreeRTOS samd21 library. For this reason, I have defined an empty build variable :

build.freertos.memory_wrapping_flags=

which is called at runtime with :

arduino-cli compile --fqbn <myboard> --build-properties build.freertos.memory_wrapping_flags="-Wl,--wrap=malloc -Wl,--wrap=free -Wl,--wrap=calloc -Wl,--wrap=realloc" mysketch.ino -vv

but just -Wl is set as its value so all which is located after the first comma is removed, altough enclosed within double quotes. I can see that because if I launch the command with the --show-properties option the output is just :

memory_wrapping_flags=-Wl

any hints about that ?

Environment

  • CLI version (output of arduino-cli version): 0.7.0 Commit: 3809fc3
  • OS and platform: Linux x64
@artynet artynet changed the title using mulitple values when overriding a build property using multiple linking flags when overriding a build property Dec 27, 2019
@artynet
Copy link
Author

artynet commented Dec 31, 2019

@per1234 @masci

this quick fix artynet@e8920fc seems to solve my problem. Would you mind taking a look at that so, in case, I can process a pull request ?

Thanks in advance

@masci
Copy link
Contributor

masci commented Dec 31, 2019

@artynet I think StringArrayVar is the way to go here, so +1 from me.
This will slightly change (for the better) the way --build-properties works, so we should also add an example in the README file showing how to pass multiple properties at once.

@artynet
Copy link
Author

artynet commented Dec 31, 2019

At this very point we should think of writing something like that. Given two sets of custom building flags :

  • build.platform_EXTRA_flags="flag1 flag2 flag3"
  • build.platform_GENERIC_flags="flag4 flag5 flag6"

we can override the main build properties this way :

--build-properties build.platform_GENERIC_flags="flag4 flag5 flag6 flag1 flag2 flag3"

or

--build-properties build.platform_GENERIC_flags="flag4 flag5 flag6" --build-properties build.platform_EXTRA_flags="flag1 flag2 flag3"

the first being a general override while the second a separated one for single set of flags. Defining :

--build-properties build.platform_GENERIC_flags="flag4 flag5 flag6",build.platform_EXTRA_flags="flag1 flag2 flag3"

would result in this merge :

"flag4 flag5 flag6",build.platform_EXTRA_flags="flag1 flag2 flag3"

where has no sense splitting an array by comma this time...

@artynet
Copy link
Author

artynet commented Jan 20, 2020

Hello @masci, any update on this issue ?

@masci
Copy link
Contributor

masci commented Jan 20, 2020

Ok for me to move on with a PR containing the comment linked above. Not sure I got the final example in your last comment: is that ok from a user perspective? Or is it a counter-example against changing that flag to StringArray?

Sorry I'm not a power Arduino user so I'm a bit slow at getting use cases 😛

@artynet
Copy link
Author

artynet commented Jan 20, 2020

Basically, we should tell the user to apply the switch for every single set of properties to override them. Comma separate values are not allowed anymore for multiple entries and, if passed that way, they won't be split as it should.

The same issue might apply to the libraries switch just introduced a couple of hours ago....please be aware of that !

@matthijskooijman
Copy link
Collaborator

No longer allowing comma-separated values seems like a good solution here. There should usually not be any problem with just supplying the option multiple times, and using a comma to separate without any means to escape literal commas is problematic.

We could just switch the meaning of the current --build-properties flag (as implemented in #532 (comment)), but if we do that, it's a bit weird that --build-properties is plural, while it only allows specifying a single property. It also breaks backward compatibility.

An alternative approach would be to introduce a new --build-property flag, with the new semantics, and keeping the old flag for now, but mark it as deprecated. How is that?

@artynet
Copy link
Author

artynet commented Sep 10, 2020

@matthijskooijman that sounds right to me as well

per1234 referenced this issue in MarginallyClever/Makelangelo-firmware Nov 13, 2020
@per1234 per1234 reopened this Mar 30, 2021
@fstasi fstasi removed the type: bug label Sep 16, 2021
@rsora rsora added the type: imperfection Perceived defect in any part of project label Sep 22, 2021
@per1234 per1234 added the topic: code Related to content of the project itself label Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Development

No branches or pull requests

6 participants