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

Add linux-asan and {linux,macOS,windows}-debug to the github test workflow #1283

Merged
merged 3 commits into from
Oct 23, 2023

Conversation

ohodson
Copy link
Contributor

@ohodson ohodson commented Oct 6, 2023

Three commits here:

  1. Adds new tests to the github test workflow:

    • debug builds on Linux, macOS, and Windows
    • linux-asan

    Initially, the new test configurations are advisory and do not block PR submission.

    For ASAN and debug builds, benchmark builds are skipped to free up disk space. The runners have a nominal 14GB of storage consumed by build files, the build cache, and the tarball of build cache when it is saved.

    With more build configurations there is more pressure on the bazel cache. This PR avoids caching files larger than 100MB as the nominal limit is 10GB across all the workerd build combinations.

  2. Fixes a warning on macOS (workspace-status.sh using non-existent realpath). Variable was unused so just deleted.

  3. Configures git hooks to quell warnings from workspace-status.sh on every invocation of bazel.

@ohodson ohodson force-pushed the orion/matrix-test2 branch 22 times, most recently from 87e42d5 to 05e05ad Compare October 8, 2023 23:49
@ohodson ohodson changed the title Quick check with distinct compilation modes. Add linux-asan and {linux,macOS,windows}-debug to the github test workflow Oct 8, 2023
@ohodson ohodson force-pushed the orion/matrix-test2 branch 5 times, most recently from 75f4b9b to 67ba538 Compare October 9, 2023 01:19
.bazelrc Outdated
@@ -194,3 +202,15 @@ build:windows --per_file_copt='external/v8/src/objects/swiss-name-dictionary\.cc
# enable clang coverage: https://clang.llvm.org/docs/SourceBasedCodeCoverage.html
build:clang-coverage --copt="-fprofile-instr-generate" --linkopt="-fprofile-instr-generate"
build:clang-coverage --copt="-fcoverage-mapping" --linkopt="-fcoverage-mapping"

# Debug
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually debug builds are less intense than ASAN.

Copy link
Contributor Author

@ohodson ohodson Oct 9, 2023

Choose a reason for hiding this comment

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

Well they both fill the disk of the linux builder, hence the change here. I suspect there is some trimming we can do (or Felix has a handle on already) to avoid this longer term. Main priority whilst this is in draft is just getting the builds working. llvn-symbolizer on macOS does not seem very happy, this might relate to bazelbuild/bazel#6327 which we have a workaround for in VSCode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

macOS failure is a timeout: //src/workerd/api:sql-test after ~300s, test completes in 25s on an uncontested M1.

Windows build failure does not repro on Windows dev machine. It looks like it's falling fowl of the hackery that was needed for LLVM16 and is shortly going away.

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 half suspect the macOS issue is due to invoking llvm-symbolizer and it doing a crazy slow symbol search just based on watching top whilst bazel runs the test (detached from the terminal).

.bazelrc Outdated
@@ -80,6 +80,14 @@ build:sanitizer-common --copt="-fno-omit-frame-pointer" --copt="-mno-omit-leaf-f
build:asan --config=sanitizer-common
build:asan --copt="-fsanitize=address" --linkopt="-fsanitize=address"
build:asan --test_env=ASAN_OPTIONS=abort_on_error=true
# Exclude benchmarks from asan build due to large size (exceed github runner limits)
build:asan -- -//src/workerd/tests:bench-api-headers
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably want to create the following

build:limit-storage -- -//src/workerd/tests:bench-api-headers
build:limit-storage -//src/workerd/tests:bench-global-scope
build:limit-storage -//src/workerd/tests:bench-json
build:limit-storage -//src/workerd/tests:bench-kj-headers
build:limit-storage -//src/workerd/tests:bench-mimetype
build:limit-storage -//src/workerd/tests:bench-regex
build:limit-storage -//src/workerd/tests:bench-tools

build:asan --config=limit-storage
build:debug --config=limit-storage

Copy link
Contributor Author

@ohodson ohodson Oct 13, 2023

Choose a reason for hiding this comment

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

Probably want to create the following

build:limit-storage -- -//src/workerd/tests:bench-api-headers ...

Thanks Sam, moved this config into test.yml so it only affects builds on github ci. 👍

@ohodson ohodson marked this pull request as ready for review October 13, 2023 16:07
@ohodson ohodson requested a review from fhanau October 13, 2023 16:35
@ohodson
Copy link
Contributor Author

ohodson commented Oct 13, 2023

There is a failure here in Windows debug, but I believe that is pre-existing, ie have reproed at home, but not yet had time to investigate. @jasnell might be able to pinpoint this faster.

.bazelrc Show resolved Hide resolved
@ohodson ohodson force-pushed the orion/matrix-test2 branch 4 times, most recently from 00c45b2 to 81f626d Compare October 23, 2023 11:18
@ohodson
Copy link
Contributor Author

ohodson commented Oct 23, 2023

PTAL, I've decided not to try and solve the windows-dbg failure in this PR. Looking into this separately. Thanks.

.github/workflows/test.yml Outdated Show resolved Hide resolved
@fhanau
Copy link
Collaborator

fhanau commented Oct 23, 2023

One potential concern: I vaguely remember Github preventing merges if there are any builds still running, even if those builds are not required builds. I'm not sure if I got that right/if it applies here but that would effectively require debug/asan builds to finish running before PRs can be merged. We can see if that's actually happening/how to address it after merging this though.

@ohodson
Copy link
Contributor Author

ohodson commented Oct 23, 2023

One potential concern: I vaguely remember Github preventing merges if there are any builds still running, even if those builds are not required builds. I'm not sure if I got that right/if it applies here but that would effectively require debug/asan builds to finish running before PRs can be merged. We can see if that's actually happening/how to address it after merging this though.

We have the existing configs setup to be required, but not the new ones. Hopefully, this doesn't affect the time-to-merge. For my money, time-to-revert needs to be very fast, whereas time-to-merge just needs to be prompt. An alternative would be to have post-merge builders, but that's more infra to setup and maintain. Let's see how we go.

@ohodson
Copy link
Contributor Author

ohodson commented Oct 23, 2023

PS Love that GitHub includes an advert for larger runners with this PR, particularly apt 💸 ☁️

Screenshot 2023-10-23 at 19 33 40

Adds debug and address-sanitizer build configurations to the test
workflow. At the time of writing these are advisory.

Temporarily drop benchmarks from asan and debug builds for github
runner limits.
Variable was unused and initialized with the output for realpath
which appears not to exist on GitHub macOS runners.
On runners git hooks aren't used, but we now configure them anyway to
avoid logspam on each bazel invocation since workspace-status.sh checks
whether githooks are in place.
@ohodson ohodson merged commit 39ff173 into main Oct 23, 2023
10 checks passed
@ohodson ohodson deleted the orion/matrix-test2 branch October 23, 2023 20:10
ohodson added a commit that referenced this pull request Oct 24, 2023
Accidentally dropped in a pre-merge rebase of #1283.
fhanau pushed a commit that referenced this pull request Oct 24, 2023
Accidentally dropped in a pre-merge rebase of #1283.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants