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.

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

ci: migrate ci setup from circle ci to github actions #464

Merged
merged 20 commits into from
Jul 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
61 changes: 61 additions & 0 deletions .github/actions/configure-environment/action.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
name: Configure Environment Variables
description: Configure environment variables for Filecoin FFI

runs:
using: 'composite'
steps:
- if: runner.os == 'Linux' && runner.arch == 'ARM64'
run: |
wget -q https://developer.download.nvidia.com/compute/cuda/repos/ubuntu2204/sbsa/cuda-ubuntu2204.pin
sudo mv cuda-ubuntu2204.pin /etc/apt/preferences.d/cuda-repository-pin-600
wget -q https://developer.download.nvidia.com/compute/cuda/12.5.1/local_installers/cuda-repo-ubuntu2204-12-5-local_12.5.1-555.42.06-1_arm64.deb
sudo dpkg -i cuda-repo-ubuntu2204-12-5-local_12.5.1-555.42.06-1_arm64.deb
sudo cp /var/cuda-repo-ubuntu2204-12-5-local/cuda-*-keyring.gpg /usr/share/keyrings/
sudo apt-get update
sudo apt-get -y install cuda-toolkit-12-5
sudo mkdir -p /usr/lib/aarch64-linux-gnu/stubs
Comment on lines +9 to +16
Copy link
Member

Choose a reason for hiding this comment

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

This is all new for GHA, is the arm64 image available to us different enough that there's no cuda available already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're using an almost clean Ubuntu image for Linux arm64 self-hosted runners. We can add installing a version of cuda-toolkit in the machine image to the self-hosted runners backlog if you think it'd be good to have a default available on a machine. One question I have is how much we care about the version of the toolkit.

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to hear @vmx on this; it's his area, cuda and nvidia driver versions for me are just a source of frustration rather than something I know a whole lot about!

Copy link
Contributor

Choose a reason for hiding this comment

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

For development I recommend installing CUDA drivers from the NVIDIA website as the distro ones often don't work well (for whatever reason). Though for CI, I think using the distro packages could work (KISS). If it works for Linux, it should also work for ARM (I hope).

We should support old enough versions and for filecoin-ffi it's more of a sanity check. So whatever the CUDA version is, as long as the tests pass, we are good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it; I'd move on with installing it on linux arm during the run first, and once we add A version of it to the base image, we'll come back here and remove that step.

sudo ln -s /usr/local/cuda-12.5/lib64/stubs/libcuda.so /usr/lib/aarch64-linux-gnu/stubs/libcuda.so.1
sudo ln -s /usr/local/cuda-12.5/lib64/stubs/libcuda.so /usr/lib/aarch64-linux-gnu/stubs/libcuda.so
Comment on lines +17 to +18
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if that's really needed. It looks a bit like a direct port from the CircleCI config. I'd expect the files to be accessible through /usr/local/cuda/lib64/stubs/libcuda.so (not that there's no specific cuda version in the path). And then later below, it should be enough to put the LD_LIBRARY_PATH to usr/local/cuda/lib64/stubs/ (if that's needed at all, perhaps it's already found automatically).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried the suggestion in #472, but it doesn't seem to cut it, unfortunately.

shell: bash
- run: |
echo "FIL_PROOFS_PARAMETER_CACHE=${GITHUB_WORKSPACE}/filecoin-proof-parameters/" >> $GITHUB_ENV
echo 'GO111MODULE=on' >> $GITHUB_ENV
echo 'RUST_LOG=info' >> $GITHUB_ENV
echo "GOPATH=${HOME}/go" >> $GITHUB_ENV
echo "CARGO_TERM_COLOR=never" >> $GITHUB_ENV
shell: bash
- run: |
echo "/usr/local/go/bin" >> $GITHUB_PATH
echo "${GOPATH}/bin" >> $GITHUB_PATH
echo "${HOME}/.cargo/bin" >> $GITHUB_PATH
echo "${HOME}/.bin" >> $GITHUB_PATH
shell: bash
- if: runner.os == 'Linux' && runner.arch == 'ARM64'
run: |
echo "LD_LIBRARY_PATH=/usr/lib/aarch64-linux-gnu/stubs:${LD_LIBRARY_PATH}" >> $GITHUB_ENV
echo "LIBRARY_PATH=/usr/lib/aarch64-linux-gnu/stubs:${LIBRARY_PATH}" >> $GITHUB_ENV
shell: bash
- if: runner.os == 'macOS'
run: |
echo "CPATH=$(brew --prefix)/include" >> $GITHUB_ENV
echo "LIBRARY_PATH=$(brew --prefix)/lib" >> $GITHUB_ENV
shell: bash
- if: runner.os == 'Linux'
run: |
sudo apt-get update
sudo apt-get install --no-install-recommends -y valgrind ocl-icd-opencl-dev libssl-dev libhwloc-dev nvidia-cuda-toolkit g++-10 pkgconf
# Downgrade to GCC 10, as CUDA 11 doesn't play nice with GCC 11
sudo update-alternatives --install /usr/bin/c++ c++ /usr/bin/g++-10 10
sudo update-alternatives --set c++ /usr/bin/g++-10
# Check if we need to install cuda-toolkit-12-5
shell: bash
- if: runner.os == 'macOS'
run: |
HOMEBREW_NO_AUTO_UPDATE=1 brew install pkg-config md5sha1sum jq hwloc
shell: bash
- uses: dtolnay/rust-toolchain@21dc36fb71dd22e3317045c0c31a3f4249868b17
with:
toolchain: 1.73
vmx marked this conversation as resolved.
Show resolved Hide resolved
- uses: actions/setup-go@v5
with:
go-version: '1.21'
213 changes: 213 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,213 @@
name: CI

on:
pull_request:
push:
branches:
- master
tags:
- v*
workflow_dispatch:
inputs:
save:
description: 'Save Filecoin parameters'
required: false
default: 'false'
publish:
description: 'Publish the static library'
required: false
default: 'false'
run-leak-detector:
description: 'Run the CGO leak detector'
required: false
default: 'false'
ref:
description: 'The ref to build'
required: false

defaults:
run:
shell: bash

concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: ${{ github.event_name == 'pull_request' }}

permissions:
contents: read

# Can we apply these to the entire workflow?
env:
# Build the kernel only for the single architecture. This should reduce
# the overall compile-time significantly.
EC_GPU_CUDA_NVCC_ARGS: --fatbin --gpu-architecture=sm_75 --generate-code=arch=compute_75,code=sm_75
BELLMAN_CUDA_NVCC_ARGS: --fatbin --gpu-architecture=sm_75 --generate-code=arch=compute_75,code=sm_75
NEPTUNE_CUDA_NVCC_ARGS: --fatbin --gpu-architecture=sm_75 --generate-code=arch=compute_75,code=sm_75
DEBIAN_FRONTEND: noninteractive

jobs:
check:
name: Check code style and run linters
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
with:
submodules: recursive
- uses: ./.github/actions/configure-environment
- if: github.event.inputs.ref != ''
uses: actions/checkout@v4
with:
submodules: recursive
ref: ${{ github.event.inputs.ref }}
- name: Run shellcheck
run: shellcheck ./install-filcrypto
- name: Run cargo fmt
run: |
rustup component add rustfmt
cargo fmt --manifest-path ./rust/Cargo.toml --all -- --check
- name: Run cargo clippy
run: cd rust && cargo clippy --all-targets --features blst-portable,opencl -- -D warnings
- name: Run go fmt
# `! go fmt ./... 2>&1 | read"` doesn't work, this one does, thanks
# https://carsonip.me/posts/go-fmt-and-ci/
run: |
output=$(go fmt ./...)
echo "${output}"
test -z "${output}"
- name: Run various linters
run: |
go install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.59.0
make go-lint
cgo-bindings:
name: Build and test CGO bindings (${{ matrix.runner }})
runs-on: ${{ matrix.runner }}
strategy:
matrix:
runner: ['ubuntu-latest', ["self-hosted", "linux", "arm64", "xlarge"], 'macos-latest']
fail-fast: false
steps:
- run: echo "Running on $RUNNER_OS $RUNNER_ARCH"
- uses: actions/checkout@v4
with:
submodules: recursive
- uses: ./.github/actions/configure-environment
- if: github.event.inputs.ref != ''
uses: actions/checkout@v4
with:
submodules: recursive
ref: ${{ github.event.inputs.ref }}
- if: runner.os == 'macOS'
run: cd rust && cargo fetch
- name: Build project
run: make
- name: Build project without CGO
run: env CGO_ENABLED=0 go build .
- if: runner.os == 'Linux'
uses: actions/cache/restore@v3
with:
key: v28-proof-params-${{ runner.os }}-${{ runner.arch }}
path: |
./filecoin-proof-parameters
- if: runner.os == 'Linux'
name: Obtain Filecoin parameters
run: |
DIR=$(pwd)
cd $(mktemp -d)
go install github.com/filecoin-project/go-paramfetch/paramfetch@latest
$GOPATH/bin/paramfetch 2048 "${DIR}/parameters.json" "${DIR}/srs-inner-product.json"
- if: runner.os == 'Linux' && ((github.event == 'push' && !startsWith(github.ref, 'refs/tags/')) || github.event.inputs.save == 'true')
uses: actions/cache/save@v3
with:
key: v28-proof-params-${{ runner.os }}-${{ runner.arch }}
path: |
./filecoin-proof-parameters
- if: runner.os == 'Linux'
run: cd rust && rustup target add wasm32-unknown-unknown
- if: github.event.inputs.run-leak-detector == 'true'
run: make cgo-leakdetect
- if: runner.os == 'Linux'
run: cd rust && FIL_PROOFS_PARAMETER_CACHE="${GITHUB_WORKSPACE}/filecoin-proof-parameters/" RUST_LOG=info cargo test --all --release -- --test-threads 1 && cd ..
- if: runner.os == 'Linux'
run: GOEXPERIMENT=cgocheck2 RUST_LOG=info go test -p 1 -timeout 60m
- if: runner.os == 'macOS'
name: Build project and tests, but don't actually run the tests (used to verify that build/link works with Darwin)
run: GOEXPERIMENT=cgocheck2 RUST_LOG=info go test -run=^$
supraseal:
name: Build with CUDA supraseal
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
with:
submodules: recursive
- uses: ./.github/actions/configure-environment
- if: github.event.inputs.ref != ''
uses: actions/checkout@v4
with:
submodules: recursive
ref: ${{ github.event.inputs.ref }}
- name: Build project with `FFI_USE_CUDA_SUPRASEAL=1`
run: FFI_BUILD_FROM_SOURCE=1 FFI_USE_CUDA_SUPRASEAL=1 make
publish:
needs: [check, cgo-bindings, supraseal]
if: (github.event_name == 'push' && startsWith(github.ref, 'refs/tags/')) || github.event.inputs.publish == 'true'
rvagg marked this conversation as resolved.
Show resolved Hide resolved
name: Publish the static library (${{ matrix.runner }})
runs-on: ${{ matrix.runner }}
strategy:
matrix:
runner: ['ubuntu-latest', ['self-hosted', 'linux', 'arm64', 'xlarge'], 'macos-latest']
fail-fast: false
steps:
- run: echo "Running on $RUNNER_OS $RUNNER_ARCH"
- uses: actions/checkout@v4
with:
submodules: recursive
- uses: ./.github/actions/configure-environment
- if: github.event.inputs.ref != ''
uses: actions/checkout@v4
with:
submodules: recursive
ref: ${{ github.event.inputs.ref }}
- if: runner.os == 'macOS'
run: |
cd rust && rustup target add x86_64-apple-darwin
cd rust && cargo fetch
- if: runner.os == 'Linux'
name: Build and publish the standard release
run: |
cd rust

REPOSITORY_NAME=${GITHUB_REPOSITORY##*/}

TARBALL_PATH="/tmp/${REPOSITORY_NAME}-$(uname)-$(uname -m)-standard.tar.gz"
RELEASE_NAME="${REPOSITORY_NAME}-$(uname)-$(uname -m)-standard"

# Note: the blst dependency uses the portable configuration for maximum compatibility
./scripts/build-release.sh build --verbose --no-default-features --features multicore-sdr,opencl,blst-portable
./scripts/package-release.sh $TARBALL_PATH
./scripts/publish-release.sh $TARBALL_PATH $RELEASE_NAME
- if: runner.os == 'Linux'
name: Build the optimized release
run: |
cd rust

REPOSITORY_NAME=${GITHUB_REPOSITORY##*/}

TARBALL_PATH="/tmp/${CIRCLE_PROJECT_REPONAME}-$(uname)-$(uname -m)-optimized.tar.gz"
RUSTFLAGS="-C target-feature=$(cat rustc-target-features-optimized.json | jq -r '.[].rustc_target_feature' | tr '\n' ',')"

./scripts/build-release.sh build --verbose --no-default-features --features multicore-sdr,opencl
./scripts/package-release.sh $TARBALL_PATH
- if: runner.os == 'macOS'
name: Build and publish the universal standard release
run: |
cd rust

REPOSITORY_NAME=${GITHUB_REPOSITORY##*/}

RELEASE_NAME="${CIRCLE_PROJECT_REPONAME}-$(uname)-standard"
TARBALL_PATH="/tmp/${RELEASE_NAME}.tar.gz"

# Note: the blst dependency uses the portable configuration for maximum compatibility
./scripts/build-release.sh lipo --verbose --no-default-features --features multicore-sdr,opencl,blst-portable
./scripts/package-release.sh $TARBALL_PATH
./scripts/publish-release.sh $TARBALL_PATH $RELEASE_NAME
2 changes: 0 additions & 2 deletions bls_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,12 +121,10 @@ func BenchmarkBLSVerifyBatch(b *testing.B) {
func benchmarkBLSVerifyBatchSize(size int) func(b *testing.B) {
return func(b *testing.B) {
var digests []Digest
var msgs []Message
var sigs []Signature
var pubks []PublicKey
for i := 0; i < size; i++ {
msg := Message(fmt.Sprintf("cats cats cats cats %d %d %d dogs", i, i, i))
msgs = append(msgs, msg)
Copy link

@laurentsenta laurentsenta Jul 9, 2024

Choose a reason for hiding this comment

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

We're touching a test, is a typo or expected? I guess that breaks some linting rules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the linting rules started failing. The issue made sense to me so I fixed it. I suspect we're using newer version of some linting tool or something like that.

Copy link
Member

Choose a reason for hiding this comment

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

https://hub.docker.com/layers/cimg/go/1.21.2-node/images/sha256-59b3fff1c4745e9195ea35eef2cfbee4cb0b562616869e304b581644020ad4b8 - golangci-lint v1.54.2 vs v1.56.2

but, I don't see any version of golangci-lint failing on this file. staticcheck does but I don't see it being used here?

Copy link
Member

Choose a reason for hiding this comment

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

btw I'm not objecting to this fix, just interested in why this is showing up for this diff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the failing run before I added this change - https://github.com/filecoin-project/filecoin-ffi/actions/runs/9805936223/job/27076653921

It does come from staticcheck as you pointed out. I wonder why we weren't running it in CircleCI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is listed in the linters list there, too. Honestly, I'm not sure why it wasn't flagging this issue.

Copy link
Member

@rvagg rvagg Jul 12, 2024

Choose a reason for hiding this comment

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

make go-lint only runs golangci-lint. So how about we add staticcheck install to this workflow and to the Makefile too so we have it properly covered

Needs investigation, golangci-lint is invoking staticcheck, I wonder if we need it installed for that to happen or if there's some config we're missing. Maybe we should just import the lotus config and be done with it. I'll try and get to figuring this out unless someone has a chance before me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is running in GHA - that's why I had to add the fix - so I wonder how much time we want to spend investigating this.

Copy link
Member

Choose a reason for hiding this comment

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

apparently golangci-lint has "staticcheck" but not quite the same as "staticcheck", so this is even more confusing ..

it's a good fix, so not a big deal, it would just be god to understand this and maybe fix our linting to do a better job

not a blocker for this PR

digests = append(digests, Hash(msg))
priv := PrivateKeyGenerate()
sig := PrivateKeySign(priv, msg)
Expand Down
Loading