From c2b4d5cf3e6f602ebdeae75f09c92abc1781446e Mon Sep 17 00:00:00 2001 From: Martino Facchin Date: Thu, 14 Dec 2023 14:11:41 +0100 Subject: [PATCH 1/5] Reuse archiveCompiledFiles helper for long commandline shrink Since archiveCompiledFiles already handles hot cache correctly, this avoids objs.a being rebuilt even if files don't change. Would be ideal if PathList could expose a generic Filter API (to get rid of the "duplicated" filter) --- internal/arduino/builder/linker.go | 39 +++++++++++++----------------- 1 file changed, 17 insertions(+), 22 deletions(-) diff --git a/internal/arduino/builder/linker.go b/internal/arduino/builder/linker.go index 55534503cd4..20580a5e1db 100644 --- a/internal/arduino/builder/linker.go +++ b/internal/arduino/builder/linker.go @@ -22,6 +22,16 @@ import ( "github.com/arduino/go-paths-helper" ) +func filter(p *paths.PathList, filter func(*paths.Path) bool) paths.PathList { + res := (*p)[:0] + for _, path := range *p { + if filter(path) { + res = append(res, path) + } + } + return res +} + // link fixdoc func (b *Builder) link() error { if b.onlyUpdateCompilationDatabase { @@ -53,31 +63,16 @@ func (b *Builder) link() error { // it may happen that a subdir/spi.o inside the archive may be overwritten by a anotherdir/spi.o // because thery are both named spi.o. - properties := b.buildProperties.Clone() archives := paths.NewPathList() + for _, object := range objectFiles { - if object.HasSuffix(".a") { - archives.Add(object) - continue - } archive := object.Parent().Join("objs.a") - if !archives.Contains(archive) { - archives.Add(archive) - // Cleanup old archives - _ = archive.Remove() - } - properties.Set("archive_file", archive.Base()) - properties.SetPath("archive_file_path", archive) - properties.SetPath("object_file", object) - - command, err := b.prepareCommandForRecipe(properties, "recipe.ar.pattern", false) - if err != nil { - return err - } - - if err := b.execCommand(command); err != nil { - return err - } + archives.AddIfMissing(archive) + } + + for _, archive := range archives { + relatedObjectFiles := filter(&objectFiles, func(object *paths.Path) bool { return object.Parent().EquivalentTo(archive.Parent()) }) + b.archiveCompiledFiles(archive.Parent(), paths.New(archive.Base()), relatedObjectFiles) } objectFileList = strings.Join(f.Map(archives.AsStrings(), wrapWithDoubleQuotes), " ") From 7af319719cd6a42599bda8a1a45c134719e6b09b Mon Sep 17 00:00:00 2001 From: Cristian Maglie Date: Tue, 19 Dec 2023 18:24:52 +0100 Subject: [PATCH 2/5] Upgrade go-paths / remove duplicate filter function --- .../arduino/go-paths-helper.dep.yml | 2 +- go.mod | 2 +- go.sum | 4 ++-- internal/arduino/builder/linker.go | 19 +++++++------------ 4 files changed, 11 insertions(+), 16 deletions(-) diff --git a/.licenses/go/github.com/arduino/go-paths-helper.dep.yml b/.licenses/go/github.com/arduino/go-paths-helper.dep.yml index f06e4ce1625..c821a1e1141 100644 --- a/.licenses/go/github.com/arduino/go-paths-helper.dep.yml +++ b/.licenses/go/github.com/arduino/go-paths-helper.dep.yml @@ -1,6 +1,6 @@ --- name: github.com/arduino/go-paths-helper -version: v1.10.1 +version: v1.11.0 type: go summary: homepage: https://pkg.go.dev/github.com/arduino/go-paths-helper diff --git a/go.mod b/go.mod index d9120143dfb..600766e7664 100644 --- a/go.mod +++ b/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.10.1 + github.com/arduino/go-paths-helper v1.11.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 diff --git a/go.sum b/go.sum index a8610c150b6..080ac0c22df 100644 --- a/go.sum +++ b/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.10.1 h1:j8InnhLrSeoPiOvTnZL0XMFt7l407ciTBJJJs7W9bs4= -github.com/arduino/go-paths-helper v1.10.1/go.mod h1:jcpW4wr0u69GlXhTYydsdsqAjLaYK5n7oWHfKqOG6LM= +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-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= diff --git a/internal/arduino/builder/linker.go b/internal/arduino/builder/linker.go index 20580a5e1db..0af2e9b3e53 100644 --- a/internal/arduino/builder/linker.go +++ b/internal/arduino/builder/linker.go @@ -22,16 +22,6 @@ import ( "github.com/arduino/go-paths-helper" ) -func filter(p *paths.PathList, filter func(*paths.Path) bool) paths.PathList { - res := (*p)[:0] - for _, path := range *p { - if filter(path) { - res = append(res, path) - } - } - return res -} - // link fixdoc func (b *Builder) link() error { if b.onlyUpdateCompilationDatabase { @@ -64,14 +54,19 @@ func (b *Builder) link() error { // because thery are both named spi.o. archives := paths.NewPathList() - for _, object := range objectFiles { archive := object.Parent().Join("objs.a") archives.AddIfMissing(archive) } + // Generate archive for each directory for _, archive := range archives { - relatedObjectFiles := filter(&objectFiles, func(object *paths.Path) bool { return object.Parent().EquivalentTo(archive.Parent()) }) + archiveDir := archive.Parent() + relatedObjectFiles := objectFiles.Clone() + relatedObjectFiles.Filter(func(object *paths.Path) bool { + // extract all the object files that are in the same directory of the archive + return object.Parent().EquivalentTo(archiveDir) + }) b.archiveCompiledFiles(archive.Parent(), paths.New(archive.Base()), relatedObjectFiles) } From 06e3e7f1c65f9f306a58f2113eb23715cfc5ec11 Mon Sep 17 00:00:00 2001 From: Cristian Maglie Date: Tue, 19 Dec 2023 18:49:58 +0100 Subject: [PATCH 3/5] Consider existing archives during the build --- internal/arduino/builder/linker.go | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/internal/arduino/builder/linker.go b/internal/arduino/builder/linker.go index 0af2e9b3e53..b7c407b4fd7 100644 --- a/internal/arduino/builder/linker.go +++ b/internal/arduino/builder/linker.go @@ -53,14 +53,18 @@ func (b *Builder) link() error { // it may happen that a subdir/spi.o inside the archive may be overwritten by a anotherdir/spi.o // because thery are both named spi.o. - archives := paths.NewPathList() + // Put all the existing archives apart from the other object files + existingArchives := objectFiles.Clone() + existingArchives.FilterSuffix(".a") + objectFiles.FilterOutSuffix(".a") + + // Generate an archive for each directory from the remaining object files + newArchives := paths.NewPathList() for _, object := range objectFiles { archive := object.Parent().Join("objs.a") - archives.AddIfMissing(archive) + newArchives.AddIfMissing(archive) } - - // Generate archive for each directory - for _, archive := range archives { + for _, archive := range newArchives { archiveDir := archive.Parent() relatedObjectFiles := objectFiles.Clone() relatedObjectFiles.Filter(func(object *paths.Path) bool { @@ -70,7 +74,10 @@ func (b *Builder) link() error { b.archiveCompiledFiles(archive.Parent(), paths.New(archive.Base()), relatedObjectFiles) } - objectFileList = strings.Join(f.Map(archives.AsStrings(), wrapWithDoubleQuotes), " ") + // Put everything together + allArchives := existingArchives.Clone() + allArchives.AddAll(newArchives) + objectFileList = strings.Join(f.Map(allArchives.AsStrings(), wrapWithDoubleQuotes), " ") objectFileList = "-Wl,--whole-archive " + objectFileList + " -Wl,--no-whole-archive" } From 916e795d5ca7db551d2525f4b993530407144a90 Mon Sep 17 00:00:00 2001 From: Cristian Maglie Date: Tue, 19 Dec 2023 21:04:02 +0100 Subject: [PATCH 4/5] Simplified archiveCompiledFiles function signature It doesn't make sense anymore to keep path and filename separated. --- internal/arduino/builder/archive_compiled_files.go | 4 +--- internal/arduino/builder/core.go | 2 +- internal/arduino/builder/libraries.go | 2 +- internal/arduino/builder/linker.go | 2 +- 4 files changed, 4 insertions(+), 6 deletions(-) diff --git a/internal/arduino/builder/archive_compiled_files.go b/internal/arduino/builder/archive_compiled_files.go index 31caed6df41..dc43d82ac87 100644 --- a/internal/arduino/builder/archive_compiled_files.go +++ b/internal/arduino/builder/archive_compiled_files.go @@ -20,9 +20,7 @@ import ( ) // ArchiveCompiledFiles fixdoc -func (b *Builder) archiveCompiledFiles(buildPath *paths.Path, archiveFile *paths.Path, objectFilesToArchive paths.PathList) (*paths.Path, error) { - archiveFilePath := buildPath.JoinPath(archiveFile) - +func (b *Builder) archiveCompiledFiles(archiveFilePath *paths.Path, objectFilesToArchive paths.PathList) (*paths.Path, error) { if b.onlyUpdateCompilationDatabase { if b.logger.Verbose() { b.logger.Info(tr("Skipping archive creation of: %[1]s", archiveFilePath)) diff --git a/internal/arduino/builder/core.go b/internal/arduino/builder/core.go index 102e1be2eac..78bedc18d5f 100644 --- a/internal/arduino/builder/core.go +++ b/internal/arduino/builder/core.go @@ -128,7 +128,7 @@ func (b *Builder) compileCore() (*paths.Path, paths.PathList, error) { return nil, nil, err } - archiveFile, err := b.archiveCompiledFiles(b.coreBuildPath, paths.New("core.a"), coreObjectFiles) + archiveFile, err := b.archiveCompiledFiles(b.coreBuildPath.Join("core.a"), coreObjectFiles) if err != nil { return nil, nil, err } diff --git a/internal/arduino/builder/libraries.go b/internal/arduino/builder/libraries.go index d0a32bf0806..e1df2fd64cd 100644 --- a/internal/arduino/builder/libraries.go +++ b/internal/arduino/builder/libraries.go @@ -196,7 +196,7 @@ func (b *Builder) compileLibrary(library *libraries.Library, includes []string) return nil, err } if library.DotALinkage { - archiveFile, err := b.archiveCompiledFiles(libraryBuildPath, paths.New(library.DirName+".a"), libObjectFiles) + archiveFile, err := b.archiveCompiledFiles(libraryBuildPath.Join(library.DirName+".a"), libObjectFiles) if err != nil { return nil, err } diff --git a/internal/arduino/builder/linker.go b/internal/arduino/builder/linker.go index b7c407b4fd7..c991778336d 100644 --- a/internal/arduino/builder/linker.go +++ b/internal/arduino/builder/linker.go @@ -71,7 +71,7 @@ func (b *Builder) link() error { // extract all the object files that are in the same directory of the archive return object.Parent().EquivalentTo(archiveDir) }) - b.archiveCompiledFiles(archive.Parent(), paths.New(archive.Base()), relatedObjectFiles) + b.archiveCompiledFiles(archive, relatedObjectFiles) } // Put everything together From 373e8711125f914b69e45e2d0348ab427e3a8656 Mon Sep 17 00:00:00 2001 From: Cristian Maglie Date: Wed, 20 Dec 2023 10:46:23 +0100 Subject: [PATCH 5/5] Added integration test --- internal/integrationtest/compile_1/compile_test.go | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/internal/integrationtest/compile_1/compile_test.go b/internal/integrationtest/compile_1/compile_test.go index 40add0dc391..2808d14f5e0 100644 --- a/internal/integrationtest/compile_1/compile_test.go +++ b/internal/integrationtest/compile_1/compile_test.go @@ -21,6 +21,7 @@ import ( "encoding/json" "fmt" "os" + "regexp" "sort" "strings" "testing" @@ -830,8 +831,17 @@ func TestCompileWithArchivesAndLongPaths(t *testing.T) { sketchPath := paths.New(libOutput) sketchPath = sketchPath.Join("examples", "ArduinoIoTCloud-Advanced") - _, _, err = cli.Run("compile", "-b", "esp8266:esp8266:huzzah", sketchPath.String(), "--config-file", "arduino-cli.yaml") - require.NoError(t, err) + t.Run("Compile", func(t *testing.T) { + _, _, err = cli.Run("compile", "-b", "esp8266:esp8266:huzzah", sketchPath.String(), "--config-file", "arduino-cli.yaml") + require.NoError(t, err) + }) + + t.Run("CheckCachingOfFolderArchives", func(t *testing.T) { + // Run compile again and check if the archive is re-used (cached) + out, _, err := cli.Run("compile", "-b", "esp8266:esp8266:huzzah", sketchPath.String(), "--config-file", "arduino-cli.yaml", "-v") + require.NoError(t, err) + require.True(t, regexp.MustCompile(`(?m)^Using previously compiled file:.*libraries.ArduinoIoTCloud.objs\.a$`).Match(out)) + }) } func TestCompileWithPrecompileLibrary(t *testing.T) {