Fixed build_cache.path behaviour / The --build-path dir now produ…

…ce a full build (#2673) * If a build path is specified ignore all build caches The complete build will be performed in the specified build path. The build artifacts in the build path will be reused for the next build. * Fixed arguments.CheckFlagsConflicts helper Previously it would reject only if ALL the arguments in the given set are used. Now it rejects if AT LEAST TWO arguments of the given set are used. * Added --build-path as alias for --input-dir in upload and debug commands * Created configuration defaults for build_cache.path setting * The build_cache.path setting now affect also the sketches cache * Deprecated --build-cache-path option in compile * Use default user's cache dir instead of tmp for build cache * Add notes in UPGRADING.md * Updated integration test * Updated integration test * Updated integration test * Updated integration test * Updated integration test
arduino · Sep 18, 2024 · 863c1ec · 863c1ec
1 parent 23d5036
commit 863c1ec
Show file tree

Hide file tree

Showing 25 changed files with 385 additions and 271 deletions.
diff --git a/commands/service_compile.go b/commands/service_compile.go
@@ -160,7 +160,7 @@ func (s *arduinoCoreServerImpl) Compile(req *rpc.CompileRequest, stream rpc.Ardu
 		return errors.New(i18n.Tr("Firmware encryption/signing requires all the following properties to be defined: %s", "build.keys.keychain, build.keys.sign_key, build.keys.encrypt_key"))
 	}
 
-	// Generate or retrieve build path
+	// Retrieve build path from user arguments
 	var buildPath *paths.Path
 	if buildPathArg := req.GetBuildPath(); buildPathArg != "" {
 		buildPath = paths.New(req.GetBuildPath()).Canonical()
@@ -170,44 +170,46 @@ func (s *arduinoCoreServerImpl) Compile(req *rpc.CompileRequest, stream rpc.Ardu
 			}
 		}
 	}
-	if buildPath == nil {
-		buildPath = sk.DefaultBuildPath()
-	}
-	if err = buildPath.MkdirAll(); err != nil {
-		return &cmderrors.PermissionDeniedError{Message: i18n.Tr("Cannot create build directory"), Cause: err}
-	}
-	buildcache.New(buildPath.Parent()).GetOrCreate(buildPath.Base())
-	// cache is purged after compilation to not remove entries that might be required
-
-	defer maybePurgeBuildCache(
-		s.settings.GetCompilationsBeforeBuildCachePurge(),
-		s.settings.GetBuildCacheTTL().Abs())
 
-	var buildCachePath *paths.Path
-	if req.GetBuildCachePath() != "" {
-		p, err := paths.New(req.GetBuildCachePath()).Abs()
-		if err != nil {
+	// If no build path has been set by the user:
+	// - set up the build cache directory
+	// - set the sketch build path inside the build cache directory.
+	var coreBuildCachePath *paths.Path
+	var extraCoreBuildCachePaths paths.PathList
+	if buildPath == nil {
+		var buildCachePath *paths.Path
+		if p := req.GetBuildCachePath(); p != "" { //nolint:staticcheck
+			buildCachePath = paths.New(p)
+		} else {
+			buildCachePath = s.settings.GetBuildCachePath()
+		}
+		if err := buildCachePath.ToAbs(); err != nil {
 			return &cmderrors.PermissionDeniedError{Message: i18n.Tr("Cannot create build cache directory"), Cause: err}
 		}
-		buildCachePath = p
-	} else if p, ok := s.settings.GetBuildCachePath(); ok {
-		buildCachePath = p
-	} else {
-		buildCachePath = paths.TempDir().Join("arduino")
-	}
-	if err := buildCachePath.MkdirAll(); err != nil {
-		return &cmderrors.PermissionDeniedError{Message: i18n.Tr("Cannot create build cache directory"), Cause: err}
-	}
-	coreBuildCachePath := buildCachePath.Join("cores")
+		if err := buildCachePath.MkdirAll(); err != nil {
+			return &cmderrors.PermissionDeniedError{Message: i18n.Tr("Cannot create build cache directory"), Cause: err}
+		}
+		coreBuildCachePath = buildCachePath.Join("cores")
 
-	var extraCoreBuildCachePaths paths.PathList
-	if len(req.GetBuildCacheExtraPaths()) == 0 {
-		extraCoreBuildCachePaths = s.settings.GetBuildCacheExtraPaths()
-	} else {
-		extraCoreBuildCachePaths = paths.NewPathList(req.GetBuildCacheExtraPaths()...)
+		if len(req.GetBuildCacheExtraPaths()) == 0 {
+			extraCoreBuildCachePaths = s.settings.GetBuildCacheExtraPaths()
+		} else {
+			extraCoreBuildCachePaths = paths.NewPathList(req.GetBuildCacheExtraPaths()...)
+		}
+		for i, p := range extraCoreBuildCachePaths {
+			extraCoreBuildCachePaths[i] = p.Join("cores")
+		}
+
+		buildPath = s.getDefaultSketchBuildPath(sk, buildCachePath)
+		buildcache.New(buildPath.Parent()).GetOrCreate(buildPath.Base())
+
+		// cache is purged after compilation to not remove entries that might be required
+		defer maybePurgeBuildCache(
+			s.settings.GetCompilationsBeforeBuildCachePurge(),
+			s.settings.GetBuildCacheTTL().Abs())
 	}
-	for i, p := range extraCoreBuildCachePaths {
-		extraCoreBuildCachePaths[i] = p.Join("cores")
+	if err = buildPath.MkdirAll(); err != nil {
+		return &cmderrors.PermissionDeniedError{Message: i18n.Tr("Cannot create build directory"), Cause: err}
 	}
 
 	if _, err := pme.FindToolsRequiredForBuild(targetPlatform, buildPlatform); err != nil {
@@ -416,6 +418,16 @@ func (s *arduinoCoreServerImpl) Compile(req *rpc.CompileRequest, stream rpc.Ardu
 	return nil
 }
 
+// getDefaultSketchBuildPath generates the default build directory for a given sketch.
+// The sketch build path is inside the build cache path and is unique for each sketch.
+// If overriddenBuildCachePath is nil the build cache path is taken from the settings.
+func (s *arduinoCoreServerImpl) getDefaultSketchBuildPath(sk *sketch.Sketch, overriddenBuildCachePath *paths.Path) *paths.Path {
+	if overriddenBuildCachePath == nil {
+		overriddenBuildCachePath = s.settings.GetBuildCachePath()
+	}
+	return overriddenBuildCachePath.Join("sketches", sk.Hash())
+}
+
 // maybePurgeBuildCache runs the build files cache purge if the policy conditions are met.
 func maybePurgeBuildCache(compilationsBeforePurge uint, cacheTTL time.Duration) {
 	// 0 means never purge

diff --git a/commands/service_compile_test.go b/commands/service_compile_test.go
@@ -0,0 +1,32 @@
+// This file is part of arduino-cli.
+//
+// Copyright 2024 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 commands
+
+import (
+	"testing"
+
+	"github.com/arduino/arduino-cli/internal/arduino/sketch"
+	paths "github.com/arduino/go-paths-helper"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestGenBuildPath(t *testing.T) {
+	srv := NewArduinoCoreServer().(*arduinoCoreServerImpl)
+	want := srv.settings.GetBuildCachePath().Join("sketches", "ACBD18DB4CC2F85CEDEF654FCCC4A4D8")
+	act := srv.getDefaultSketchBuildPath(&sketch.Sketch{FullPath: paths.New("foo")}, nil)
+	assert.True(t, act.EquivalentTo(want))
+	assert.Equal(t, "ACBD18DB4CC2F85CEDEF654FCCC4A4D8", (&sketch.Sketch{FullPath: paths.New("foo")}).Hash())
+}
diff --git a/commands/service_debug.go b/commands/service_debug.go
@@ -214,7 +214,7 @@ func (s *arduinoCoreServerImpl) Debug(stream rpc.ArduinoCoreService_DebugServer)
 	defer release()
 
 	// Exec debugger
-	commandLine, err := getCommandLine(debugConfReq, pme)
+	commandLine, err := s.getDebugCommandLine(debugConfReq, pme)
 	if err != nil {
 		return err
 	}
@@ -254,9 +254,9 @@ func (s *arduinoCoreServerImpl) Debug(stream rpc.ArduinoCoreService_DebugServer)
 	return sendResult(&rpc.DebugResponse_Result{})
 }
 
-// getCommandLine compose a debug command represented by a core recipe
-func getCommandLine(req *rpc.GetDebugConfigRequest, pme *packagemanager.Explorer) ([]string, error) {
-	debugInfo, err := getDebugProperties(req, pme, false)
+// getDebugCommandLine compose a debug command represented by a core recipe
+func (s *arduinoCoreServerImpl) getDebugCommandLine(req *rpc.GetDebugConfigRequest, pme *packagemanager.Explorer) ([]string, error) {
+	debugInfo, err := s.getDebugProperties(req, pme, false)
 	if err != nil {
 		return nil, err
 	}

diff --git a/commands/service_debug_config.go b/commands/service_debug_config.go
@@ -45,7 +45,7 @@ func (s *arduinoCoreServerImpl) GetDebugConfig(ctx context.Context, req *rpc.Get
 		return nil, err
 	}
 	defer release()
-	return getDebugProperties(req, pme, false)
+	return s.getDebugProperties(req, pme, false)
 }
 
 // IsDebugSupported checks if the given board/programmer configuration supports debugging.
@@ -65,7 +65,7 @@ func (s *arduinoCoreServerImpl) IsDebugSupported(ctx context.Context, req *rpc.I
 		Programmer:      req.GetProgrammer(),
 		DebugProperties: req.GetDebugProperties(),
 	}
-	expectedOutput, err := getDebugProperties(configRequest, pme, true)
+	expectedOutput, err := s.getDebugProperties(configRequest, pme, true)
 	var x *cmderrors.FailedDebugError
 	if errors.As(err, &x) {
 		return &rpc.IsDebugSupportedResponse{DebuggingSupported: false}, nil
@@ -81,7 +81,7 @@ func (s *arduinoCoreServerImpl) IsDebugSupported(ctx context.Context, req *rpc.I
 		checkFQBN := minimumFQBN.Clone()
 		checkFQBN.Configs.Remove(config)
 		configRequest.Fqbn = checkFQBN.String()
-		checkOutput, err := getDebugProperties(configRequest, pme, true)
+		checkOutput, err := s.getDebugProperties(configRequest, pme, true)
 		if err == nil && reflect.DeepEqual(expectedOutput, checkOutput) {
 			minimumFQBN.Configs.Remove(config)
 		}
@@ -92,7 +92,7 @@ func (s *arduinoCoreServerImpl) IsDebugSupported(ctx context.Context, req *rpc.I
 	}, nil
 }
 
-func getDebugProperties(req *rpc.GetDebugConfigRequest, pme *packagemanager.Explorer, skipSketchChecks bool) (*rpc.GetDebugConfigResponse, error) {
+func (s *arduinoCoreServerImpl) getDebugProperties(req *rpc.GetDebugConfigRequest, pme *packagemanager.Explorer, skipSketchChecks bool) (*rpc.GetDebugConfigResponse, error) {
 	var (
 		sketchName             string
 		sketchDefaultFQBN      string
@@ -111,7 +111,7 @@ func getDebugProperties(req *rpc.GetDebugConfigRequest, pme *packagemanager.Expl
 		}
 		sketchName = sk.Name
 		sketchDefaultFQBN = sk.GetDefaultFQBN()
-		sketchDefaultBuildPath = sk.DefaultBuildPath()
+		sketchDefaultBuildPath = s.getDefaultSketchBuildPath(sk, nil)
 	} else {
 		// Use placeholder sketch data
 		sketchName = "Sketch"

diff --git a/commands/service_debug_test.go b/commands/service_debug_test.go
@@ -66,15 +66,16 @@ func TestGetCommandLine(t *testing.T) {
 	pme, release := pm.NewExplorer()
 	defer release()
 
+	srv := NewArduinoCoreServer().(*arduinoCoreServerImpl)
 	{
 		// Check programmer not found
 		req.Programmer = "not-existent"
-		_, err := getCommandLine(req, pme)
+		_, err := srv.getDebugCommandLine(req, pme)
 		require.Error(t, err)
 	}
 
 	req.Programmer = "edbg"
-	command, err := getCommandLine(req, pme)
+	command, err := srv.getDebugCommandLine(req, pme)
 	require.Nil(t, err)
 	commandToTest := strings.Join(command, " ")
 	require.Equal(t, filepath.FromSlash(goldCommand), filepath.FromSlash(commandToTest))
@@ -97,7 +98,7 @@ func TestGetCommandLine(t *testing.T) {
 		fmt.Sprintf(" --file \"%s/arduino-test/samd/variants/mkr1000/openocd_scripts/arduino_zero.cfg\"", customHardware) +
 		fmt.Sprintf(" -c \"gdb_port pipe\" -c \"telnet_port 0\" %s/build/arduino-test.samd.mkr1000/hello.ino.elf", sketchPath)
 
-	command2, err := getCommandLine(req2, pme)
+	command2, err := srv.getDebugCommandLine(req2, pme)
 	assert.Nil(t, err)
 	commandToTest2 := strings.Join(command2, " ")
 	assert.Equal(t, filepath.FromSlash(goldCommand2), filepath.FromSlash(commandToTest2))

diff --git a/commands/service_upload.go b/commands/service_upload.go
@@ -189,7 +189,7 @@ func (s *arduinoCoreServerImpl) Upload(req *rpc.UploadRequest, stream rpc.Arduin
 		})
 	})
 	defer errStream.Close()
-	updatedPort, err := runProgramAction(
+	updatedPort, err := s.runProgramAction(
 		stream.Context(),
 		pme,
 		sk,
@@ -262,7 +262,7 @@ func (s *arduinoCoreServerImpl) UploadUsingProgrammer(req *rpc.UploadUsingProgra
 	}, streamAdapter)
 }
 
-func runProgramAction(ctx context.Context, pme *packagemanager.Explorer,
+func (s *arduinoCoreServerImpl) runProgramAction(ctx context.Context, pme *packagemanager.Explorer,
 	sk *sketch.Sketch,
 	importFile, importDir, fqbnIn string, userPort *rpc.Port,
 	programmerID string,
@@ -443,7 +443,7 @@ func runProgramAction(ctx context.Context, pme *packagemanager.Explorer,
 	}
 
 	if !burnBootloader {
-		importPath, sketchName, err := determineBuildPathAndSketchName(importFile, importDir, sk)
+		importPath, sketchName, err := s.determineBuildPathAndSketchName(importFile, importDir, sk)
 		if err != nil {
 			return nil, &cmderrors.NotFoundError{Message: i18n.Tr("Error finding build artifacts"), Cause: err}
 		}
@@ -746,7 +746,7 @@ func runTool(recipeID string, props *properties.Map, outStream, errStream io.Wri
 	return nil
 }
 
-func determineBuildPathAndSketchName(importFile, importDir string, sk *sketch.Sketch) (*paths.Path, string, error) {
+func (s *arduinoCoreServerImpl) determineBuildPathAndSketchName(importFile, importDir string, sk *sketch.Sketch) (*paths.Path, string, error) {
 	// In general, compiling a sketch will produce a set of files that are
 	// named as the sketch but have different extensions, for example Sketch.ino
 	// may produce: Sketch.ino.bin; Sketch.ino.hex; Sketch.ino.zip; etc...
@@ -799,7 +799,7 @@ func determineBuildPathAndSketchName(importFile, importDir string, sk *sketch.Sk
 
 	// Case 4: only sketch specified. In this case we use the generated build path
 	// and the given sketch name.
-	return sk.DefaultBuildPath(), sk.Name + sk.MainFile.Ext(), nil
+	return s.getDefaultSketchBuildPath(sk, nil), sk.Name + sk.MainFile.Ext(), nil
 }
 
 func detectSketchNameFromBuildPath(buildPath *paths.Path) (string, error) {

diff --git a/commands/service_upload_burnbootloader.go b/commands/service_upload_burnbootloader.go
@@ -72,7 +72,7 @@ func (s *arduinoCoreServerImpl) BurnBootloader(req *rpc.BurnBootloaderRequest, s
 	}
 	defer release()
 
-	if _, err := runProgramAction(
+	if _, err := s.runProgramAction(
 		stream.Context(),
 		pme,
 		nil, // sketch

diff --git a/commands/service_upload_test.go b/commands/service_upload_test.go
@@ -71,6 +71,8 @@ func TestDetermineBuildPathAndSketchName(t *testing.T) {
 	fqbn, err := cores.ParseFQBN("arduino:samd:mkr1000")
 	require.NoError(t, err)
 
+	srv := NewArduinoCoreServer().(*arduinoCoreServerImpl)
+
 	tests := []test{
 		// 00: error: no data passed in
 		{"", "", nil, nil, "<nil>", ""},
@@ -81,7 +83,7 @@ func TestDetermineBuildPathAndSketchName(t *testing.T) {
 		// 03: error: used both importPath and importFile
 		{"testdata/upload/build_path_2/Blink.ino.hex", "testdata/upload/build_path_2", nil, nil, "<nil>", ""},
 		// 04: only sketch without FQBN
-		{"", "", blonk, nil, blonk.DefaultBuildPath().String(), "Blonk.ino"},
+		{"", "", blonk, nil, srv.getDefaultSketchBuildPath(blonk, nil).String(), "Blonk.ino"},
 		// 05: use importFile to detect build.path and project_name, sketch is ignored.
 		{"testdata/upload/build_path_2/Blink.ino.hex", "", blonk, nil, "testdata/upload/build_path_2", "Blink.ino"},
 		// 06: use importPath as build.path and Blink as project name, ignore the sketch Blonk
@@ -97,7 +99,7 @@ func TestDetermineBuildPathAndSketchName(t *testing.T) {
 		// 11: error: used both importPath and importFile
 		{"testdata/upload/build_path_2/Blink.ino.hex", "testdata/upload/build_path_2", nil, fqbn, "<nil>", ""},
 		// 12: use sketch to determine project name and sketch+fqbn to determine build path
-		{"", "", blonk, fqbn, blonk.DefaultBuildPath().String(), "Blonk.ino"},
+		{"", "", blonk, fqbn, srv.getDefaultSketchBuildPath(blonk, nil).String(), "Blonk.ino"},
 		// 13: use importFile to detect build.path and project_name, sketch+fqbn is ignored.
 		{"testdata/upload/build_path_2/Blink.ino.hex", "", blonk, fqbn, "testdata/upload/build_path_2", "Blink.ino"},
 		// 14: use importPath as build.path and Blink as project name, ignore the sketch Blonk, ignore fqbn
@@ -111,7 +113,7 @@ func TestDetermineBuildPathAndSketchName(t *testing.T) {
 	}
 	for i, test := range tests {
 		t.Run(fmt.Sprintf("SubTest%02d", i), func(t *testing.T) {
-			buildPath, sketchName, err := determineBuildPathAndSketchName(test.importFile, test.importDir, test.sketch)
+			buildPath, sketchName, err := srv.determineBuildPathAndSketchName(test.importFile, test.importDir, test.sketch)
 			if test.resBuildPath == "<nil>" {
 				require.Error(t, err)
 				require.Nil(t, buildPath)
@@ -183,10 +185,11 @@ func TestUploadPropertiesComposition(t *testing.T) {
 	pme, release := pm.NewExplorer()
 	defer release()
 
+	srv := NewArduinoCoreServer().(*arduinoCoreServerImpl)
 	testRunner := func(t *testing.T, test test, verboseVerify bool) {
 		outStream := &bytes.Buffer{}
 		errStream := &bytes.Buffer{}
-		_, err := runProgramAction(
+		_, err := srv.runProgramAction(
 			context.Background(),
 			pme,
 			nil,                     // sketch

diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md
@@ -1,6 +1,31 @@
 # Upgrading
 
-Here you can find a list of migration guides to handle breaking changes between releases of the CLI.
+Here you can find a list of migration guides to handle breaking changes, deprecations, and bugfixes that may cause
+problems between releases of the CLI.
+
+## 1.0.4
+
+### The build cache path specified with `compile --build-cache-path` or `build_cache.path` now affects also sketches.
+
+Previously the specified build cache path only affected cores and it was ignored for sketches. This is now fixed and
+both cores and sketches are cached in the given directory.
+
+### A full build of the sketch is performed if a build path is specified in `compile --build-path ...`.
+
+Previously if a build path was specified a cached core could have been used from the global build cache path resulting
+in a partial build inside the given build path.
+
+Now if a build path is specified, the global build cache path is ignored and the full build is done in the given build
+path.
+
+#### `compile --build-cache-path` is deprecated.
+
+The change above, makes the `compile --build-cache-path` flag useless. It is kept just for backward compatibility.
+
+### The default `build_cache.path` has been moved from the temp folder to the user's cache folder.
+
+Previously the `build_cache.path` was in `$TMP/arduino`. Now it has been moved to the specific OS user's cache folder,
+for example in Linux it happens to be `$HOME/.cache/arduino`.
 
 ## 1.0.0
 

diff --git a/internal/arduino/sketch/sketch.go b/internal/arduino/sketch/sketch.go
@@ -279,12 +279,6 @@ func (e *InvalidSketchFolderNameError) Error() string {
 	return i18n.Tr("no valid sketch found in %[1]s: missing %[2]s", e.SketchFolder, e.SketchFile)
 }
 
-// DefaultBuildPath generates the default build directory for a given sketch.
-// The build path is in a temporary directory and is unique for each sketch.
-func (s *Sketch) DefaultBuildPath() *paths.Path {
-	return paths.TempDir().Join("arduino", "sketches", s.Hash())
-}
-
 // Hash generate a unique hash for the given sketch.
 func (s *Sketch) Hash() string {
 	path := s.FullPath.String()

diff --git a/internal/arduino/sketch/sketch_test.go b/internal/arduino/sketch/sketch_test.go
@@ -285,12 +285,6 @@ func TestNewSketchFolderSymlink(t *testing.T) {
 	require.True(t, sketch.RootFolderFiles.ContainsEquivalentTo(sketchPathSymlink.Join("s_file.S")))
 }
 
-func TestGenBuildPath(t *testing.T) {
-	want := paths.TempDir().Join("arduino", "sketches", "ACBD18DB4CC2F85CEDEF654FCCC4A4D8")
-	assert.True(t, (&Sketch{FullPath: paths.New("foo")}).DefaultBuildPath().EquivalentTo(want))
-	assert.Equal(t, "ACBD18DB4CC2F85CEDEF654FCCC4A4D8", (&Sketch{FullPath: paths.New("foo")}).Hash())
-}
-
 func TestNewSketchWithSymlink(t *testing.T) {
 	sketchPath, _ := paths.New("testdata", "SketchWithSymlink").Abs()
 	mainFilePath := sketchPath.Join("SketchWithSymlink.ino")