-
-
Notifications
You must be signed in to change notification settings - Fork 388
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,8 @@ | |
package phases | ||
|
||
import ( | ||
"encoding/json" | ||
"fmt" | ||
"strings" | ||
|
||
"github.com/arduino/arduino-cli/legacy/builder/builder_utils" | ||
|
@@ -63,46 +65,150 @@ 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 { | ||
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()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why |
||
} | ||
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")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 https://github.com/arduino/ArduinoCore-samd/blob/1.8.13/platform.txt#L99-L100
Expanding I think the complete
Related to arduino/Arduino#4431 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought it was an equivalent change, the purpose is to remove Thanks for finding the error and to write this detailed description! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() { | ||
ctx.Info(fmt.Sprintf("%s %s", tr("Using previously build archive:"), a.ArchivePath)) | ||
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" | ||
} | ||
|
||
|
There was a problem hiding this comment.
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)?