[GHA] Make unit tests, benchmarks and cxx interop easier to reuse #2801

FranzBusch · 2024-07-22T14:40:37Z

Motivation

Currently the pull_request_swift_matrix workflow provided an easy way to run a command across many Swift versions. We do have a few common commands that we want to run across the repos. Namely, unit tests, benchmarks and cxx interop.

Modification

This PR creates new workflows for the above common matrix commands so it's easier to reuse them from other repos.

Result

Even easier to share these workflows.

.github/workflows/benchmarks.yml

simonjbeaumont · 2024-07-23T09:04:47Z

.github/workflows/cxx_interop.yml

+
+jobs:
+    cxx-interop:
+        name: Cxx interop


Nit: Any reason to not spell this as "C++" when we have a string label?

Kinda stayed consistent with the script name and the flag that's set in the Package.swift. Also AFAIK all of C++/Cpp/Cxx is valid to use.

Sure, they're all valid and I'm fine with keeping it. I always assumed Cxx and Cpp were there for when we couldn't use non-ascii characters, like in command line flags and file extensions.

.github/workflows/cxx_interop.yml

simonjbeaumont · 2024-07-23T09:06:42Z

.github/workflows/pull_request.yml

-      name: "Integration tests"
-      matrix_linux_command: "apt-get update -y -q && apt-get install -y -q lsof dnsutils netcat-openbsd net-tools curl jq && ./scripts/integration_tests.sh"
+    integration-tests:
+        name: Checks


A bunch of these all share the same name: Checks, what is the result of that?

That is intentional to get the naming of the checks somewhat aligned. Otherwise it would be Pull Request / Benchmarks / Benchmarks / 5.9

simonjbeaumont · 2024-07-23T09:07:52Z

.github/workflows/pull_request.yml

+        uses: ./.github/workflows/pull_request_swift_matrix.yml
+        with:
+          name: "Integration tests"
+          matrix_linux_command: "apt-get update -y -q && apt-get install -y -q lsof dnsutils netcat-openbsd net-tools curl jq && ./scripts/integration_tests.sh"


Nice. Is this an example of how a project could use the matrix for an arbitrary job that they can name and provide a command for, and have that job be run for all the Swift versions?

Yes exactly. This still allows individual project to configure their own jobs.

simonjbeaumont · 2024-07-23T09:10:14Z

.github/workflows/unit_tests.yml

+        uses: ./.github/workflows/pull_request_swift_matrix.yml
+        with:
+          name: "Unit tests"
+          matrix_linux_command: "swift test -Xswiftc -warnings-as-errors --explicit-target-dependency-import-check error"


IIUC these flags shouldn't be here, since they are provided in the override in the other YAML file?

Wondering if the most acceptable thing to do would be for the default to command to just be swift test and allow folks to provide their override when they adopt, opting into whatever additional level of validation they want for their project.

We need to pick a default here since this input is required. In practice this input will never actually be used because we provide an override for each version specific job. I do think we should have strong defaults though. In particular those two seem like net wins and I would rather people have to disable warnings as errors than the other way around.

If you feel super strongly about it, then fine, but I would have had a slight lean the other way...

I think if we're offering this as a reusable building block it shouldn't be so opinionated so would prefer it for this to be free of anything that isn't the default, i.e. just swift test. I think that will be the least surprising behaviour. It would be annoying for folks to adopt the pipeline when their project currently builds fine (without additional strict flags, and then scratch their heads a little).

You convinced me and I removed the default args now.

# Motivation Currently the `pull_request_swift_matrix` workflow provided an easy way to run a command across many Swift versions. We do have a few common commands that we want to run across the repos. Namely, unit tests, benchmarks and cxx interop. # Modification This PR creates new workflows for the above common matrix commands so it's easier to reuse them from other repos. # Result Even easier to share these workflows.

simonjbeaumont

Looks good. Still not sure I like the defaults but if you really want them, then OK.

simonjbeaumont · 2024-07-24T09:07:01Z

.github/workflows/cxx_interop.yml

+
+jobs:
+    cxx-interop:
+        name: Cxx interop


Sure, they're all valid and I'm fine with keeping it. I always assumed Cxx and Cpp were there for when we couldn't use non-ascii characters, like in command line flags and file extensions.

simonjbeaumont · 2024-07-24T09:09:06Z

.github/workflows/pull_request.yml

-      name: "Integration tests"
-      matrix_linux_command: "apt-get update -y -q && apt-get install -y -q lsof dnsutils netcat-openbsd net-tools curl jq && ./scripts/integration_tests.sh"
+    integration-tests:
+        name: Checks


simonjbeaumont · 2024-07-24T09:12:28Z

.github/workflows/unit_tests.yml

+        uses: ./.github/workflows/pull_request_swift_matrix.yml
+        with:
+          name: "Unit tests"
+          matrix_linux_command: "swift test -Xswiftc -warnings-as-errors --explicit-target-dependency-import-check error"


If you feel super strongly about it, then fine, but I would have had a slight lean the other way...

I think if we're offering this as a reusable building block it shouldn't be so opinionated so would prefer it for this to be free of anything that isn't the default, i.e. just swift test. I think that will be the least surprising behaviour. It would be annoying for folks to adopt the pipeline when their project currently builds fine (without additional strict flags, and then scratch their heads a little).

FranzBusch force-pushed the fb-more-resuable branch 4 times, most recently from 2c7b77a to b0885b4 Compare July 22, 2024 15:30

simonjbeaumont reviewed Jul 23, 2024

View reviewed changes

FranzBusch requested a review from simonjbeaumont July 23, 2024 09:45

FranzBusch added 3 commits July 24, 2024 11:02

Review comments

9ad513d

Pass benchmark path

12bd6c5

FranzBusch force-pushed the fb-more-resuable branch 2 times, most recently from d79e603 to a0d66ee Compare July 24, 2024 09:05

Rename and cleanup

e0372b2

FranzBusch force-pushed the fb-more-resuable branch from a0d66ee to e0372b2 Compare July 24, 2024 09:06

simonjbeaumont approved these changes Jul 24, 2024

View reviewed changes

FranzBusch added 2 commits July 24, 2024 11:22

Remove default arguments for testing

5cb864f

Rename integration test checks

9bc8b3c

FranzBusch merged commit 1d25937 into main Jul 24, 2024
26 checks passed

FranzBusch deleted the fb-more-resuable branch July 24, 2024 10:15

glbrntt added the semver/none No version bump required. label Jul 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GHA] Make unit tests, benchmarks and cxx interop easier to reuse #2801

[GHA] Make unit tests, benchmarks and cxx interop easier to reuse #2801

FranzBusch commented Jul 22, 2024

simonjbeaumont Jul 23, 2024

FranzBusch Jul 23, 2024

simonjbeaumont Jul 24, 2024

simonjbeaumont Jul 23, 2024

FranzBusch Jul 23, 2024

simonjbeaumont Jul 24, 2024

simonjbeaumont Jul 23, 2024

FranzBusch Jul 23, 2024

simonjbeaumont Jul 23, 2024

FranzBusch Jul 23, 2024

simonjbeaumont Jul 24, 2024

FranzBusch Jul 24, 2024

simonjbeaumont left a comment

simonjbeaumont Jul 24, 2024

simonjbeaumont Jul 24, 2024

simonjbeaumont Jul 24, 2024

[GHA] Make unit tests, benchmarks and cxx interop easier to reuse #2801

[GHA] Make unit tests, benchmarks and cxx interop easier to reuse #2801

Conversation

FranzBusch commented Jul 22, 2024

Motivation

Modification

Result

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

simonjbeaumont left a 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