-
-
Notifications
You must be signed in to change notification settings - Fork 371
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?
Conversation
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.
Code looks good, I left some nitpicks inline.
This PR only improves the archive creation done for long commandlines, it does not affect archive creation for the arduino core or dot_a_linkage
. Maybe it would be good to clarify this in the commit message? Are you planning to also improve those later? Would probably be fairly easy with the CppArchive class created?
@@ -197,7 +197,9 @@ func link(ctx *types.Context, objectFiles paths.PathList, coreDotARelPath *paths | |||
for _, a := range archives { | |||
filesToLink.Add(a.ArchivePath) | |||
if a.IsUpToDate() { | |||
ctx.Info(fmt.Sprintf("%s %s", tr("Using previously build archive:"), a.ArchivePath)) | |||
if ctx.Verbose { | |||
ctx.Info(fmt.Sprintf("%s %s", tr("Using previously build archive:"), a.ArchivePath)) |
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.
While you're here, I think there's a typo here too: s/build/built/
// 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 { |
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)?
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.
Left some questions
} | ||
|
||
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 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
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 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
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.
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!
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.
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
}
}
Please check if the PR fulfills these requirements
before creating one)
our contributing guidelines
UPGRADING.md
has been updated with a migration guide (for breaking changes)What kind of change does this PR introduce?
Archive (.a) creation is skipped if the archive is already up-to-date.
We should see a visible speed improvement when re-building libraries with a lot of files.
With this patch:
Without the patch:
What is the current behavior?
The archive is always regenerated even if none of the object files in it are changed.
What is the new behavior?
The archive is not regenerated if none of the object files archived is changed.
Does this PR introduce a breaking change, and is titled accordingly?
No