[GHA] Introduce reusable matrix workflow #2789

FranzBusch · 2024-07-15T09:07:58Z

Motivation

We have a few checks that we want to run across all supported Swift versions e.g. unit tests and benchmarks. Right now we already added two matrix based jobs to the reusable workflow. This leads to a lot of duplication and harder maintenance.

Modification

This PR splits off the current reusable workflow to only contain soundness related checks and introduces a new reusable matrix based workflow which can be customized to execute different commands.

Result

It is now easy to setup multiple matrix based workflows.

FranzBusch · 2024-07-15T11:27:01Z

Benchmarks and Unit tests are expected to fail since the script isn't on main yet. The previous run showed the expected failures of the nightly-next and nightly-main unit test check due to warnings in the build.

FranzBusch · 2024-07-15T11:30:31Z

@swift-server-bot test this please

# Motivation We have a few checks that we want to run across all supported Swift versions e.g. unit tests and benchmarks. Right now we already added two matrix based jobs to the reusable workflow. This leads to a lot of duplication and harder maintenance. # Modification This PR splits off the current reusable workflow to only contain soundness related checks and introduces a new reusable matrix based workflow which can be customized to execute different commands. # Result It is now easy to setup multiple matrix based workflows.

FranzBusch · 2024-07-15T11:38:42Z

Our Jenkins CI seems to have some problems today

glbrntt · 2024-07-15T12:32:31Z

.github/workflows/pull_request_swift_matrix.yml

+ matrix_linux_5_8_enabled:
+ type: string
+ description: "The command of the 5.8 Swift version linux matrix job to execute."


This and all the other _enableds don't seem right, they should surely be booleans?

actually, are at the _enableds used at all?

Thanks. Good catch!

glbrntt · 2024-07-15T12:34:15Z

.github/workflows/pull_request_swift_matrix.yml

+ - image: swift:5.10-jammy
+ swift_version: "5.10"
+ - image: swiftlang/swift:nightly-6.0-jammy
+ swift_version: "nightly-next"


Should the version be more specific? e.g. "nightly-6.0"?

The next version number is often decided quite close to the release so I wanted to keep it general with next

I don't think that's true, it's must be known when it branches and we know it at this point because we have docker image for it. Before the version is known it's just "main" which is a separate entry.

Earlier you made the point about not having "current", "current-minus-1" etc., but "next" falls into that same pattern.

As an example the next release was called swift 5.11 for a long time until it got renamed to swift 6.

I just changed it to nightly-6.0 now.

But there was never a 5.11 branch or docker image, it only appeared in swiftc --version. At this point, where we're selecting a docker image we must know the version (apart from main builds)

.github/workflows/pull_request_swift_matrix.yml

MahdiBM · 2024-07-15T12:45:19Z

.github/workflows/pull_request_soundness.yml

@@ -50,64 +38,10 @@ on:

 ## We are cancelling previously triggered workflow runs
 concurrency:
- group: ${{ github.workflow }}-${{ github.ref }}
+ group: ${{ github.workflow }}-${{ github.ref }}-soundness


I ... don't think this addition is needed? Did you run into a problem and this solved it?

Yes. Without this addition the different reusable workflows cancel each other

MahdiBM · 2024-07-15T12:45:21Z

.github/workflows/pull_request_swift_matrix.yml

+ name:
+ type: string
+ description: "The name of the workflow used for the concurrency group."
+ required: true
+ matrix_linux_command:
+ type: string
+ description: "The command of the current Swift version linux matrix job to execute."
+ required: true
+ matrix_linux_5_8_enabled:
+ type: string
+ description: "The command of the 5.8 Swift version linux matrix job to execute."
+ matrix_linux_5_8_command_override:
+ type: string
+ description: "The command of the 5.8 Swift version linux matrix job to execute."
+ matrix_linux_5_9_enabled:
+ type: string
+ description: "The command of the 5.9 Swift version linux matrix job to execute."
+ matrix_linux_5_9_command_override:
+ type: string
+ description: "The command of the 5.9 Swift version linux matrix job to execute."
+ matrix_linux_5_10_enabled:
+ type: string
+ description: "The command of the 5.10 Swift version linux matrix job to execute."
+ matrix_linux_5_10_command_override:
+ type: string
+ description: "The command of the 5.10 Swift version linux matrix job to execute."
+ matrix_linux_nightly_next_enabled:
+ type: string
+ description: "The command of the nightly next Swift version linux matrix job to execute."
+ matrix_linux_nightly_next_command_override:
+ type: string
+ description: "The command of the nightly next Swift version linux matrix job to execute."
+ matrix_linux_nightly_main_enabled:
+ type: string
+ description: "The command of the nightly main Swift version linux matrix job to execute."
+ matrix_linux_nightly_main_command_override:
+ type: string
+ description: "The command of the nightly main Swift version linux matrix job to execute."


Seems redundant. You could accept key-value pairs in a JSON.

fromJson(inputs.matrix_linux_command_override).510

assuming we have a

matrix_linux_command_override: type: string

I haven't tried but it think using brackets should also work:

fromJson(inputs.matrix_linux_command_override)[matrix.swift_version]

I don't like using a JSON for that since we get no validation from the GitHub side on that. My thinking was that once we bump the minimum version and remove/add new Swift versions we change the names of the overrides and if some repo defined one their CI will fail due to yaml validations. Which gives them a good indication what changed

hmm but do you really want to be backward incompatible? I think that's not a great idea to be honest.
Imagine doing a PR and noticing that CI is failing due to unrelated reasons.
I guess the maintainers can be advised to look out for these kinds of changes: "if CI failed and was complaining about an invalid value in the inputs, that's because we've probably removed it". But is that a good idea?

Lukasa · 2024-07-15T17:06:52Z

Merging over flaky benchmark

* apple/main: (26 commits) [GHA] Only format Swift files that are in Git index (apple#2797) Ignore format commit from git blame (apple#2796) Adopt swift-format (apple#2794) Disable warnings as errors on Swift 6 and main (apple#2793) ChannelHandler: provide static (un)wrap(In|Out)bound(In|Out) (apple#2791) Add manual control to NIOLockedValueBox (apple#2786) [GHA] Cxx interoperability compatibility and integration tests check (apple#2790) [GHA] Introduce reusable matrix workflow (apple#2789) Fix benchmark thresholds update script (apple#2783) [GHA] Broken symlink and format check (apple#2787) [GHA] Add license header check (apple#2781) Improved documentation for HTTP Parts to clarify how often each part is received (apple#2775) [GHA] Download the scripts to make workflow reusable (apple#2785) Combine the two NIOAsyncChannel channel handlers (apple#2779) [GHA] Benchmark job (apple#2780) [GHA] Move docs check to script (apple#2776) Add benchmark for creating NIOAsyncChannel (apple#2774) Avoid creating a yield ID counter per async writer (apple#2768) [GHA] Unacceptable language check (apple#2766) Allow in-place mutation of `NIOLoopBoundBox.value` (apple#2771) ... # Conflicts: # Sources/NIOPosix/BSDSocketAPICommon.swift # Sources/NIOPosix/GetaddrinfoResolver.swift # Sources/NIOPosix/HappyEyeballs.swift

FranzBusch assigned glbrntt Jul 15, 2024

FranzBusch added the semver/none No version bump required. label Jul 15, 2024

FranzBusch force-pushed the fb-gha-matrix-workflow branch 20 times, most recently from f25c2ea to 0d38fd0 Compare July 15, 2024 11:26

FranzBusch assigned Lukasa Jul 15, 2024

FranzBusch force-pushed the fb-gha-matrix-workflow branch 3 times, most recently from 2c8e214 to f6a482c Compare July 15, 2024 11:28

FranzBusch enabled auto-merge (squash) July 15, 2024 11:28

FranzBusch force-pushed the fb-gha-matrix-workflow branch from f6a482c to a8ed467 Compare July 15, 2024 11:32

FranzBusch force-pushed the fb-gha-matrix-workflow branch from a8ed467 to 67b123b Compare July 15, 2024 11:34

FranzBusch requested review from Lukasa and glbrntt July 15, 2024 11:44

FranzBusch unassigned Lukasa and glbrntt Jul 15, 2024

glbrntt requested changes Jul 15, 2024

View reviewed changes

MahdiBM reviewed Jul 15, 2024

View reviewed changes

Code review

9fb03fd

FranzBusch requested a review from glbrntt July 15, 2024 13:00

Change nightly next

0cad6b7

glbrntt approved these changes Jul 15, 2024

View reviewed changes

Lukasa disabled auto-merge July 15, 2024 17:06

Lukasa merged commit b1bf036 into main Jul 15, 2024
13 of 25 checks passed

Lukasa deleted the fb-gha-matrix-workflow branch July 15, 2024 17:07

shinGangan mentioned this pull request Jul 23, 2024

[GHA] Call the Soundness workflow apple/swift-openapi-generator#603

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GHA] Introduce reusable matrix workflow #2789

[GHA] Introduce reusable matrix workflow #2789

FranzBusch commented Jul 15, 2024

FranzBusch commented Jul 15, 2024

FranzBusch commented Jul 15, 2024

FranzBusch commented Jul 15, 2024

glbrntt Jul 15, 2024

glbrntt Jul 15, 2024

FranzBusch Jul 15, 2024

glbrntt Jul 15, 2024

FranzBusch Jul 15, 2024

glbrntt Jul 15, 2024

FranzBusch Jul 15, 2024

FranzBusch Jul 15, 2024

glbrntt Jul 15, 2024

MahdiBM Jul 15, 2024

FranzBusch Jul 15, 2024

MahdiBM Jul 15, 2024

FranzBusch Jul 15, 2024

MahdiBM Jul 15, 2024

Lukasa commented Jul 15, 2024

[GHA] Introduce reusable matrix workflow #2789

[GHA] Introduce reusable matrix workflow #2789

Conversation

FranzBusch commented Jul 15, 2024

Motivation

Modification

Result

FranzBusch commented Jul 15, 2024

FranzBusch commented Jul 15, 2024

FranzBusch commented Jul 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa commented Jul 15, 2024