gRPC: `Compile` will now fail if a platform core has been modified. (#2551)

* Slighlty moved variables near their utilization place

* Added facility to keep timestamps of used files

* Compile will fail if platforms are changed

* Added integration test

* Make linter happy

* Fixed grammar

* Test all monitored files

commands/cmderrors/cmderrors.go

@@ -869,3 +869,19 @@ func (e *MultipleLibraryInstallDetected) Error() string {
 func (e *MultipleLibraryInstallDetected) ToRPCStatus() *status.Status {
 	return status.New(codes.InvalidArgument, e.Error())
 }
+
+// InstanceNeedsReinitialization
+type InstanceNeedsReinitialization struct {
+}
+
+func (e *InstanceNeedsReinitialization) Error() string {
+	return tr("The instance is no longer valid and needs to be reinitialized")
+}
+
+// ToRPCStatus converts the error into a *status.Status
+func (e *InstanceNeedsReinitialization) ToRPCStatus() *status.Status {
+	st, _ := status.
+		New(codes.InvalidArgument, e.Error()).
+		WithDetails(&rpc.InstanceNeedsReinitializationError{})
+	return st
+}

commands/compile/compile.go

@@ -63,6 +63,10 @@ func Compile(ctx context.Context, req *rpc.CompileRequest, outStream, errStream
 	}
 	defer release()
 
+	if pme.Dirty() {
+		return nil, &cmderrors.InstanceNeedsReinitialization{}
+	}
+
 	lm, err := instances.GetLibraryManager(req.GetInstance())
 	if err != nil {
 		return nil, err

internal/arduino/cores/cores.go

@@ -24,6 +24,7 @@ import (
 	"path/filepath"
 	"sort"
 	"strings"
+	"time"
 
 	"github.com/arduino/arduino-cli/internal/arduino/globals"
 	"github.com/arduino/arduino-cli/internal/arduino/resources"
@@ -70,6 +71,7 @@ type PlatformRelease struct {
 	Programmers             map[string]*Programmer        `json:"-"`
 	Menus                   *properties.Map               `json:"-"`
 	InstallDir              *paths.Path                   `json:"-"`
+	Timestamps              *TimestampsStore              // Contains the timestamps of the files used to build this PlatformRelease
 	IsTrusted               bool                          `json:"-"`
 	PluggableDiscoveryAware bool                          `json:"-"` // true if the Platform supports pluggable discovery (no compatibility layer required)
 	Monitors                map[string]*MonitorDependency `json:"-"`
@@ -77,6 +79,51 @@ type PlatformRelease struct {
 	Compatible              bool                          `json:"-"` // true if at all ToolDependencies are available for the current OS/ARCH.
 }
 
+// TimestampsStore is a generic structure to store timestamps
+type TimestampsStore struct {
+	timestamps map[*paths.Path]*time.Time
+}
+
+// NewTimestampsStore creates a new TimestampsStore
+func NewTimestampsStore() *TimestampsStore {
+	return &TimestampsStore{
+		timestamps: map[*paths.Path]*time.Time{},
+	}
+}
+
+// AddFile adds a file to the TimestampsStore
+func (t *TimestampsStore) AddFile(path *paths.Path) {
+	if info, err := path.Stat(); err != nil {
+		t.timestamps[path] = nil // Save a missing file with a nil timestamp
+	} else {
+		modtime := info.ModTime()
+		t.timestamps[path] = &modtime
+	}
+}
+
+// Dirty returns true if one of the files stored in the TimestampsStore has been
+// changed after being added to the store.
+func (t *TimestampsStore) Dirty() bool {
+	for path, timestamp := range t.timestamps {
+		if info, err := path.Stat(); err != nil {
+			if timestamp != nil {
+				return true
+			}
+		} else {
+			if timestamp == nil || info.ModTime() != *timestamp {
+				return true
+			}
+		}
+	}
+	return false
+}
+
+// Dirty returns true if one of the files of this PlatformRelease has been changed
+// (it means that the PlatformRelease should be rebuilt to be used correctly).
+func (release *PlatformRelease) Dirty() bool {
+	return release.Timestamps.Dirty()
+}
+
 // BoardManifest contains information about a board. These metadata are usually
 // provided by the package_index.json
 type BoardManifest struct {
@@ -207,6 +254,7 @@ func (platform *Platform) GetOrCreateRelease(version *semver.Version) *PlatformR
 		Properties:  properties.NewMap(),
 		Programmers: map[string]*Programmer{},
 		Platform:    platform,
+		Timestamps:  NewTimestampsStore(),
 	}
 	platform.Releases[tag] = release
 	return release

internal/arduino/cores/packagemanager/loader.go

@@ -239,29 +239,32 @@ func (pm *Builder) loadPlatform(targetPackage *cores.Package, architecture strin
 func (pm *Builder) loadPlatformRelease(platform *cores.PlatformRelease, path *paths.Path) error {
 	platform.InstallDir = path
 
-	// Some useful paths
-	installedJSONPath := path.Join("installed.json")
-	platformTxtPath := path.Join("platform.txt")
-	platformTxtLocalPath := path.Join("platform.local.txt")
-	programmersTxtPath := path.Join("programmers.txt")
-
 	// If the installed.json file is found load it, this is done to handle the
 	// case in which the platform's index and its url have been deleted locally,
 	// if we don't load it some information about the platform is lost
+	installedJSONPath := path.Join("installed.json")
+	platform.Timestamps.AddFile(installedJSONPath)
 	if installedJSONPath.Exist() {
 		if _, err := pm.LoadPackageIndexFromFile(installedJSONPath); err != nil {
 			return fmt.Errorf(tr("loading %[1]s: %[2]s"), installedJSONPath, err)
 		}
 	}
 
+	// TODO: why CLONE?
+	platform.Properties = platform.Properties.Clone()
+
 	// Create platform properties
-	platform.Properties = platform.Properties.Clone() // TODO: why CLONE?
-	if p, err := properties.SafeLoad(platformTxtPath.String()); err == nil {
+	platformTxtPath := path.Join("platform.txt")
+	platform.Timestamps.AddFile(platformTxtPath)
+	if p, err := properties.SafeLoadFromPath(platformTxtPath); err == nil {
 		platform.Properties.Merge(p)
 	} else {
 		return fmt.Errorf(tr("loading %[1]s: %[2]s"), platformTxtPath, err)
 	}
-	if p, err := properties.SafeLoad(platformTxtLocalPath.String()); err == nil {
+
+	platformTxtLocalPath := path.Join("platform.local.txt")
+	platform.Timestamps.AddFile(platformTxtLocalPath)
+	if p, err := properties.SafeLoadFromPath(platformTxtLocalPath); err == nil {
 		platform.Properties.Merge(p)
 	} else {
 		return fmt.Errorf(tr("loading %[1]s: %[2]s"), platformTxtLocalPath, err)
@@ -287,7 +290,9 @@ func (pm *Builder) loadPlatformRelease(platform *cores.PlatformRelease, path *pa
 	}
 
 	// Create programmers properties
-	if programmersProperties, err := properties.SafeLoad(programmersTxtPath.String()); err == nil {
+	programmersTxtPath := path.Join("programmers.txt")
+	platform.Timestamps.AddFile(programmersTxtPath)
+	if programmersProperties, err := properties.SafeLoadFromPath(programmersTxtPath); err == nil {
 		for programmerID, programmerProps := range programmersProperties.FirstLevelOf() {
 			if !platform.PluggableDiscoveryAware {
 				convertUploadToolsToPluggableDiscovery(programmerProps)
@@ -410,12 +415,14 @@ func (pm *Builder) loadBoards(platform *cores.PlatformRelease) error {
 	}
 
 	boardsTxtPath := platform.InstallDir.Join("boards.txt")
+	platform.Timestamps.AddFile(boardsTxtPath)
 	allBoardsProperties, err := properties.LoadFromPath(boardsTxtPath)
 	if err != nil {
 		return err
 	}
 
 	boardsLocalTxtPath := platform.InstallDir.Join("boards.local.txt")
+	platform.Timestamps.AddFile(boardsLocalTxtPath)
 	if boardsLocalProperties, err := properties.SafeLoadFromPath(boardsLocalTxtPath); err == nil {
 		allBoardsProperties.Merge(boardsLocalProperties)
 	} else {

internal/arduino/cores/packagemanager/package_manager.go

@@ -22,6 +22,7 @@ import (
 	"os"
 	"path"
 	"path/filepath"
+	"slices"
 	"strconv"
 	"strings"
 	"sync"
@@ -898,3 +899,11 @@ func (pme *Explorer) NormalizeFQBN(fqbn *cores.FQBN) (*cores.FQBN, error) {
 	}
 	return normalizedFqbn, nil
 }
+
+// Dirty returns true if one of the loaded platforms needs to be re-initialized
+// due to file changes in the platform releases.
+func (pme *Explorer) Dirty() bool {
+	return slices.ContainsFunc(
+		pme.InstalledPlatformReleases(),
+		(*cores.PlatformRelease).Dirty)
+}

internal/integrationtest/daemon/detect_core_changes_test.go

+// This file is part of arduino-cli.
+//
+// Copyright 2022 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 license@arduino.cc.
+
+package daemon_test
+
+import (
+	"context"
+	"errors"
+	"fmt"
+	"io"
+	"testing"
+	"time"
+
+	"github.com/arduino/arduino-cli/internal/integrationtest"
+	"github.com/arduino/arduino-cli/rpc/cc/arduino/cli/commands/v1"
+	"github.com/arduino/go-paths-helper"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestDetectionOfChangesInCoreBeforeCompile(t *testing.T) {
+	// See: https://github.com/arduino/arduino-cli/issues/2523
+
+	env, cli := integrationtest.CreateEnvForDaemon(t)
+	defer env.CleanUp()
+
+	// Create a new instance of the daemon
+	grpcInst := cli.Create()
+	require.NoError(t, grpcInst.Init("", "", func(ir *commands.InitResponse) {
+		fmt.Printf("INIT> %v\n", ir.GetMessage())
+	}))
+
+	// Install avr core
+	installCl, err := grpcInst.PlatformInstall(context.Background(), "arduino", "avr", "", true)
+	require.NoError(t, err)
+	for {
+		installResp, err := installCl.Recv()
+		if errors.Is(err, io.EOF) {
+			break
+		}
+		require.NoError(t, err)
+		fmt.Printf("INSTALL> %v\n", installResp)
+	}
+	installCl.CloseSend()
+
+	// Utility functions: tryCompile
+	sketchPath, err := paths.New("testdata", "bare_minimum").Abs()
+	require.NoError(t, err)
+	tryCompile := func() error {
+		compileCl, err := grpcInst.Compile(context.Background(), "arduino:avr:uno", sketchPath.String(), "")
+		require.NoError(t, err)
+		defer compileCl.CloseSend()
+		for {
+			if compileResp, err := compileCl.Recv(); errors.Is(err, io.EOF) {
+				return nil
+			} else if err != nil {
+				return err
+			} else {
+				fmt.Printf("COMPILE> %v\n", compileResp)
+			}
+		}
+	}
+
+	// Utility functions: tryTouch will touch a file and see if the compile detects the change
+	tryTouch := func(fileToTouch *paths.Path) {
+		time.Sleep(time.Second) // await at least one second so the timestamp of the file is different
+
+		// touch the file
+		f, err := fileToTouch.Append()
+		require.NoError(t, err)
+		_, err = f.WriteString("\n")
+		require.NoError(t, err)
+		require.NoError(t, f.Close())
+
+		// try compile: should fail
+		err = tryCompile()
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "The instance is no longer valid and needs to be reinitialized")
+
+		// re-init instance
+		require.NoError(t, grpcInst.Init("", "", func(ir *commands.InitResponse) {
+			fmt.Printf("INIT> %v\n", ir.GetMessage())
+		}))
+
+		// try compile: should succeed
+		require.NoError(t, tryCompile())
+	}
+
+	avrCorePath := cli.DataDir().Join("packages", "arduino", "hardware", "avr", "1.8.6")
+	tryTouch(avrCorePath.Join("installed.json"))
+	tryTouch(avrCorePath.Join("platform.txt"))
+	tryTouch(avrCorePath.Join("platform.local.txt"))
+	tryTouch(avrCorePath.Join("programmers.txt"))
+	tryTouch(avrCorePath.Join("boards.txt"))
+	tryTouch(avrCorePath.Join("boards.local.txt"))
+
+	// Delete a file and check if the change is detected
+	require.NoError(t, avrCorePath.Join("programmers.txt").Remove())
+	err = tryCompile()
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "The instance is no longer valid and needs to be reinitialized")
+
+	// Re-init instance and check again
+	require.NoError(t, grpcInst.Init("", "", func(ir *commands.InitResponse) {
+		fmt.Printf("INIT> %v\n", ir.GetMessage())
+	}))
+	require.NoError(t, tryCompile())
+
+	// Create a file and check if the change is detected
+	{
+		f, err := avrCorePath.Join("programmers.txt").Create()
+		require.NoError(t, err)
+		require.NoError(t, f.Close())
+	}
+	err = tryCompile()
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "The instance is no longer valid and needs to be reinitialized")
+
+	// Re-init instance and check again
+	require.NoError(t, grpcInst.Init("", "", func(ir *commands.InitResponse) {
+		fmt.Printf("INIT> %v\n", ir.GetMessage())
+	}))
+	require.NoError(t, tryCompile())
+}

rpc/cc/arduino/cli/commands/v1/compile.proto

@@ -109,6 +109,8 @@ message CompileResponse {
   }
 }
 
+message InstanceNeedsReinitializationError {}
+
 message BuilderResult {
   // The compiler build path
   string build_path = 1;