Skip to content

[skip-changelog] Use a matrix to create parallel builds for each OS #1883

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

Merged
merged 9 commits into from
Sep 29, 2022

Conversation

MatteoPologruto
Copy link
Contributor

@MatteoPologruto MatteoPologruto commented Sep 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?

Infrastructure Enhancement

What is the current behavior?

Release-go-task, publish-go-nightly-task and publish-go-tester-task are currently using task dist:all to build and create the release artifacts. This is done by calling the tasks related to each OS consecutively. As a result, the finishing time of task dist:all is the sum of the finishing times of each individual dist. Furthermore, the checksums related to the files built are calculated more than once, unnecessarily.

What is the new behavior?

  • Using a matrix to run each build task greatly improves performance, since they can all start concurrently. The finishing time of the job will be equal to the one of the longer task to build, instead of being the sum of each individual task's finishing time.
  • Checksums are only being calculated once, during the final steps of the workflows.
  • A check has been introduced to avoid creating the same changelog for each parallel building job.
  • publish-tester-build now uploads a different artifact for each build during the first job, instead of creating a combined one that needs to be split later.

Does this PR introduce a breaking change, and is titled accordingly?

No

Other information:

Release run: https://github.com/MatteoPologruto/arduino-cli/actions/runs/3128175485
Test release: https://github.com/MatteoPologruto/arduino-cli/releases/tag/0.0.3
Nightly build run: https://github.com/MatteoPologruto/arduino-cli/actions/runs/3128099818
Tester build run: https://github.com/MatteoPologruto/arduino-cli/actions/runs/3137219983

Publish-go-tester-task was correctly skipped after run-determination when I pushed the tag 0.0.2 onto my fork (here). Out of curiosity, I triggered the workflow run manually on the same commit and the checksums' calculation was not correct (here). Checksums calculated previously and afterwards seem to be correct, even though the workflow has not been modified. I think it might be worth mentioning.


See how to contribute

@MatteoPologruto MatteoPologruto added type: enhancement Proposed improvement topic: infrastructure Related to project infrastructure labels Sep 20, 2022
@MatteoPologruto MatteoPologruto self-assigned this Sep 20, 2022
@MatteoPologruto MatteoPologruto changed the title Parallel dist Use a matrix to create parallel builds for each OS Sep 20, 2022
@MatteoPologruto MatteoPologruto changed the title Use a matrix to create parallel builds for each OS [skip-changelog]Use a matrix to create parallel builds for each OS Sep 20, 2022
@MatteoPologruto MatteoPologruto marked this pull request as ready for review September 20, 2022 07:11
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.

Wow! The improvement in time is huge! 🚅
Unfortunately this is not sufficient: we need to find another approach for the checksum generation, because the generated one is not complete.
The task dist:all was appending the checksums in the same file and then uploading it (since the task were executed sequentially it was possible). Unfortunately this is no longer working.
I suggest removing the checksum calculation from the tasks, from the notarization step. And perform it only during the release creation job

@umbynos
Copy link
Contributor

umbynos commented Sep 20, 2022

Also it does not make sense to perform the changelog generation in all the jobs.. could be an idea to do it only for one of the runs

@umbynos
Copy link
Contributor

umbynos commented Sep 20, 2022

I've just noticed an unrelated problem with the release CI:

PLATFORM_DIR: "{{.PROJECT_NAME}}_linux_arm_6"

This should be:
PLATFORM_DIR: "{{.PROJECT_NAME}}_linux_arm_64"
It's worth to fix it in this PR IMHO.
More context here

@MatteoPologruto MatteoPologruto force-pushed the parallel-dist branch 8 times, most recently from 0b60a3e to 44d57ca Compare September 22, 2022 12:55
@MatteoPologruto MatteoPologruto changed the title [skip-changelog]Use a matrix to create parallel builds for each OS [skip-changelog] Use a matrix to create parallel builds for each OS Sep 22, 2022
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 suggestions 👾

@MatteoPologruto MatteoPologruto force-pushed the parallel-dist branch 2 times, most recently from 742c3bc to 7335d8e Compare September 26, 2022 10:13
Using a matrix to run each build task greatly improves performances,
since they can all start concurrently. The finishing time of the job
will be equal to the one of the longer task to build, instead of being
the sum of each individual task's finishing time.
Checksums of the output files where previously calculated during the initial creation of the artifacts, during the notarization process and, finally, at the release creation's step.
The whole process has been simplified and checksums are now computed only during the creation of the release.
The changelog is the same for each OS. It does not make sense to generate it more than once.
Previously, the different builds were firstly uploaded using a single artifact, which was then downloaded to create different ones and eventually deleted.
Now, since builds are created concurrently, the same matrix can be used to directly upload an artifact for each build. It's necessary to use a second job to calculate the checksum related to
each build and save them all in a single .txt file.
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.

Great Job Matteo 🥇

@MatteoPologruto MatteoPologruto merged commit 9c784d8 into arduino:master Sep 29, 2022
@MatteoPologruto MatteoPologruto deleted the parallel-dist branch September 29, 2022 13:49
umbynos added a commit to umbynos/arduino-cli that referenced this pull request Oct 5, 2022
umbynos added a commit to umbynos/arduino-cli that referenced this pull request Oct 6, 2022
umbynos added a commit to umbynos/arduino-cli that referenced this pull request Oct 17, 2022
umbynos added a commit that referenced this pull request Oct 17, 2022
* add step to generate a windows msi installer of the Arduino CLI

* remove warning:

`warning : Solution properties are only available during IDE builds or when building the solution file from the command line. To turn off this warning set <DefineSolutionProperties>false</DefineSolutionProperties> in your .wixproj`

* fix error caused by wix peculiar version constraints:

`error CNDL0108: The Product/@Version attribute's value, '0.0.0-test', is not a valid version.  Legal version values should look like 'x.x.x.x' where x is an integer from 0 to 65534.`

* fix `error LGHT0103: The system cannot find the file 'arduino-cli.exe'`

* final adjustments in wix config

* exclude prettier from checking the installer config files, prettier does not support xml out of the box

* add download links

* temporarily disable s3 push and homebrew update for testing

* Upload nightly artifacts for testing

* add windows check certs

* bump to latest windows-sdk and document better

specify timestamp algorithm. Apparently this is required in the latest version of the sdk

* remove useless job id not used anymore (followup of #1883)

* fixed comments

* It's useless to use the output from setupmsbuild

* use YAML-style syntax

* try to uniform the usage of version/tag across the workflow

* Revert "Upload nightly artifacts for testing" && "temporarily disable s3 push and homebrew update for testing"

This reverts commit 3123dfd.
This reverts commit 24fa25c.

* Apply suggestions from code review

Co-authored-by: per1234 <[email protected]>

Co-authored-by: per1234 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: infrastructure Related to project infrastructure type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants