Fixed compile error when sketch has a broken symlink (#2497)

* Removed unneeded pointer

* Improved error message

* Update go-paths library

* Added integration tests

* Reduced timeout for symlink-loop tests

* Fixed unit tests

Previously the unit tests were creating the wrong env to test:

internal/arduino/libraries/testdata/TestLib
├── examples
│   ├── UpGoer1 -> testdata/TestLib
│   └── UpGoer2 -> testdata/TestLib
├── library.properties
└── src
    └── TestLib.h

The two UpGoer1 and UpGoer2 are broken links.
The correct tree is the following:

internal/arduino/libraries/testdata/TestLib
├── examples
│   ├── UpGoer1 -> ..
│   └── UpGoer2 -> ..
├── library.properties
└── src
    └── TestLib.h

that actually triggers the symlink loop we are testing.

* Fixed integration test

* Removed apparently useless check for "readable" files

Author: cmaglie

.licenses/go/github.com/arduino/go-paths-helper.dep.yml

@@ -1,6 +1,6 @@
 ---
 name: github.com/arduino/go-paths-helper
-version: v1.11.0
+version: v1.12.0
 type: go
 summary: 
 homepage: https://pkg.go.dev/github.com/arduino/go-paths-helper

go.mod

@@ -7,7 +7,7 @@ replace github.com/mailru/easyjson => github.com/cmaglie/easyjson v0.8.1
 
 require (
 	github.com/ProtonMail/go-crypto v0.0.0-20230828082145-3c4c8a2d2371
-	github.com/arduino/go-paths-helper v1.11.0
+	github.com/arduino/go-paths-helper v1.12.0
 	github.com/arduino/go-properties-orderedmap v1.8.0
 	github.com/arduino/go-timeutils v0.0.0-20171220113728-d1dd9e313b1b
 	github.com/arduino/go-win32-utils v1.0.0

go.sum

@@ -11,8 +11,8 @@ github.com/acomagu/bufpipe v1.0.3/go.mod h1:mxdxdup/WdsKVreO5GpW4+M/1CE2sMG4jeGJ
 github.com/anmitsu/go-shlex v0.0.0-20161002113705-648efa622239 h1:kFOfPq6dUM1hTo4JG6LR5AXSUEsOjtdm0kw0FtQtMJA=
 github.com/anmitsu/go-shlex v0.0.0-20161002113705-648efa622239/go.mod h1:2FmKhYUyUczH0OGQWaF5ceTx0UBShxjsH6f8oGKYe2c=
 github.com/arduino/go-paths-helper v1.0.1/go.mod h1:HpxtKph+g238EJHq4geEPv9p+gl3v5YYu35Yb+w31Ck=
-github.com/arduino/go-paths-helper v1.11.0 h1:hkpGb9AtCTByTj2FKutuHWb3klDf4kAKL10hW+fN+oE=
-github.com/arduino/go-paths-helper v1.11.0/go.mod h1:jcpW4wr0u69GlXhTYydsdsqAjLaYK5n7oWHfKqOG6LM=
+github.com/arduino/go-paths-helper v1.12.0 h1:xizOQtI9iHdl19qXd1EmWg5i9W//2bOCOYwlNv8F61E=
+github.com/arduino/go-paths-helper v1.12.0/go.mod h1:jcpW4wr0u69GlXhTYydsdsqAjLaYK5n7oWHfKqOG6LM=
 github.com/arduino/go-properties-orderedmap v1.8.0 h1:wEfa6hHdpezrVOh787OmClsf/Kd8qB+zE3P2Xbrn0CQ=
 github.com/arduino/go-properties-orderedmap v1.8.0/go.mod h1:DKjD2VXY/NZmlingh4lSFMEYCVubfeArCsGPGDwb2yk=
 github.com/arduino/go-timeutils v0.0.0-20171220113728-d1dd9e313b1b h1:9hDi4F2st6dbLC3y4i02zFT5quS4X6iioWifGlVwfy4=

internal/arduino/libraries/libraries_test.go

@@ -95,15 +95,16 @@ func TestLibrariesLoader(t *testing.T) {
 func TestSymlinkLoop(t *testing.T) {
 	// Set up directory structure of test library.
 	testLib := paths.New("testdata", "TestLib")
-	examplesPath := testLib.Join("examples")
+	examplesPath, err := testLib.Join("examples").Abs()
+	require.NoError(t, err)
 	require.NoError(t, examplesPath.Mkdir())
 	defer examplesPath.RemoveAll()
 
 	// It's probably most friendly for contributors using Windows to create the symlinks needed for the test on demand.
-	err := os.Symlink(examplesPath.Join("..").String(), examplesPath.Join("UpGoer1").String())
+	err = os.Symlink(examplesPath.String(), examplesPath.Join("UpGoer1").String())
 	require.NoError(t, err, "This test must be run as administrator on Windows to have symlink creation privilege.")
 	// It's necessary to have multiple symlinks to a parent directory to create the loop.
-	err = os.Symlink(examplesPath.Join("..").String(), examplesPath.Join("UpGoer2").String())
+	err = os.Symlink(examplesPath.String(), examplesPath.Join("UpGoer2").String())
 	require.NoError(t, err)
 
 	// The failure condition is Load() never returning, testing for which requires setting up a timeout.
@@ -123,15 +124,16 @@ func TestSymlinkLoop(t *testing.T) {
 func TestLegacySymlinkLoop(t *testing.T) {
 	// Set up directory structure of test library.
 	testLib := paths.New("testdata", "LegacyLib")
-	examplesPath := testLib.Join("examples")
+	examplesPath, err := testLib.Join("examples").Abs()
+	require.NoError(t, err)
 	require.NoError(t, examplesPath.Mkdir())
 	defer examplesPath.RemoveAll()
 
 	// It's probably most friendly for contributors using Windows to create the symlinks needed for the test on demand.
-	err := os.Symlink(examplesPath.Join("..").String(), examplesPath.Join("UpGoer1").String())
+	err = os.Symlink(examplesPath.String(), examplesPath.Join("UpGoer1").String())
 	require.NoError(t, err, "This test must be run as administrator on Windows to have symlink creation privilege.")
 	// It's necessary to have multiple symlinks to a parent directory to create the loop.
-	err = os.Symlink(examplesPath.Join("..").String(), examplesPath.Join("UpGoer2").String())
+	err = os.Symlink(examplesPath.String(), examplesPath.Join("UpGoer2").String())
 	require.NoError(t, err)
 
 	// The failure condition is Load() never returning, testing for which requires setting up a timeout.

internal/arduino/sketch/sketch.go

@@ -111,18 +111,11 @@ func New(path *paths.Path) (*Sketch, error) {
 
 	sketchFolderFiles, err := sketch.supportedFiles()
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("%s: %w", tr("reading sketch files"), err)
 	}
 
 	// Collect files
-	for _, p := range *sketchFolderFiles {
-		// Skip files that can't be opened
-		f, err := p.Open()
-		if err != nil {
-			continue
-		}
-		f.Close()
-
+	for _, p := range sketchFolderFiles {
 		ext := p.Ext()
 		if globals.MainFileValidExtensions[ext] {
 			if p.EqualsTo(mainFile) {
@@ -160,7 +153,7 @@ func New(path *paths.Path) (*Sketch, error) {
 
 // supportedFiles reads all files recursively contained in Sketch and
 // filter out unneded or unsupported ones and returns them
-func (s *Sketch) supportedFiles() (*paths.PathList, error) {
+func (s *Sketch) supportedFiles() (paths.PathList, error) {
 	filterValidExtensions := func(p *paths.Path) bool {
 		return globals.MainFileValidExtensions[p.Ext()] || globals.AdditionalFileValidExtensions[p.Ext()]
 	}
@@ -180,7 +173,7 @@ func (s *Sketch) supportedFiles() (*paths.PathList, error) {
 	if err != nil {
 		return nil, err
 	}
-	return &files, nil
+	return files, nil
 }
 
 // GetProfile returns the requested profile or an error if not found

internal/arduino/sketch/sketch_test.go

@@ -341,7 +341,7 @@ func TestNewSketchWithSymlinkLoop(t *testing.T) {
 				return false
 			}
 		},
-		20*time.Second,
+		5*time.Second,
 		10*time.Millisecond,
 		"Infinite symlink loop while loading sketch",
 	)
@@ -380,7 +380,7 @@ func TestSketchWithMultipleSymlinkLoops(t *testing.T) {
 				return false
 			}
 		},
-		20*time.Second,
+		5*time.Second,
 		10*time.Millisecond,
 		"Infinite symlink loop while loading sketch",
 	)

internal/integrationtest/compile_1/compile_test.go

@@ -220,16 +220,36 @@ func compileWithSketchWithSymlinkSelfloop(t *testing.T, env *integrationtest.Env
 		require.NoError(t, err)
 		require.Contains(t, string(stdout), "Sketch created in: "+sketchPath.String())
 
-		// create a symlink that loops on himself
+		// Create a symlink that loops on himself
+		//
+		// /tmp/cli2843369229/Arduino/CompileIntegrationTestSymlinkSelfLoop
+		// ├── CompileIntegrationTestSymlinkSelfLoop.ino
+		// └── loop -> /tmp/cli2843369229/Arduino/CompileIntegrationTestSymlinkSelfLoop/loop
+		//
+		// in this case the link is "broken", and it will be ignored by the compiler
 		loopFilePath := sketchPath.Join("loop")
 		err = os.Symlink(loopFilePath.String(), loopFilePath.String())
 		require.NoError(t, err)
 
-		// Build sketch for arduino:avr:uno
+		_, _, err = cli.Run("compile", "-b", fqbn, sketchPath.String())
+		require.NoError(t, err)
+
+		// Add a symlink that loops on himself named as a .ino file
+		//
+		// /tmp/cli2843369229/Arduino/CompileIntegrationTestSymlinkSelfLoop
+		// ├── CompileIntegrationTestSymlinkSelfLoop.ino
+		// ├── loop -> /tmp/cli2843369229/Arduino/CompileIntegrationTestSymlinkSelfLoop/loop
+		// └── loop.ino -> /tmp/cli2843369229/Arduino/CompileIntegrationTestSymlinkSelfLoop/loop.ino
+		//
+		// in this case the new link is "broken" as before, but being part of the sketch will trigger an error.
+		loopInoFilePath := sketchPath.Join("loop.ino")
+		err = os.Symlink(loopFilePath.String(), loopInoFilePath.String())
+		require.NoError(t, err)
+
 		_, stderr, err := cli.Run("compile", "-b", fqbn, sketchPath.String())
 		// The assertion is a bit relaxed in this case because win behaves differently from macOs and linux
 		// returning a different error detailed message
-		require.Contains(t, string(stderr), "Can't open sketch:")
+		require.Contains(t, string(stderr), "Error during build:")
 		require.Error(t, err)
 	}
 	{

internal/integrationtest/compile_4/broken_symlink_test.go

@@ -0,0 +1,54 @@
+// This file is part of arduino-cli.
+//
+// Copyright 2023 ARDUINO SA (http://www.arduino.cc/)
+//
+// This software is released under the GNU General Public License version 3,
+// which covers the main part of arduino-cli.
+// The terms of this license can be found at:
+// https://www.gnu.org/licenses/gpl-3.0.en.html
+//
+// You can be released from the requirements of the above licenses by purchasing
+// a commercial license. Buying such a license is mandatory if you want to
+// modify or otherwise use the software for commercial activities involving the
+// Arduino software without disclosing the source code of your own applications.
+// To purchase a commercial license, send an email to [email protected].
+
+package compile_test
+
+import (
+	"testing"
+
+	"github.com/arduino/arduino-cli/internal/integrationtest"
+	"github.com/arduino/go-paths-helper"
+	"github.com/stretchr/testify/require"
+)
+
+func TestCompileWithBrokenSymLinks(t *testing.T) {
+	env, cli := integrationtest.CreateArduinoCLIWithEnvironment(t)
+	t.Cleanup(env.CleanUp)
+
+	// Install Arduino AVR Boards
+	_, _, err := cli.Run("core", "install", "arduino:[email protected]")
+	require.NoError(t, err)
+
+	t.Run("NonSketchFileBroken", func(t *testing.T) {
+		sketch, err := paths.New("testdata", "ValidSketchWithBrokenSymlink").Abs()
+		require.NoError(t, err)
+		_, _, err = cli.Run("compile", "-b", "arduino:avr:uno", sketch.String())
+		require.NoError(t, err)
+	})
+
+	t.Run("SketchFileBroken", func(t *testing.T) {
+		sketch, err := paths.New("testdata", "ValidSketchWithBrokenSketchFileSymlink").Abs()
+		require.NoError(t, err)
+		_, _, err = cli.Run("compile", "-b", "arduino:avr:uno", sketch.String())
+		require.Error(t, err)
+	})
+
+	t.Run("NonInoSketchFileBroken", func(t *testing.T) {
+		sketch, err := paths.New("testdata", "ValidSketchWithNonInoBrokenSketchFileSymlink").Abs()
+		require.NoError(t, err)
+		_, _, err = cli.Run("compile", "-b", "arduino:avr:uno", sketch.String())
+		require.Error(t, err)
+	})
+}

internal/integrationtest/compile_4/testdata/ValidSketchWithBrokenSketchFileSymlink/ValidSketchWithBrokenSketchFileSymlink.ino

@@ -0,0 +1,2 @@
+void setup() {}
+void loop() {}

internal/integrationtest/compile_4/testdata/ValidSketchWithBrokenSketchFileSymlink/other_file.ino

@@ -0,0 +1 @@
+broken