Skip to content
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.

Sign up for GitHub

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

Bump GitHub Actions #266

Merged
merged 4 commits into from
Jun 11, 2024
Merged

Bump GitHub Actions #266

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
14122d4
Bump GitHub Actions
mjameswh Jun 5, 2024
90744c8
Can use download-artifact@v3-node20 in subsequent job
mjameswh Jun 5, 2024
1f1bc95
Merge branch 'main' into bump-gha-artifact-v2
mjameswh Jun 7, 2024
76e9e0f
Merge branch 'main' into bump-gha-artifact-v2
mjameswh Jun 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,12 @@ jobs:
runs-on: ${{ matrix.runsOn || matrix.os }}
steps:
- name: Checkout repository
uses: actions/checkout@v2
uses: actions/checkout@v4
with:
submodules: recursive

- name: Install Rust
uses: actions-rs/toolchain@v1
with:
toolchain: stable
uses: dtolnay/rust-toolchain@stable

- name: Setup Rust cache
uses: Swatinem/rust-cache@v2
Expand Down Expand Up @@ -80,7 +78,7 @@ jobs:

- name: Upload test failure
if: ${{ failure() }}
uses: actions/upload-artifact@v2
uses: actions/upload-artifact@v4
with:
name: test-fail-${{ matrix.os }}
path: tests/Temporalio.Tests/TestResults
Expand Down
42 changes: 22 additions & 20 deletions .github/workflows/package.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,43 +38,44 @@ jobs:
out-prefix: win-x64
runs-on: ${{ matrix.runsOn || matrix.os }}
container: ${{ matrix.container }}
env:
# This is required to allow continuing usage of Node 16 for actions,
# as Node 20 won't run on the docker image we use for linux builds
# (Node 20 require glibc 2.28+, but container image has glibc 2.17).
# https://github.blog/changelog/2024-05-17-updated-dates-for-actions-runner-using-node20-instead-of-node16-by-default/
ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which action still needs node 16 here? I wonder if it'd be best to comment above those too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are FIXME comments on each actions in that job that are postponed from updating to Node 20.

      - name: Upload bridge library
        # FIXME: v4+ requires Node 20
        uses: actions/upload-artifact@v3
        with:
          name: ${{ matrix.out-prefix }}-bridge

The reason I made these FIXMEs is that this is only a temporary measure. GHA will completely pull the plug on Node 16 (no longer "soft deprecated") in a few months (exact date to be announced early October). By then, we'll have to either drop support for glibc releases older than 2.28 for all our SDKs, or figure out another way to work around this.

Copy link
Member

@cretz cretz Jun 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My question is why not update to node 20 now and update those action versions now? Which steps do not have a node-20 equivalent that is keeping us from upgrading? If it's just a glibc concern, don't worry there. We build our Linux binaries in a certain container w/ older glibc (or is this the problem?).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's just a glibc concern, don't worry there. We build our Linux binaries in a certain container w/ older glibc (or is this the problem?).

Ah… Yes, that's specifically the problem. Most GHA Actions are written in JS. When running a job in a container, actions also run in that container. At this time, GHA maps a volume in the container containing Node 16 and Node 20, all the actions git repos (I mean all actions you use in your workflow), and a few other things, hence that works even if your container image doesn't have Node 16/20. But starting on June 30th, they will no longer include the Node 16 binary on the linked volume, unless you set that environment variable.

So the problem is that the Node 20 binary they bring into your container itself requires glibc 2.28. That's a questionable decision of their part, given that they could have use a Node 20 statically linked against musl, but they didn't, and FWIW, let's just assume they had good reasons for doing it the way they did.

Anyway, that's where we get these conflicting requirements. On one side, we want a runner or container image that has glibc 2.17, so that are builds artifacts are compatible with older distros, but on the other side, it must have at least glibc 2.28 to be able to run Node 20, which is required for most GHA actions…

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, so that's why you made sure to only do this in the package workflow and not the CI one. I wonder how long ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION will work for, heh. I also think they "good reasons" they have are probably hinted in the env var name, they don't consider older glibc secure.

In the future we can probably look to isolate just the building part to the container instead of the whole workflow (we lose Rust caching, but this is a packaging CI that probably doesn't need it much and I should probably turn it off on push to main and make it manual dispatch only). This looks good for now, approving/merging...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A possible alternative would be to use an up-to-date linux runner and no container at the job level, but use a run step to execute the actual build step (and only that step) inside a docker container (i.e. we'd have a step like - run: docker run .... build-native-code-for-linux, or wrap that in a shell script).

That would involve a distinct build path for the Linux production-grade native build artifacts vs other platforms. Can't use GHA actions to install required tools in that case. Caching actions would still be usable, but may require some special handling (they would run outside of the container, so the cached directory might need to be volume-mapped inside the container if they are not already contained in the working directory).

That's essentially what happens for the Python build. I'm not familiar with the details of Python packaging, but essentially the native build is done against the manylinux image, but it is some python-specific build tool that starts the docker container, not the GHA workflow file (i.e. you don't see a container: 'manylinux-2014' directive in the workflow file). But then, that build tool is also responsible for the build process on other platforms, so you don't get the downside of having to support two distinct build processes.

Really, it shouldn't be difficult to create that setup. But that would add extra maintenance that isn't required at this point (probably some custom docker images to maintain, e.g. based on manylinux, but install extra deps). And by the time that GHA actually pull the plug on Node 16 (they will announce the exact date in early October, which would presumably be a few months after that), it's very possible that the "glibc 2.28 minimal requirement" might feel more reasonable (AFAIK, all major distros releases that still predates glibc 2.28 will have reached TLS EOL). If it turns to be so, then we won't need to make things more complicated than they need to be. Otherwise, we'll bite the bullet and implement that alternative build process.

steps:
- name: Checkout repository
uses: actions/checkout@v2
# FIXME: v4+ requires Node 20
uses: actions/checkout@v3
with:
submodules: recursive

- name: Install Rust
uses: actions-rs/toolchain@v1
uses: dtolnay/rust-toolchain@stable
with:
toolchain: stable

- name: Setup Rust cache
# FIXME: v2.7.2+ requires Node 20
# Fixed version due to https://github.com/Swatinem/rust-cache/issues/183#issuecomment-1893979126
uses: Swatinem/rust-cache@v2.7.0
uses: Swatinem/rust-cache@v2.7.1
with:
workspaces: src/Temporalio/Bridge
key: ${{ matrix.os }}

- name: Install protoc (non-Linux)
if: ${{ matrix.os != 'ubuntu-latest' && matrix.os != 'ubuntu-arm' }}
uses: arduino/setup-protoc@v3
- name: Install protoc
# FIXME: v3+ requires Node 20
uses: arduino/setup-protoc@v2
with:
version: "23.x"
repo-token: ${{ secrets.GITHUB_TOKEN }}

- name: Install protoc (Linux)
if: ${{ matrix.os == 'ubuntu-latest' || matrix.os == 'ubuntu-arm' }}
run: |
curl --location -o protobuf-compiler.zip ${{ matrix.protobuf-url }}
mkdir protobuf-compiler
unzip protobuf-compiler.zip -d protobuf-compiler
echo $(realpath .)/protobuf-compiler/bin >> $GITHUB_PATH

- name: Build
run: cargo build --manifest-path src/Temporalio/Bridge/Cargo.toml --release

- name: Upload bridge library
# FIXME: v4+ requires Node 20
uses: actions/upload-artifact@v3
with:
name: ${{ matrix.out-prefix }}-bridge
Expand All @@ -86,12 +87,13 @@ jobs:
runs-on: windows-latest
steps:
- name: Checkout repository
uses: actions/checkout@v2
uses: actions/checkout@v4
with:
submodules: recursive

- name: Download bridge libraries
uses: actions/download-artifact@v3
# Need v3 here to stay compatible with the compile-native-binaries job.
uses: actions/download-artifact@v3-node20
with:
path: bridge-libraries

Expand All @@ -102,7 +104,7 @@ jobs:
run: dotnet pack -c Release /p:BridgeLibraryRoot=${{ github.workspace }}/bridge-libraries

- name: Upload NuGet artifact
uses: actions/upload-artifact@v3
uses: actions/upload-artifact@v4
with:
name: nuget-package
path: |
Expand Down Expand Up @@ -132,12 +134,12 @@ jobs:
runs-on: ${{ matrix.runsOn || matrix.os }}
steps:
- name: Checkout repository
uses: actions/checkout@v2
uses: actions/checkout@v4
with:
submodules: recursive

- name: Download NuGet artifact
uses: actions/download-artifact@v3
uses: actions/download-artifact@v4
with:
name: nuget-package
path: nuget-package
Expand All @@ -156,7 +158,7 @@ jobs:

- name: Setup msbuild (Windows only)
if: ${{ matrix.os == 'windows-latest' }}
uses: microsoft/setup-msbuild@v1.1
uses: microsoft/setup-msbuild@v2

- name: Run .NET framework smoke test (Windows only)
if: ${{ matrix.os == 'windows-latest' }}
Expand Down
15 changes: 6 additions & 9 deletions .github/workflows/run-bench.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,28 +8,25 @@ jobs:
runs-on: buildjet-4vcpu-ubuntu-2204
steps:
- name: Checkout repository
uses: actions/checkout@v2
uses: actions/checkout@v4
with:
submodules: recursive

- name: Install Rust
uses: actions-rs/toolchain@v1
with:
toolchain: stable
uses: dtolnay/rust-toolchain@stable

- name: Setup Rust cache
uses: Swatinem/rust-cache@v2
with:
workspaces: src/Temporalio/Bridge

- name: Setup .NET
uses: actions/setup-dotnet@v3
uses: actions/setup-dotnet@v4

- name: Install protoc
uses: arduino/setup-protoc@v1
with:
# TODO(cretz): Upgrade when https://github.com/arduino/setup-protoc/issues/33 fixed
version: '3.x'
uses: arduino/setup-protoc@v3
# TODO: Upgrade proto once https://github.com/arduino/setup-protoc/issues/99 is fixed
version: '23.x'
repo-token: ${{ secrets.GITHUB_TOKEN }}

- name: Build
Expand Down