-
-
Notifications
You must be signed in to change notification settings - Fork 398
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
Correct conditional logic for publishing steps of build workflow #2261
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The "Arduino IDE" GitHub Actions workflow is used to generate several distinct types of builds: - Tester builds of commits - Nightly builds - Release builds Different actions must be performed depending on which type of build is being produced. The workflow uses dedicated jobs for publishing the nightly builds and for publishing the release builds. Those jobs are configured to run only when certain criteria are met. One such criteria is that the merge-channel-files job ran as expected. There are four possible result types of a job, which should be handled as follows: | Result | Run dependent job? | | -------- | ------------------ | | success | Yes | | failure | No | | canceled | No | | skipped | Yes | GitHub Actions automatically takes the desired action regarding whether the dependent job should run for the first three result types, but that is not the case for the "skipped" result. The merge-channel-files job dependency is skipped when a channel file merge is not needed and so this is not cause to cancel the build publishing. The only way to make a dependent job run when the dependency was skipped is to add the `always()` expression to the job conditional. This goes too far in the other direction by causing the job to run even when the dependency failed or was canceled. So it is necessary to also add logic for each of the dependency job result types to the conditional, which makes it quite complex when combined with the logic for the other criteria of the job. In order to reduce the amount of complexity of the conditionals of the dependent jobs, a job was interposed in the dependency chain, which was intended to act simply as a container for the logic about the merge-channel-files job result. Unfortunately it turns out that even if the direct dependency job's result was success, if any ancestor in the dependency chain was skipped, GitHub Actions still skips all dependent jobs without an `always()` expression in their conditional, meaning the intermediate job was pointless. This caused the build publishing jobs to be skipped under the conditions where they should have ran. The pointless intermediate job is hereby removed, an `always()` expression added to the conditionals of the dependent jobs, and the full logic for how to handle each dependent job result type added there as well.
In addition to the builds, when a nightly or production release is published, a "channel update info file" is also uploaded to Amazon S3 by the "Arduino IDE" GitHub Actions workflow. The IDE checks the channel file on the server to get information about available updates. Previously the Linux production release builds were being remade manually due to the ones produced by GitHub Actions not being compatible with older distro versions due to a dynamically linked dependency. For this reason, a step was temporarily added to the workflow to cause it to not upload the Linux channel file in order to avoid Linux users from receiving an update offer before the limited compatibility automated build had been replaced with the manually produced build. The automated build system has been adjusted to produce Linux builds with the intended range of compatibility, but the step that deleted the channel file was not removed at that time. The obviated step is hereby removed in order to allow complete releases to be published by the workflow.
Python 3.12.x is incompatible with the current version of node-gyp (9.4.0). For this reason, it is necessary to configure the "GitHub Actions" workflow to use the compatible Python 3.11.x until the next release of node-gyp (which should contain a fix for the breakage) is made.
The "Arduino IDE" GitHub Actions workflow uses a Docker container for the build job of certain target, while running others directly in the runner environment. Due to differences between these two environments, some steps must run only when a container is used by the job, and others only when the job is running in the runner environment. This is done by a conditional on the job matrix data regarding the container. The container value is set to null for the jobs that run in the runner environment, while containing a full container configuration mapping for the jobs that run in a container. Previously the conditional unnecessarily used the value of the image key of the container object specifically. The comparison can be done against the container value itself. Doing this will make it a little easier to understand the workflow code, since the conditional more clearly reflects the matrix (where `container` is set to `null` instead of `container.image`).
per1234
added
topic: infrastructure
Related to project infrastructure
type: imperfection
Perceived defect in any part of project
labels
Oct 19, 2023
kittaakos
approved these changes
Oct 19, 2023
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.
It's looking good, Per 👍
Thanks for the detailed explanation of each commit + the example workflow runs.
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: imperfection
Perceived defect in any part of project
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Background
The "Arduino IDE" GitHub Actions workflow is used to generate several distinct types of builds:
Different operations must be performed depending on which type of build is being produced. The workflow uses dedicated jobs for publishing the nightly builds and for publishing the release builds. Those jobs are configured to run only when certain criteria are met.
One such criteria is that the
merge-channel-files
job ran as expected. There are four possible result types of a job, which should be handled as follows:GitHub Actions automatically takes the desired action regarding whether the dependent job should run for the first three result types, but that is not the case for the "skipped" result. The
merge-channel-files
job dependency is skipped when a channel file merge is not needed and so this is not cause to cancel the build publishing.The only way to make a dependent job run when the dependency was skipped is to add the
always()
expression to the job conditional. This goes too far in the other direction by causing the job to run even when the dependency failed or was canceled. So it is necessary to also add logic for each of the dependency job result types to the conditional, which makes it quite complex when combined with the logic for the other criteria of the job.Problem
In order to reduce the amount of complexity of the conditionals of the dependent jobs, a job (
merge-channel-files-complete
) was interposed in the dependency chain, which was intended to act simply as a container for the logic about themerge-channel-files
job result. Unfortunately it turns out that even if the direct dependency job's result was success, if any ancestor in the dependency chain was skipped, GitHub Actions still skips all dependent jobs without analways()
expression in their conditional, meaning the intermediate job was pointless. This caused the build publishing jobs to be skipped under the conditions where they should have ran.Solution
The pointless intermediate job is hereby removed, an
always()
expression added to the conditionals of the dependent jobs, and the full logic for how to handle each dependent job result type added there as well.Additional Changes
Pin Python to 3.11.x minor version
97b0bc0
I found that the node-gyp installation was broken by the update to Python 3.12 in the GitHub Actions runner: nodejs/node-gyp#2869
This caused a failure of the workflow runs unrelated to the changes made by this PR:
https://github.com/per1234/arduino-ide/actions/runs/6566684071/job/17837889420#step:8:247
The workaround is to pin the version of Python used for the build job in the runner environment to 3.11.
I believe the node-gyp maintainers are already working on a fix, which will be in the next release so we can likely revert this pin at some point in the future if it is preferred to only pin the major version of Python.
Restore Linux channel file uploading
dd79c63
The Linux channel file upload was disabled due to the fact that we were manually producing that build (#2020). The problem causing the need for a manually produced Linux build was fixed (#2253), but I forgot to restore the channel file upload at that time.
Minor workflow refactoring
883426d
I made a small change to the conditionals for the container/runner environment-specific steps of the build job. This is unrelated to the other work and has no functional effect, but I think it makes the workflow a little easier to understand by matching the comparison in the conditional to the matrix data.
Information for reviewers
I did some demonstration runs of the workflow (in addition to the tester build run that will be associated with the PR):
Triggered by
workflow_dispatch
event:https://github.com/arduino/arduino-ide/actions/runs/6568764692
Triggered by
schedule
event (which causes a "nightly" build publishing):https://github.com/per1234/arduino-ide/actions/runs/6567577721
This was performed in my fork, where I don't have the AWS credentials, so the failure of the S3 upload step in the "publish" job is expected, but you can see the rest of the workflow was successful
Production release:
https://github.com/per1234/arduino-ide/actions/runs/6567497724
This was performed in my fork, where I don't have the AWS credentials, so the failure of the S3 upload step in the "release" job is expected, but you can see the rest of the workflow was successful, including the creation of the GitHub Release: https://github.com/per1234/arduino-ide/releases/tag/0.0.0-rc.32
Additional context
Originally reported by @KurtE:
https://forum.arduino.cc/t/where-are-wifis3-examples-hiding/1178596/12