[GHA] Make unit tests, benchmarks and cxx interop easier to reuse #2801
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| name: Benchmarks | ||
|
|
||
| on: | ||
| workflow_call: | ||
| inputs: | ||
| benchmark_package_path: | ||
| type: string | ||
| description: "Path to the directory containing the benchmarking package. Defaults to ." | ||
| default: "." | ||
| swift_package_arguments: | ||
| type: string | ||
| description: "Arguments to the switch package command invocation e.g. `--disable-sandbox`" | ||
| linux_5_8_enabled: | ||
| type: boolean | ||
| description: "Boolean to enable the Linux 5.8 Swift version matrix job. Defaults to true." | ||
| default: true | ||
| linux_5_9_enabled: | ||
| type: boolean | ||
| description: "Boolean to enable the Linux 5.9 Swift version matrix job. Defaults to true." | ||
| default: true | ||
| linux_5_10_enabled: | ||
| type: boolean | ||
| description: "Boolean to enable the Linux 5.10 Swift version matrix job. Defaults to true." | ||
| default: true | ||
| linux_nightly_6_0_enabled: | ||
| type: boolean | ||
| description: "Boolean to enable the Linux nightly 6.0 Swift version matrix job. Defaults to true." | ||
| default: true | ||
| linux_nightly_main_enabled: | ||
| type: boolean | ||
| description: "Boolean to enable the Linux nightly main Swift version matrix job. Defaults to true." | ||
| default: true | ||
|
|
||
| jobs: | ||
| benchmarks: | ||
| name: Benchmarks | ||
| uses: ./.github/workflows/swift_matrix.yml | ||
| with: | ||
| name: "Benchmarks" | ||
| matrix_linux_command: "apt-get update -y -q && apt-get install -y -q libjemalloc-dev && swift package --package-path ${{ inputs.benchmark_package_path }} ${{ inputs.swift_package_arguments }} benchmark baseline check --check-absolute-path ${{ inputs.benchmark_package_path }}/Thresholds/${SWIFT_VERSION}/" | ||
| matrix_linux_5_8_enabled: ${{ inputs.linux_5_8_enabled }} | ||
| matrix_linux_5_9_enabled: ${{ inputs.linux_5_9_enabled }} | ||
| matrix_linux_5_10_enabled: ${{ inputs.linux_5_10_enabled }} | ||
| matrix_linux_nightly_6_0_enabled: ${{ inputs.linux_nightly_6_0_enabled }} | ||
| matrix_linux_nightly_main_enabled: ${{ inputs.linux_nightly_main_enabled }} |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| name: Cxx interop | ||
|
|
||
| on: | ||
| workflow_call: | ||
| inputs: | ||
| linux_5_8_enabled: | ||
| type: boolean | ||
| description: "Boolean to enable the Linux 5.8 Swift version matrix job. Defaults to true." | ||
| default: true | ||
| linux_5_9_enabled: | ||
| type: boolean | ||
| description: "Boolean to enable the Linux 5.9 Swift version matrix job. Defaults to true." | ||
| default: true | ||
| linux_5_10_enabled: | ||
| type: boolean | ||
| description: "Boolean to enable the Linux 5.10 Swift version matrix job. Defaults to true." | ||
| default: true | ||
| linux_nightly_6_0_enabled: | ||
| type: boolean | ||
| description: "Boolean to enable the Linux nightly 6.0 Swift version matrix job. Defaults to true." | ||
| default: true | ||
| linux_nightly_main_enabled: | ||
| type: boolean | ||
| description: "Boolean to enable the Linux nightly main Swift version matrix job. Defaults to true." | ||
| default: true | ||
|
|
||
| jobs: | ||
| cxx-interop: | ||
| name: Cxx interop | ||
| uses: ./.github/workflows/swift_matrix.yml | ||
| with: | ||
| name: "Cxx interop" | ||
| matrix_linux_command: "apt-get update -y -q && apt-get install -y -q jq && curl -s https://raw.githubusercontent.com/apple/swift-nio/main/scripts/check-cxx-interop-compatibility.sh | bash" | ||
| matrix_linux_5_8_enabled: ${{ inputs.linux_5_8_enabled }} | ||
| matrix_linux_5_9_enabled: ${{ inputs.linux_5_9_enabled }} | ||
| matrix_linux_5_10_enabled: ${{ inputs.linux_5_10_enabled }} | ||
| matrix_linux_nightly_6_0_enabled: ${{ inputs.linux_nightly_6_0_enabled }} | ||
| matrix_linux_nightly_main_enabled: ${{ inputs.linux_nightly_main_enabled }} | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,43 +1,36 @@ | ||
| name: Pull Request | ||
|
|
||
| on: | ||
| pull_request: | ||
| types: [opened, reopened, synchronize] | ||
|
|
||
| jobs: | ||
| soundness: | ||
| name: Soundness | ||
| uses: ./.github/workflows/soundness.yml | ||
| with: | ||
| license_header_check_project_name: "SwiftNIO" | ||
|
|
||
| unit-tests: | ||
| name: Checks | ||
| uses: ./.github/workflows/unit_tests.yml | ||
| with: | ||
| linux_nightly_6_0_arguments_override: "--explicit-target-dependency-import-check error" | ||
| linux_nightly_main_arguments_override: "--explicit-target-dependency-import-check error" | ||
|
|
||
| benchmarks: | ||
| name: Checks | ||
| uses: ./.github/workflows/benchmarks.yml | ||
| with: | ||
| benchmark_package_path: "Benchmarks" | ||
|
|
||
| cxx-interop: | ||
| name: Checks | ||
| uses: ./.github/workflows/cxx_interop.yml | ||
|
|
||
| integration-tests: | ||
| name: Checks | ||
|
There was a problem hiding this comment. A bunch of these all share the same There was a problem hiding this comment. That is intentional to get the naming of the checks somewhat aligned. Otherwise it would be |
||
| uses: ./.github/workflows/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" | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| name: Soundness | ||
|
|
||
| on: | ||
| workflow_call: | ||
|
|
||
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| name: Unit tests | ||
|
|
||
| on: | ||
| workflow_call: | ||
| inputs: | ||
| linux_5_8_enabled: | ||
| type: boolean | ||
| description: "Boolean to enable the Linux 5.8 Swift version matrix job. Defaults to true." | ||
| default: true | ||
| linux_5_8_arguments_override: | ||
| type: string | ||
| description: "The arguments passed to swift test in the Linux 5.8 Swift version matrix job." | ||
| default: "-Xswiftc -warnings-as-errors --explicit-target-dependency-import-check error" | ||
| linux_5_9_enabled: | ||
| type: boolean | ||
| description: "Boolean to enable the Linux 5.9 Swift version matrix job. Defaults to true." | ||
| default: true | ||
| linux_5_9_arguments_override: | ||
| type: string | ||
| description: "The arguments passed to swift test in the Linux 5.9 Swift version matrix job." | ||
| default: "-Xswiftc -warnings-as-errors --explicit-target-dependency-import-check error" | ||
| linux_5_10_enabled: | ||
| type: boolean | ||
| description: "Boolean to enable the Linux 5.10 Swift version matrix job. Defaults to true." | ||
| default: true | ||
| linux_5_10_arguments_override: | ||
| type: string | ||
| description: "The arguments passed to swift test in the Linux 5.10 Swift version matrix job." | ||
| default: "-Xswiftc -warnings-as-errors --explicit-target-dependency-import-check error" | ||
| linux_nightly_6_0_enabled: | ||
| type: boolean | ||
| description: "Boolean to enable the Linux nightly 6.0 Swift version matrix job. Defaults to true." | ||
| default: true | ||
| linux_nightly_6_0_arguments_override: | ||
| type: string | ||
| description: "The arguments passed to swift test in the Linux nightly 6.0 Swift version matrix job." | ||
| default: "-Xswiftc -warnings-as-errors --explicit-target-dependency-import-check error" | ||
| linux_nightly_main_enabled: | ||
| type: boolean | ||
| description: "Boolean to enable the Linux nightly main Swift version matrix job. Defaults to true." | ||
| default: true | ||
| linux_nightly_main_arguments_override: | ||
| type: string | ||
| description: "The arguments passed to swift test in the Linux nightly main Swift version matrix job." | ||
| default: "-Xswiftc -warnings-as-errors --explicit-target-dependency-import-check error" | ||
|
|
||
| jobs: | ||
| unit-tests: | ||
| name: Unit tests | ||
| uses: ./.github/workflows/swift_matrix.yml | ||
| with: | ||
| name: "Unit tests" | ||
| matrix_linux_command: "swift test -Xswiftc -warnings-as-errors --explicit-target-dependency-import-check error" | ||
|
There was a problem hiding this comment. 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 There was a problem hiding this comment. 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. There was a problem hiding this comment. 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 There was a problem hiding this comment. You convinced me and I removed the default args now. |
||
| matrix_linux_5_8_enabled: ${{ inputs.linux_5_8_enabled }} | ||
| matrix_linux_5_8_command_override: "swift test ${{ inputs.linux_5_8_arguments_override }}" | ||
| matrix_linux_5_9_enabled: ${{ inputs.linux_5_9_enabled }} | ||
| matrix_linux_5_9_command_override: "swift test ${{ inputs.linux_5_9_arguments_override }}" | ||
| matrix_linux_5_10_enabled: ${{ inputs.linux_5_10_enabled }} | ||
| matrix_linux_5_10_command_override: "swift test ${{ inputs.linux_5_10_arguments_override }}" | ||
| matrix_linux_nightly_6_0_enabled: ${{ inputs.linux_nightly_6_0_enabled }} | ||
| matrix_linux_nightly_6_0_command_override: "swift test ${{ inputs.linux_nightly_6_0_arguments_override }}" | ||
| matrix_linux_nightly_main_enabled: ${{ inputs.linux_nightly_main_enabled }} | ||
| matrix_linux_nightly_main_command_override: "swift test ${{ inputs.linux_nightly_main_arguments_override }}" | ||
Oops, something went wrong.
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.
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.
Nit: Any reason to not spell this as "C++" when we have a string label?
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.
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.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.
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.