arduino-builder on Travis-CI Win32 instance error reporting results in another error, "errors.errorString invalid syntax"

[earlephilhower]

Bug Report

Just to be up front, I'm not sure if this is an error in arduino-builder(really, inside -cli now) or an issue with the beta Travis-CI Windows VMs.

When trying to report some error, arduino-builder actually ends up with an exception in the error printing routines themselves. (I can't really identify the first error it's trying to report exactly, since its crashing while trying to print it, but I believe from the trace it's related to a hardware directory issue in my CI setup.) I'm fine with having a bug in my new CI config, but with it crashing I can't actually see what the problem is.

Current behavior

(taken from an esp8266-Arduino CI run on Travis-CI under Windows at https://api.travis-ci.org/v3/job/582174496/log.txt )

C:\Users\travis\arduino_ide\arduino-builder -compile -logger=human -build-path "C:\Users\travis\build\esp8266\Arduino\build.tmp" -tools "C:\Users\travis\arduino_ide\tools-builder" -libraries "C:\Users\travis\Arduino\libraries" -hardware "C:\Users\travis\arduino_ide\hardware" -hardware "C:\Users\travis\build\esp8266\Arduino\tools\..\cores" -fqbn=esp8266com:esp8266:generic:xtal=80,FlashFreq=40,FlashMode=qio,baud=921600,eesz=4M1M,ip=lm2f,ResetMethod=nodemcu -built-in-libraries "C:\Users\travis\arduino_ide\libraries" -ide-version=10607 -warnings=all -verbose C:\Users\travis\build\esp8266\Arduino\libraries\ArduinoOTA\examples\BasicOTA\BasicOTA.ino


...

*errors.errorString invalid syntax
/home/jenkins/workspace/arduino-builder-all-cross-cli-inception/src/github.com/arduino/arduino-cli/legacy/builder/i18n/errors.go:19 (0x858073)
/home/jenkins/workspace/arduino-builder-all-cross-cli-inception/src/github.com/arduino/arduino-builder/main.go:440 (0x85804a)
/home/jenkins/workspace/arduino-builder-all-cross-cli-inception/src/github.com/arduino/arduino-builder/main.go:249 (0x857910)
/home/jenkins/go1.12.5/src/runtime/proc.go:200 (0x42bc87)
/home/jenkins/go1.12.5/src/runtime/asm_386.s:1321 (0x4526f1)

Expected behavior

I'd expect the appropriate error message to be displayed.

Environment

• CLI version (output of arduino-cli version): arduino-builder nightly from https://downloads.arduino.cc/arduino-nightly-linux64.tar.xz
• OS and platform: Win32 on Travis-CI

[facchinm]

From the log it looks like something horrible is going on with paths handling (unix -> win -> cygwin-like)

 ------------ Building /c/Users/travis/build/esp8266/Arduino/libraries/ArduinoOTA/examples/BasicOTA/BasicOTA.ino ------------ 

python3 tools/build.py -b generic -v -w all -s 4M1M -v -k -p C:/Users/travis/build/esp8266/Arduino/build.tmp -n lm2f -l C:/Users/travis/Arduino/libraries  C:/Users/travis/build/esp8266/Arduino/libraries/ArduinoOTA/examples/BasicOTA/BasicOTA.ino

Which version of the builder are you using? 1.4.6? Found it 😉
@cmaglie

[facchinm]

Ah, and I'm moving back the issue to arduino-builder repo

[earlephilhower]

Thanks @facchinm for looking at it.

The slash conversion is handled in our Python scripts which are Linux/MacOS native and patched minimally to run within the Win32 bash that Travis-CI uses.

So the command is printed with /(forward slashes), but we do a cmdline = cmdline.replace('/', '\\') before splitting it up and calling the actual Python subprocess.exec() on it.

The exec line boils down to:
p = subprocess.Popen('"C:\Users\travis\arduino_ide\arduino-builder -compile -logger=human -build-path "C:\Users\travis\build\esp8266\Arduino\build.tmp" -tools "C:\Users\travis\arduino_ide\tools-builder" -libraries "C:\Users\travis\Arduino\libraries" -hardware "C:\Users\travis\arduino_ide\hardware" -hardware "C:\Users\travis\build\esp8266\Arduino\tools\..\cores" -fqbn=esp8266com:esp8266:generic:xtal=80,FlashFreq=40,FlashMode=qio,baud=921600,eesz=4M1M,ip=lm2f,ResetMethod=nodemcu -built-in-libraries "C:\Users\travis\arduino_ide\libraries" -ide-version=10607 -warnings=all -verbose C:\Users\travis\build\esp8266\Arduino\libraries\ArduinoOTA\examples\BasicOTA\BasicOTA.ino"'.split(' '), stdout=f, stderr=subprocess.STDOUT)

I can definitely believe that there's something mistaken there, but I would expect an error message and not an exception trying to print the actual error message. :)

[cmaglie]

@earlephilhower in theory windows should handle both forward and backward slashes, I think you don't need to do the conversion at all.

[earlephilhower]

Yes, @cmaglie, I believe you're right. I started without changing the slash direction, got the same go exception when printing an error message, so I tried to normalize everything to Windows paths.

The exception backtrace points to a simple string adjustment failing, not even looking at the directories passes in yet (line 249):

arduino-builder/main.go

Lines 247 to 249 in bc2846e

	// FLAG_HARDWARE
	if hardwareFolders, err := toSliceOfUnquoted(hardwareFoldersFlag); err != nil {
	printCompleteError(err)

with toSliceOfUnquoted (simple string list manipulation) returning an error which the above line is trying to print out...

arduino-builder/main.go

Lines 427 to 437 in bc2846e

	func toSliceOfUnquoted(value []string) ([]string, error) {
	var values []string
	for _, v := range value {
	v, err := gohasissues.Unquote(v)
	if err != nil {
	return nil, err
	}
	values = append(values, v)
	}
	return values, nil
	}

[cmaglie]

yes it comes from gohasissues.Unquote(v) let me check what's happening

[cmaglie]

ok, it seems that there are many things going on here, but let's start from your call to popen:

p = subprocess.Popen('"C:\Users\travis\arduino_ide\arduino-builder -compile -logger=human -build-path "C:\Users\travis\build\esp8266\Arduino\build.tmp" -tools "C:\Users\travis\arduino_ide\tools-builder" -libraries "C:\Users\travis\Arduino\libraries" -hardware "C:\Users\travis\arduino_ide\hardware" -hardware "C:\Users\travis\build\esp8266\Arduino\tools\..\cores" -fqbn=esp8266com:esp8266:generic:xtal=80,FlashFreq=40,FlashMode=qio,baud=921600,eesz=4M1M,ip=lm2f,ResetMethod=nodemcu -built-in-libraries "C:\Users\travis\arduino_ide\libraries" -ide-version=10607 -warnings=all -verbose C:\Users\travis\build\esp8266\Arduino\libraries\ArduinoOTA\examples\BasicOTA\BasicOTA.ino"'.split(' '), stdout=f, stderr=subprocess.STDOUT)

if I read it correctly, the command line arguments are obtained by splitting on spaces the string

"C:\Users\travis\arduino_ide\arduino-builder -compile -logger=human -build-path "C:\Users\travis\build\esp8266\Arduino\build.tmp" -tools "C:\Users\travis\arduino_ide\tools-builder" -libraries "C:\Users\travis\Arduino\libraries" -hardware "C:\Users\travis\arduino_ide\hardware" -hardware "C:\Users\travis\build\esp8266\Arduino\tools\..\cores" -fqbn=esp8266com:esp8266:generic:xtal=80,FlashFreq=40,FlashMode=qio,baud=921600,eesz=4M1M,ip=lm2f,ResetMethod=nodemcu -built-in-libraries "C:\Users\travis\arduino_ide\libraries" -ide-version=10607 -warnings=all -verbose C:\Users\travis\build\esp8266\Arduino\libraries\ArduinoOTA\examples\BasicOTA\BasicOTA.ino"

so the resulting array of arguments should be (one arg per line):

"C:\Users\travis\arduino_ide\arduino-builder
-compile
-logger=human
-build-path
"C:\Users\travis\build\esp8266\Arduino\build.tmp"
-tools
"C:\Users\travis\arduino_ide\tools-builder"
-libraries
"C:\Users\travis\Arduino\libraries"
-hardware
"C:\Users\travis\arduino_ide\hardware"
-hardware
"C:\Users\travis\build\esp8266\Arduino\tools\..\cores"
-fqbn=esp8266com:esp8266:generic:xtal=80,FlashFreq=40,FlashMode=qio,baud=921600,eesz=4M1M,ip=lm2f,ResetMethod=nodemcu -built-in-libraries
"C:\Users\travis\arduino_ide\libraries"
-ide-version=10607
-warnings=all
-verbose
C:\Users\travis\build\esp8266\Arduino\libraries\ArduinoOTA\examples\BasicOTA\BasicOTA.ino"

I think that you have an extra pair of quotes on each path argument that is passed to the builder process as-is. I think that this is wrong you may probably want to try to edit the command line and remove the extra quotes (or maybe create directly an array without string splitting).

The function gohasissues.Unquote(...) is defined as:

func Unquote(s string) (string, error) {
    if stringStartsEndsWith(s, "'") {
        s = s[1 : len(s)-1]
    }

    if !stringStartsEndsWith(s, "\"") {
        return s, nil
    }

    return strconv.Unquote(s)
}

as we can see, if the argument startsAndEndsWith quotes is passed through the strconv.Unquote that probably tries to interpret the backslash as escape code (like \n or \") but since it's part of a path it will have no meaning or, most likely, wrong syntax.

So I'll go ahead and fix the function, but if my analysis is correct, you should remove anyway the extra quotes from the parameters as I said.

[earlephilhower]

Thanks, @cmaglie . The extra quote was a cut-n-paste error in my description above. The actual params passed in to the Popen are programmatically generated, of course (we run 100s of tests), but your observation about the quotes on the parameter paths is correct.

We do have quotes around them, I will remove them and drop the backslash format (I don't see how users will be able to specify the drive, unfortunately, w/o backslash paths so I think your fix will ultimately be needed by generic Win users who might be running Arduino standalone from, say, a USB stick, or a d:\Users directory on a VDI instance)

[earlephilhower]

I can confirm that removing any quotes around paths does fix the issue. Thanks for your help!

[earlephilhower]

We're running fine, now. I'll close this since it looks like my own problem.

As an aside, in my usage I actually did need the \ notation for paths passed in to arduino-builder, or it had trouble finding the ctags executable. I didn't go into it deeply, but for the ESP8266 Arduino Windows CI we're running with c:..... paths just fine.

Thanks for looking into it!