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

Conversation

cmaglie
Copy link
Member

@cmaglie cmaglie commented Jun 20, 2022

Please check if the PR fulfills these requirements

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • The PR follows
    our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • 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:

~/Arduino/IoTTest$ time arduino-cli compile -b arduino:mbed_nano:nanorp2040connect 
real	0m4,006s
user	0m3,279s
sys	0m1,197s

Without the patch:

~/Arduino/IoTTest$ time arduino-cli compile -b arduino:mbed_nano:nanorp2040connect 
real	0m12,303s
user	0m5,453s
sys	0m7,315s

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

@cmaglie cmaglie self-assigned this Jun 20, 2022
@cmaglie cmaglie requested a review from a team June 20, 2022 09:05
Copy link
Collaborator

@matthijskooijman matthijskooijman left a 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))
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/

// 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)?

Copy link
Contributor

@umbynos umbynos left a 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())
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

legacy/builder/phases/linker.go Show resolved Hide resolved
@per1234 per1234 added the topic: code Related to content of the project itself label Jun 22, 2022
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
 			}
 		}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants