Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork436
Skip archive (.a) creation if the archive is already up-to-date#1778
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
matthijskooijman left a comment
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 ordot_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?
| ifa.IsUpToDate() { | ||
| ctx.Info(fmt.Sprintf("%s %s",tr("Using previously build archive:"),a.ArchivePath)) | ||
| ifctx.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. | ||
| typeCppArchivestruct { |
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)?
umbynos left a comment
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
| } | ||
| iferr:=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.
Whyctx.Info to log an error? I would use something else
Uh oh!
There was an error while loading.Please reload this page.
| iferr!=nil { | ||
| returnerrors.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 ofarchive_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 valuearchive_file_path={build.path}/{archive_file}Expandingrecipe.ar.pattern before redefiningarchive_file_path causesarchive_file_path to be expanded to{build.path}/{archive_file} with configurations like inarduino:samd, meaning the override ofarchive_file_path later in thegithub.com/arduino/arduino-cli/legacy/builder/phases.Create has no effect on therecipe.ar.pattern that generates the executed command.
I think the completebuildProperties object data must be passed togithub.com/arduino/arduino-cli/legacy/builder/phases.Create to cover all possible platform configurations.
$ arduino-cli versionarduino-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-Advancedarm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\sketch\objs.a: No such file or directoryarm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\Arduino_ConnectionHandler\objs.a: No such file or directoryarm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\WiFiNINA\objs.a: No such file or directoryarm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\WiFiNINA\utility\objs.a: No such file or directoryarm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\Arduino_DebugUtils\objs.a: No such file or directoryarm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\ArduinoIoTCloud\objs.a: No such file or directoryarm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\ArduinoIoTCloud\cbor\objs.a: No such file or directoryarm-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 directoryarm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\ArduinoIoTCloud\property\objs.a: No such file or directoryarm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\ArduinoIoTCloud\tls\objs.a: No such file or directoryarm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\ArduinoIoTCloud\tls\bearssl\objs.a: No such file or directoryarm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\ArduinoIoTCloud\tls\profile\objs.a: No such file or directoryarm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\ArduinoIoTCloud\tls\utility\objs.a: No such file or directoryarm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\ArduinoIoTCloud\utility\ota\objs.a: No such file or directoryarm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\ArduinoIoTCloud\utility\time\objs.a: No such file or directoryarm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\ArduinoIoTCloud\utility\watchdog\objs.a: No such file or directoryarm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\RTCZero\objs.a: No such file or directoryarm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\ArduinoECCX08\objs.a: No such file or directoryarm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\ArduinoECCX08\utility\objs.a: No such file or directoryarm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\Wire\objs.a: No such file or directoryarm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\ArduinoMqttClient\objs.a: No such file or directoryarm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\SPI\objs.a: No such file or directoryarm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\SNU\objs.a: No such file or directoryarm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\Adafruit_SleepyDog_Library\objs.a: No such file or directoryarm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\libraries\Adafruit_SleepyDog_Library\utility\objs.a: No such file or directoryarm-none-eabi-g++: error: C:\Users\per\AppData\Local\Temp\arduino-sketch-25293530AF8F73C068F9AD108D407B3C\core\objs.a: No such file or directoryUsed library Version PathArduino_ConnectionHandler 0.6.6 C:\Users\per\Documents\Arduino\libraries\Arduino_ConnectionHandlerWiFiNINA 1.8.13 C:\Users\per\Documents\Arduino\libraries\WiFiNINAArduino_DebugUtils 1.3.0 C:\Users\per\Documents\Arduino\libraries\Arduino_DebugUtilsArduinoIoTCloud 1.6.1 C:\Users\per\Documents\Arduino\libraries\ArduinoIoTCloudRTCZero 1.6.0 C:\Users\per\Documents\Arduino\libraries\RTCZero ArduinoECCX08 1.3.6 C:\Users\per\Documents\Arduino\libraries\ArduinoECCX08Wire 1.0 C:\Users\per\AppData\Local\Arduino15\packages\arduino\hardware\samd\1.8.13\libraries\WireArduinoMqttClient 0.1.5 C:\Users\per\Documents\Arduino\libraries\ArduinoMqttClientSPI 1.0 C:\Users\per\AppData\Local\Arduino15\packages\arduino\hardware\samd\1.8.13\libraries\SPISNU 1.0.2 C:\Users\per\AppData\Local\Arduino15\packages\arduino\hardware\samd\1.8.13\libraries\SNUAdafruit_SleepyDog_Library 1.6.1 C:\Users\per\Documents\Arduino\libraries\Adafruit_SleepyDog_LibraryUsed platform Version Patharduino:samd 1.8.13 C:\Users\per\AppData\Local\Arduino15\packages\arduino\hardware\samd\1.8.13Error during build: exit status 1Related toarduino/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 removectx dependencies as much as possible with the final goal to move away from thelegacy 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.goindex 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.mdhas 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 istitled accordingly?
No