Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip archive (.a) creation if the archive is already up-to-date #1778

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
152 changes: 130 additions & 22 deletions legacy/builder/phases/linker.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
package phases

import (
"encoding/json"
"fmt"
"strings"

"github.com/arduino/arduino-cli/legacy/builder/builder_utils"
Expand Down Expand Up @@ -63,46 +65,152 @@ func (s *Linker) Run(ctx *types.Context) error {
return nil
}

// CppArchive represents a cpp archive (.a). It has a list of object files
// that must be part of the archive and has functions to build the archive
// and check if the archive is up-to-date.
type CppArchive struct {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is CppArchive a good name here? It is not Cpp-specific, but really just contains object files (which could be generated from C or assembly sources too)?

ArchivePath *paths.Path
CacheFilePath *paths.Path
Objects paths.PathList
}

// NewCppArchive creates an empty CppArchive
func NewCppArchive(archivePath *paths.Path) *CppArchive {
return &CppArchive{
ArchivePath: archivePath,
CacheFilePath: archivePath.Parent().Join(archivePath.Base() + ".cache"),
Objects: paths.NewPathList(),
}
}

// AddObject adds an object file in the list of files to be archived
func (a *CppArchive) AddObject(object *paths.Path) {
a.Objects.Add(object)
}

func (a *CppArchive) readCachedFilesList() paths.PathList {
var cache paths.PathList
if cacheData, err := a.CacheFilePath.ReadFile(); err != nil {
return nil
} else if err := json.Unmarshal(cacheData, &cache); err != nil {
return nil
} else {
return cache
}
}

func (a *CppArchive) writeCachedFilesList() error {
if cacheData, err := json.Marshal(a.Objects); err != nil {
panic(err) // should never happen
umbynos marked this conversation as resolved.
Show resolved Hide resolved
} else if err := a.CacheFilePath.WriteFile(cacheData); err != nil {
return err
} else {
return nil
}
}

// IsUpToDate checks if an already made archive is up-to-date. If this
// method returns true, there is no need to Create the archive.
func (a *CppArchive) IsUpToDate() bool {
archiveStat, err := a.ArchivePath.Stat()
if err != nil {
return false
}

cache := a.readCachedFilesList()
if cache == nil || cache.Len() != a.Objects.Len() {
return false
}
for _, object := range cache {
objectStat, err := object.Stat()
if err != nil {
return false
}
if objectStat.ModTime().After(archiveStat.ModTime()) {
return false
}
}

return true
}

// Create will create the archive using the given arPattern
func (a *CppArchive) Create(ctx *types.Context, arPattern string) error {
_ = a.ArchivePath.Remove()
for _, object := range a.Objects {
properties := properties.NewMap()
properties.Set("archive_file", a.ArchivePath.Base())
properties.SetPath("archive_file_path", a.ArchivePath)
properties.SetPath("object_file", object)
properties.Set("recipe.ar.pattern", arPattern)
command, err := builder_utils.PrepareCommandForRecipe(properties, "recipe.ar.pattern", false, ctx.PackageManager.GetEnvVarsForSpawnedProcess())
if err != nil {
return errors.WithStack(err)
}

if _, _, err := utils.ExecCommand(ctx, command, utils.ShowIfVerbose /* stdout */, utils.Show /* stderr */); err != nil {
return errors.WithStack(err)
}
}

if err := a.writeCachedFilesList(); err != nil {
ctx.Info("Error writing archive cache: " + err.Error())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why ctx.Info to log an error? I would use something else

}
return nil
}

func link(ctx *types.Context, objectFiles paths.PathList, coreDotARelPath *paths.Path, coreArchiveFilePath *paths.Path, buildProperties *properties.Map) error {
objectFileList := strings.Join(utils.Map(objectFiles.AsStrings(), wrapWithDoubleQuotes), " ")

// If command line length is too big (> 30000 chars), try to collect the object files into archives
// and use that archives to complete the build.
if len(objectFileList) > 30000 {
buildObjectFiles := objectFiles.Clone()
buildObjectFiles.FilterOutSuffix(".a")
buildArchiveFiles := objectFiles.Clone()
buildArchiveFiles.FilterSuffix(".a")

// We must create an object file for each visited directory: this is required becuase gcc-ar checks
// if an object file is already in the archive by looking ONLY at the filename WITHOUT the path, so
// 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 := 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()
// Split objects by directory and create a CppArchive for each directory
archives := []*CppArchive{}
{
generatedArchivesFiles := map[string]*CppArchive{}
for _, object := range buildObjectFiles {
archive := object.Parent().Join("objs.a")
a := generatedArchivesFiles[archive.String()]
if a == nil {
a = NewCppArchive(archive)
archives = append(archives, a)
generatedArchivesFiles[archive.String()] = a
}
a.AddObject(object)
}
properties.Set("archive_file", archive.Base())
properties.SetPath("archive_file_path", archive)
properties.SetPath("object_file", object)
}

command, err := builder_utils.PrepareCommandForRecipe(properties, constants.RECIPE_AR_PATTERN, false, ctx.PackageManager.GetEnvVarsForSpawnedProcess())
if err != nil {
return errors.WithStack(err)
}
arPattern := buildProperties.ExpandPropsInString(buildProperties.Get("recipe.ar.pattern"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this done for efficiency?

It breaks compilation of sketches that trigger the archive condition for platforms that use the common convention of providing a backwards compatibility fallback definition of archive_file_path, to be overridden by modern versions of the build system. For example:

https://github.com/arduino/ArduinoCore-samd/blob/1.8.13/platform.txt#L99-L100

# archive_file_path is needed for backwards compatibility with IDE 1.6.5 or older, IDE 1.6.6 or newer overrides this value
archive_file_path={build.path}/{archive_file}

Expanding recipe.ar.pattern before redefining archive_file_path causes archive_file_path to be expanded to {build.path}/{archive_file} with configurations like in arduino:samd, meaning the override of archive_file_path later in the github.com/arduino/arduino-cli/legacy/builder/phases.Create has no effect on the recipe.ar.pattern that generates the executed command.

I think the complete buildProperties object data must be passed to github.com/arduino/arduino-cli/legacy/builder/phases.Create to cover all possible platform configurations.

$ arduino-cli version
arduino-cli.exe  Version: git-snapshot Commit: 9bd93a52 Date: 2022-06-27T21:48:22Z

$ ./arduino-cli compile --clean --fqbn arduino:samd:mkrwifi1010 C:/Users/per/Documents/Arduino/libraries/ArduinoIoTCloud/examples/ArduinoIoTCloud-Advanced
arm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\sketch\objs.a: No such file or directory
arm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\Arduino_ConnectionHandler\objs.a: No such file or directory
arm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\WiFiNINA\objs.a: No such file or directory
arm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\WiFiNINA\utility\objs.a: No such file or directory
arm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\Arduino_DebugUtils\objs.a: No such file or directory
arm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\ArduinoIoTCloud\objs.a: No such file or directory
arm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\ArduinoIoTCloud\cbor\objs.a: No such file or directory
arm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\ArduinoIoTCloud\cbor\lib\tinycbor\src\objs.a: No such file or directory
arm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\ArduinoIoTCloud\property\objs.a: No such file or directory
arm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\ArduinoIoTCloud\tls\objs.a: No such file or directory
arm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\ArduinoIoTCloud\tls\bearssl\objs.a: No such file or directory
arm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\ArduinoIoTCloud\tls\profile\objs.a: No such file or directory
arm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\ArduinoIoTCloud\tls\utility\objs.a: No such file or directory
arm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\ArduinoIoTCloud\utility\ota\objs.a: No such file or directory
arm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\ArduinoIoTCloud\utility\time\objs.a: No such file or directory
arm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\ArduinoIoTCloud\utility\watchdog\objs.a: No such file or directory
arm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\RTCZero\objs.a: No such file or directory
arm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\ArduinoECCX08\objs.a: No such file or directory
arm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\ArduinoECCX08\utility\objs.a: No such file or directory
arm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\Wire\objs.a: No such file or directory
arm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\ArduinoMqttClient\objs.a: No such file or directory
arm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\SPI\objs.a: No such file or directory
arm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\SNU\objs.a: No such file or directory
arm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\Adafruit_SleepyDog_Library\objs.a: No such file or directory
arm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\Adafruit_SleepyDog_Library\utility\objs.a: No such file or directory
arm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\core\objs.a: No such file or directory


Used library               Version Path
Arduino_ConnectionHandler  0.6.6   C:\Users\per\Documents\Arduino\libraries\Arduino_ConnectionHandler
WiFiNINA                   1.8.13  C:\Users\per\Documents\Arduino\libraries\WiFiNINA
Arduino_DebugUtils         1.3.0   C:\Users\per\Documents\Arduino\libraries\Arduino_DebugUtils
ArduinoIoTCloud            1.6.1   C:\Users\per\Documents\Arduino\libraries\ArduinoIoTCloud
RTCZero                    1.6.0   C:\Users\per\Documents\Arduino\libraries\RTCZero                                         
ArduinoECCX08              1.3.6   C:\Users\per\Documents\Arduino\libraries\ArduinoECCX08
Wire                       1.0     C:\Users\per\AppData\Local\Arduino15\packages\arduino\hardware\samd\1.8.13\libraries\Wire
ArduinoMqttClient          0.1.5   C:\Users\per\Documents\Arduino\libraries\ArduinoMqttClient
SPI                        1.0     C:\Users\per\AppData\Local\Arduino15\packages\arduino\hardware\samd\1.8.13\libraries\SPI
SNU                        1.0.2   C:\Users\per\AppData\Local\Arduino15\packages\arduino\hardware\samd\1.8.13\libraries\SNU
Adafruit_SleepyDog_Library 1.6.1   C:\Users\per\Documents\Arduino\libraries\Adafruit_SleepyDog_Library

Used platform Version Path
arduino:samd  1.8.13  C:\Users\per\AppData\Local\Arduino15\packages\arduino\hardware\samd\1.8.13

Error during build: exit status 1

Related to arduino/Arduino#4431

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it was an equivalent change, the purpose is to remove ctx dependencies as much as possible with the final goal to move away from the legacy package... but turns out to be wrong. :-)

Thanks for finding the error and to write this detailed description!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the very unlikely chance it might be of use, I'll share the patch from the fix I made while verifying the problem I reported above:

diff --git a/legacy/builder/phases/linker.go b/legacy/builder/phases/linker.go
index 858a120b..1db06247 100644
--- a/legacy/builder/phases/linker.go
+++ b/legacy/builder/phases/linker.go
@@ -135,15 +135,13 @@ func (a *CppArchive) IsUpToDate() bool {
 }
 
 // Create will create the archive using the given arPattern
-func (a *CppArchive) Create(ctx *types.Context, arPattern string) error {
+func (a *CppArchive) Create(ctx *types.Context, properties properties.Map) error {
 	_ = a.ArchivePath.Remove()
 	for _, object := range a.Objects {
-		properties := properties.NewMap()
 		properties.Set("archive_file", a.ArchivePath.Base())
 		properties.SetPath("archive_file_path", a.ArchivePath)
 		properties.SetPath("object_file", object)
-		properties.Set("recipe.ar.pattern", arPattern)
-		command, err := builder_utils.PrepareCommandForRecipe(properties, "recipe.ar.pattern", false, ctx.PackageManager.GetEnvVarsForSpawnedProcess())
+		command, err := builder_utils.PrepareCommandForRecipe(&properties, "recipe.ar.pattern", false, ctx.PackageManager.GetEnvVarsForSpawnedProcess())
 		if err != nil {
 			return errors.WithStack(err)
 		}
@@ -191,8 +189,6 @@ func link(ctx *types.Context, objectFiles paths.PathList, coreDotARelPath *paths
 			}
 		}
 
-		arPattern := buildProperties.ExpandPropsInString(buildProperties.Get("recipe.ar.pattern"))
-
 		filesToLink := paths.NewPathList()
 		for _, a := range archives {
 			filesToLink.Add(a.ArchivePath)
@@ -202,7 +198,7 @@ func link(ctx *types.Context, objectFiles paths.PathList, coreDotARelPath *paths
 				}
 				continue
 			}
-			if err := a.Create(ctx, arPattern); err != nil {
+			if err := a.Create(ctx, *buildProperties); err != nil {
 				return err
 			}
 		}


if _, _, err := utils.ExecCommand(ctx, command, utils.ShowIfVerbose /* stdout */, utils.Show /* stderr */); err != nil {
return errors.WithStack(err)
filesToLink := paths.NewPathList()
for _, a := range archives {
filesToLink.Add(a.ArchivePath)
if a.IsUpToDate() {
if ctx.Verbose {
ctx.Info(fmt.Sprintf("%s %s", tr("Using previously build archive:"), a.ArchivePath))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you're here, I think there's a typo here too: s/build/built/

}
continue
}
if err := a.Create(ctx, arPattern); err != nil {
return err
}
}

objectFileList = strings.Join(utils.Map(archives.AsStrings(), wrapWithDoubleQuotes), " ")
// Add all remaining archives from the build
filesToLink.AddAll(buildArchiveFiles)

objectFileList = strings.Join(utils.Map(filesToLink.AsStrings(), wrapWithDoubleQuotes), " ")
objectFileList = "-Wl,--whole-archive " + objectFileList + " -Wl,--no-whole-archive"
}

Expand Down