arduino-builder improperly escapes some paths in machine output

[matthijskooijman]

This is a problem in arduino-builder, I believe only in legacy code, but since all the relevant code lives in this repo, I thought to report this in this repository.

In some (debug) output lines of the Arduino builder, in machine-readable logger mode, paths are not properly escaped. This causes problems with spaces in the path, where the IDE shows e.g.

Using board 'uno' from platform in folder C:\Program

(I typed this error message from memory, since I did not have a broken setup to reproduce it at hand).

I think this is easy to reproduce by installing the IDE in a path with spaces (which is the default on Windows).

To show the problem by running arduino-builder manually, we need a hardware folder with spaces. E.g.:

git clone https://github.com/arduino/ArduinoCore-avr hardware\ with\ spaces/arduino/avr

Then, you can run arduino-builder against that. e.g. compare arduino-builder 1.4.4:

$ arduino-builder-1.4.4 -verbose=true -logger machine -hardware hardware\ with\ spaces/ -tools ~/.arduino15/packages/arduino/tools/ -fqbn arduino:avr:uno -compile Sketch/  |grep Using
===info ||| Using board '{0}' from platform in folder: {1} ||| [uno %2Fhome%2Fmatthijs%2Fhardware+with+spaces%2Farduino%2Favr]
===info ||| Using core '{0}' from platform in folder: {1} ||| [arduino %2Fhome%2Fmatthijs%2Fhardware+with+spaces%2Farduino%2Favr]

And current master:

$ arduino-builder -verbose=true -logger machine -hardware hardware\ with\ spaces/ -tools ~/.arduino15/packages/arduino/tools/ -fqbn arduino:avr:uno -compile Sketch/  |grep Using
===info ||| Using board '{0}' from platform in folder: {1} ||| [uno /home/matthijs/hardware with spaces/arduino/avr]
===info ||| Using core '{0}' from platform in folder: {1} ||| [arduino /home/matthijs/hardware with spaces/arduino/avr]

As you can see, the path was encoded originally, but no longer since the arduino-cli inception happened.

The reason is that the path printed changed from a string:
https://github.com/arduino/arduino-builder/blob/cb100d0e47f68cba82430f376157d860d79c1d83/target_board_resolver.go#L104-L105
https://github.com/arduino/arduino-builder/blob/cb100d0e47f68cba82430f376157d860d79c1d83/types/types.go#L140

To a paths.Path object:

arduino-cli/legacy/builder/target_board_resolver.go

Lines 60 to 61 in 7663d7e

	logger.Println(constants.LOG_LEVEL_INFO, constants.MSG_USING_BOARD, targetBoard.BoardID, targetPlatform.InstallDir)
	logger.Println(constants.LOG_LEVEL_INFO, constants.MSG_USING_CORE, core, actualPlatform.InstallDir)

arduino-cli/arduino/cores/cores.go

Line 51 in 7663d7e

InstallDir *paths.Path `json:"-"`

However, the machine logger only treats strings specially by encoding them, and passes all values as-is to fmt.Fprintf():

arduino-cli/legacy/builder/i18n/i18n.go

Lines 223 to 232 in 7663d7e

	func printMachineFormattedLogLine(w io.Writer, level string, format string, a []interface{}) {
	a = append([]interface{}(nil), a...)
	for idx, value := range a {
	typeof := reflect.Indirect(reflect.ValueOf(value)).Kind()
	if typeof == reflect.String {
	a[idx] = url.QueryEscape(value.(string))
	}
	}
	fprintf(w, "===%s ||| %s ||| %s\n", level, format, a)
	}

I guess this is to encode strings, but leave floats and integers unchanged? Or are there other types that do their own encoding? I'm not too familiar with this code, or the machine-readable logger interface.

A fix that works is to simply use InstallDir.String(), forcing these paths to be a string. This approach is not ideal, since it is easy to forget the String (though if this code is to be removed soon, that might be ok). The code should be checked for similar cases, though.

An alternative would be to fix this in the logger, perhaps by checking all arguments to see if they implement the Stringer interface (i.e. have a String() method) and if so, call that and encode the resulting string (I guess this is also what Fprintf does with Stringer types, except for the encoding). Unless there are types that need to do their own encoding, but I doubt that (would also be bad design, really).

[matthijskooijman]

Note that this same problem seems to occur in the "Multiple libraries found" message, see the issue linked above.