-
-
Notifications
You must be signed in to change notification settings - Fork 370
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
Properties expansions inside build.variant
break core build
#762
Comments
build.variant
build.variant
break core build
@matthijskooijman I agree, would you mind making a PR for that? |
I'll put it on my TODO list, but won't have time for that right now. If anyone else wants to pick this up in the meantime, feel free :-) |
After the recent improvements to the build-properties generation, this bug has become much easier to fix. @matthijskooijman if you want to give it a try here is the patch: #2176 |
@cmaglie Thanks for picking this up. From the PR, it looks like it seems like an easy fix indeed. As for testing - I won't find time for that - I'd also have to recreate a testcase just for this, since I believe we fixed the original problem in STM32 in a different way (though I don't recall the details). I'll tag @fpistm here, though, in case he maybe has further use for this fix and/or wants to test it. |
Bug Report
Current behavior
With the following modification to the the AVR core:
(This is a bit of a contrived and simplified example, the original usecase was to specify a part of the variant in a menu rather than the direct board definition, see stm32duino/Arduino_Core_STM32#1091 for the more complete usecase).
Run:
(Note that I use
arduino-git
in the FQBN to get the modified version from my sketchbook rather than the board-manager installed version)This produces the following error:
Note that the compiler command has no
-I
option for the variant when compiling the core (but it is present for the sketch).Expected behavior
I would expect this to build without problems, using
build.variant=standard
Environment
arduino-cli version
): arduino-cli Version: 0.0.0-git Commit: 7541c80(todays git master)
Additional context
It seems the problem here is that normally property expansion happens very late, basically when interpreting the compilation recipe:
arduino-cli/legacy/builder/builder_utils/utils.go
Line 511 in 28a1c4c
For most properties that are included in the recipe, this is fine, since then all references will be recursively processed at the end. However, when compiling the core, the
build.variant
property is not included as-is, but its raw value (without expanding properties) is checked to be a valid directory name, if not it is ignored:arduino-cli/legacy/builder/phases/core_builder.go
Lines 68 to 77 in 28a1c4c
Note that this looks at
core.variant.path
, which is the same asbuild.variant
but as a full path.Also note that for sketches, the value is added to the commandline as-is (through
ctx.IncludeFolders
generated by the includes finder).An easy fix could be to just to drop the
isDir()
check here (replacing it with an empty string check, I think). If an invalid directory is specified, that would previously silently be dropped, and then it would just be passed to gcc which I think would ignore it anyway. Also, the sketch and library compilation steps already just add the variant to the commandline without checking, so it seems this check does not really add anything anyway.The text was updated successfully, but these errors were encountered: