feat: PocketIC tests on Windows #4755
Conversation
mraszyk
force-pushed
the
mraszyk/pic-windows-ci
branch
from
fc30c22 to
bf24bd7
Compare
mraszyk
added
the
CI_ALL_BAZEL_TARGETS
mraszyk
changed the title
.github/workflows-source/ci-main.yml
Outdated
| @@ -61,6 +61,14 @@ anchors: | |||
| uses: actions/setup-python@v5 | |||
| with: | |||
| python-version: '3.12' | |||
| pocketic-windows-trigger: &pocketic-windows-trigger | |||
| # Run on protected branches, but only on public repo | |||
| # Allow running if CI_ALL_BAZEL_TARGETS label is used | |||
There was a problem hiding this comment.
May I introduce a new label, e.g., CI_POCKET_IC_WINDOWS?
.github/workflows-source/ci-main.yml
Outdated
| - name: Build the test canister | ||
| run: | | ||
| cargo build --release --locked -p pocket-ic-test-canister --target wasm32-unknown-unknown | ||
| - name: Shrink the test canister | ||
| run: | | ||
| curl -L https://github.com/dfinity/ic-wasm/releases/download/0.9.3/ic-wasm-linux64 -o ic-wasm | ||
| chmod +x ic-wasm | ||
| ./ic-wasm -o test-canister.wasm target/wasm32-unknown-unknown/release/pocket-ic-test-canister.wasm shrink | ||
| shell: wsl-run {0} |
There was a problem hiding this comment.
Alternatively, we could fetch the test canister from bazel-test-all by uploading/downloading its artifact.
There was a problem hiding this comment.
I would prefer that option since then we don't have to update this URL all the time.
.github/workflows-source/ci-main.yml
Outdated
| - name: Compute PocketIC server path | ||
| id: pocket-ic-server-path | ||
| <<: *pocketic-windows-trigger | ||
| run: echo "pocket_ic_server_path=$(bazel cquery //rs/pocket_ic_server:pocket-ic-server --output=files)" >> $GITHUB_OUTPUT | ||
| - name: Upload PocketIC server | ||
| uses: actions/upload-artifact@v4 | ||
| <<: *pocketic-windows-trigger | ||
| with: | ||
| name: pocket-ic-server | ||
| path: ${{ steps.pocket-ic-server-path.outputs.pocket_ic_server_path }} |
There was a problem hiding this comment.
Note that the previous bazel-test-all step might not have built the pocket-ic-server since it only builds those targets whose inputs have changed. Arguably since this step is conditional on CI_ALL_BAZEL_TARGETS (via the pocketic-windows-trigger) it does mean all targets including the pocket-ic-server will be build. But this does feel quite brittle.
There was a problem hiding this comment.
Maybe we could have a separate job bazel-build-pocketic that:
- bazel builds //rs/pocket_ic_server:pocket-ic-server
- bazel builds //packages/pocket-ic/test_canister:test-canister
- uploads both artifacts to S3
and then have the PocketIC on windows job depend on that separate job bazel-build-pocketic instead of bazel-test-all. This should also decrease the CI critical path (currently: bazel-test-all -> pocket-ic-windows; later: bazel-build-pocketic -> pocket-ic-windows and separately bazel-test-all).
There was a problem hiding this comment.
It would be cleaner to have everything in pocket-ic-build.yml and have dedicated CI job for building/testing specifically pocket ic targets. And output artifacts then used in separate CI job that runs on windows (ci job that already exists now but is being extended)?
There was a problem hiding this comment.
So you're suggesting that pocket-ic-build.yml would do what I shared above? In any case, I'd like to ask you if you could please set this up?
.github/workflows-source/ci-main.yml
Outdated
| - name: Build the test canister | ||
| run: | | ||
| cargo build --release --locked -p pocket-ic-test-canister --target wasm32-unknown-unknown |
There was a problem hiding this comment.
Why build and test things with cargo? We should be using bazel. Or is this problematic on Windows?
There was a problem hiding this comment.
This build will be dropped (#4755 (comment)). Should we also move cargo test below to bazel test? (I haven't tried this on Windows yet.)
There was a problem hiding this comment.
There are at least the following issues with bazel on Windows:
- we'd need to make sure that the PocketIC binary built by a separate linux job is used instead of bazel attempting to build the PocketIC binary natively on Windows (which is not gonna work) - using
--test_env=POCKET_IC_BIN=<path>doesn't seem to override the value ofPOCKET_IC_BINin the env section ofBUILD.bazelfor the corresponding test (I tested it out on linux contradicting what ChatGPT is saying: "If a variable is specified by both --test_env and in the env attribute, the command line takes precedence."); - using bazel on Windows resulted in this error:
PS C:\Users\martin\ic> .\bazel.exe test //packages/pocket-ic:test
INFO: Invocation ID: 4c02dcf2-e58a-46c5-872f-9d5d94e0b109
INFO: Repository crate_index instantiated at:
C:/users/martin/ic/WORKSPACE.bazel:142:27: in <toplevel>
C:/users/martin/ic/bazel/external_crates.bzl:159:22: in external_crates_repository
Repository rule crates_repository defined at:
C:/users/martin/_bazel_martin/wzbgb7fn/external/rules_rust/crate_universe/private/crates_repository.bzl:124:36: in <toplevel>
INFO: repository @@crate_index' used the following cache hits instead of downloading the corresponding file.
* Hash '5c62a1c42b71af6c2765dc5f9518eba22ed28276a65f7accf5a2bca0c5cfaf74' for https://github.com/bazelbuild/rules_rust/releases/download/0.56.0/cargo-bazel-x86_64-pc-windows-msvc.exe
If the definition of 'repository @@crate_index' was updated, verify that the hashes were also updated.
ERROR: C:/users/martin/_bazel_martin/wzbgb7fn/external/rules_rust/crate_universe/private/generate_utils.bzl:422:13: An error occurred during the fetch of repository 'crate_index':
Traceback (most recent call last):
File "C:/users/martin/_bazel_martin/wzbgb7fn/external/rules_rust/crate_universe/private/crates_repository.bzl", line 45, column 28, in _crates_repository_impl
repin = determine_repin(
File "C:/users/martin/_bazel_martin/wzbgb7fn/external/rules_rust/crate_universe/private/generate_utils.bzl", line 422, column 13, in determine_repin
fail(msg)
Error in fail: Error: Digests do not match: Current Digest("524ebf9b29523b17ea8de0a2e99be8e72f3b574a1fbf397edf7fb1a49d6c6856") != Expected Digest("5df9b261b764a576639f795e2465f0342d9eb6d279dc5b68b108a2ee92e7afaa")
Stack backtrace:
0: <unknown>
1: <unknown>
2: <unknown>
3: <unknown>
4: <unknown>
5: <unknown>
6: <unknown>
7: <unknown>
8: <unknown>
9: <unknown>
10: <unknown>
11: BaseThreadInitThunk
12: RtlUserThreadStart
The current `lockfile` is out of date for 'crate_index'. Please re-run bazel using `CARGO_BAZEL_REPIN=true` if this is expected and the lockfile should be updated.
ERROR: Error computing the main repository mapping: no such package '@@crate_index//': Error: Digests do not match: Current Digest("524ebf9b29523b17ea8de0a2e99be8e72f3b574a1fbf397edf7fb1a49d6c6856") != Expected Digest("5df9b261b764a576639f795e2465f0342d9eb6d279dc5b68b108a2ee92e7afaa")
Stack backtrace:
0: <unknown>
1: <unknown>
2: <unknown>
3: <unknown>
4: <unknown>
5: <unknown>
6: <unknown>
7: <unknown>
8: <unknown>
9: <unknown>
10: <unknown>
11: BaseThreadInitThunk
12: RtlUserThreadStart
The current `lockfile` is out of date for 'crate_index'. Please re-run bazel using `CARGO_BAZEL_REPIN=true` if this is expected and the lockfile should be updated.
.github/workflows-source/ci-main.yml
Outdated
| - name: Build the test canister | ||
| run: | | ||
| cargo build --release --locked -p pocket-ic-test-canister --target wasm32-unknown-unknown | ||
| - name: Shrink the test canister | ||
| run: | | ||
| curl -L https://github.com/dfinity/ic-wasm/releases/download/0.9.3/ic-wasm-linux64 -o ic-wasm | ||
| chmod +x ic-wasm | ||
| ./ic-wasm -o test-canister.wasm target/wasm32-unknown-unknown/release/pocket-ic-test-canister.wasm shrink | ||
| shell: wsl-run {0} |
There was a problem hiding this comment.
I would prefer that option since then we don't have to update this URL all the time.
.github/workflows-source/ci-main.yml
Outdated
| @@ -200,6 +208,16 @@ jobs: | |||
| BAZEL_TARGETS: //... | |||
| CLOUD_CREDENTIALS_CONTENT: ${{ secrets.CLOUD_CREDENTIALS_CONTENT }} | |||
| GPG_PASSPHRASE: ${{ secrets.GPG_PASSPHRASE }} | |||
| - name: Compute PocketIC server path | |||
There was a problem hiding this comment.
This might not always work if pocket IC was not build. Do you really need to upload it to GH as artifact? On every build? It's not enough to have it in S3 (at least for now)?
There was a problem hiding this comment.
On every build?
It's only uploaded upon pushes to master and if CI_ALL_BAZEL_TARGETS is set.
There was a problem hiding this comment.
It's not enough to have it in S3 (at least for now)?
That's enough in fact. But I'm suggesting to replace CI_ALL_BAZEL_TARGETS by a PocketIC specific label and then we'd need to upload PocketIC to S3 separately anyway.
.github/workflows-source/ci-main.yml
Outdated
| with: | ||
| name: pocket-ic-server | ||
| - name: Setup WSLv2 | ||
| uses: vedantmgoyal9/setup-wsl2@main |
There was a problem hiding this comment.
- The action defaults to Ubuntu, it is a moving target — it could point to different versions over time. but without a version. Better would be to use the:
Ubuntu-24.04as pinned distro, as the default distribution might change. - Using @main or @v1 is risky — the maintainer could push changes that break or introduce vulnerabilities. We can pin the action to a specific commit hash that I have checked
bf0f19100f71267dd1345180b8a678864793e53f, eg:- uses: vedantmgoyal9/setup-wsl2@bf0f19100f71267dd1345180b8a678864793e53f
This PR runs PocketIC tests on Windows as part of CI. After this change, we should perform the following w.r.t. PocketIC on windows (upon every push to master and if a dedicated label is set):