Properties expansions inside `build.variant` break core build

[matthijskooijman]

Bug Report

Current behavior

With the following modification to the the AVR core:

--- a/boards.txt
+++ b/boards.txt
@@ -75,7 +75,8 @@ uno.build.mcu=atmega328p
 uno.build.f_cpu=16000000L
 uno.build.board=AVR_UNO
 uno.build.core=arduino
-uno.build.variant=standard
+uno.build.variant={build.realvariant}
+uno.build.realvariant=standard
 
 ##############################################################
 

(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:

arduino-cli compile /path/to/arduino/examples/01.Basics/BareMinimum -b arduino-git:avr:uno -v

(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:

Compiling sketch...
/home/matthijs/.arduino15/packages/arduino/tools/avr-gcc/7.3.0-atmel3.6.1-arduino5/bin/avr-g++ -c -g -Os -w -std=gnu++11 -fpermissive -fno-exceptions -ffunctio
n-sections -fdata-sections -fno-threadsafe-statics -Wno-error=narrowing -MMD -flto -mmcu=atmega328p -DF_CPU=16000000L -DARDUINO=10607 -DARDUINO_AVR_UNO -DARDUI
NO_ARCH_AVR -I/home/matthijs/docs/Electronics/Arduino/hardware/arduino-git/avr/cores/arduino -I/home/matthijs/docs/Electronics/Arduino/hardware/arduino-git/avr
/variants/standard /tmp/arduino-sketch-96B93CFDA539CD461E0E63C5A43503EE/sketch/BareMinimum.ino.cpp -o /tmp/arduino-sketch-96B93CFDA539CD461E0E63C5A43503EE/sket
ch/BareMinimum.ino.cpp.o

[ ... output snipped ... ]

Compiling core...
/home/matthijs/.arduino15/packages/arduino/tools/avr-gcc/7.3.0-atmel3.6.1-arduino5/bin/avr-gcc -c -g -Os -w -std=gnu11 -ffunction-sections -fdata-sections -MMD -flto -fno-fat-lto-objects -mmcu=atmega328p -DF_CPU=16000000L -DARDUINO=10607 -DARDUINO_AVR_UNO -DARDUINO_ARCH_AVR -I/home/matthijs/docs/Electronics/Arduino/hardware/arduino-git/avr/cores/arduino /home/matthijs/docs/Electronics/Arduino/hardware/arduino-git/avr/cores/arduino/WInterrupts.c -o /tmp/arduino-sketch-96B93CFDA539CD461E0E63C5A43503EE/core/WInterrupts.c.o
In file included from /home/matthijs/docs/Electronics/Arduino/hardware/arduino-git/avr/cores/arduino/wiring_private.h:31:0,
                 from /home/matthijs/docs/Electronics/Arduino/hardware/arduino-git/avr/cores/arduino/wiring.c:23:
/home/matthijs/docs/Electronics/Arduino/hardware/arduino-git/avr/cores/arduino/Arduino.h:258:10: fatal error: pins_arduino.h: No such file or directory
 #include "pins_arduino.h"
          ^~~~~~~~~~~~~~~~
compilation terminated.

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

• CLI version (output of arduino-cli version): arduino-cli Version: 0.0.0-git Commit: 7541c80
  (todays git master)
• OS and platform: Debian/Linux on amd64

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

commandLine := buildProperties.ExpandPropsInString(pattern)

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

	variantFolder := buildProperties.GetPath("build.variant.path")
	targetCoreFolder := buildProperties.GetPath(constants.BUILD_PROPERTIES_RUNTIME_PLATFORM_PATH)
	includes := []string{}
	includes = append(includes, coreFolder.String())
	if variantFolder != nil && variantFolder.IsDir() {
	includes = append(includes, variantFolder.String())
	}
	includes = utils.Map(includes, utils.WrapWithHyphenI)

Note that this looks at core.variant.path, which is the same as build.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.

[cmaglie]

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.

@matthijskooijman I agree, would you mind making a PR for that?

[matthijskooijman]

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 :-)

[cmaglie]

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

[matthijskooijman]

@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.