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

feat: support --noenable_runfiles on linux and windows, new runfiles bat macro, fix tests for windows #872

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

peakschris
Copy link

This PR contains an update of the runfiles code in windows_utils.bzl to match the runfiles v3 standard (compatible with latest bzlmod). I've also taken the opportunity to fix many tests to work under windows, both with --enable_runfiles and --noenable_runfiles, which required using the new runfiles code to correctly resolve data.

I suspect that the existing tests on linux don't/can't currently validate --noenable_runfiles behaviour of the underlying rules.

Whilst under linux bazel test always enables runfiles (ignoring the --noenable_runfiles flag bazelbuild/bazel#22856), under windows this flag does have effect, and is the default, so getting tests clean on windows require test cases to handle runfiles manifests. Also, this would seem to be the only way that we can validate that the rules we are testing work correctly without runfiles.

I have added a test runner script that runs each test case with bazel run, simulating windows behaviour with --noenable_runfiles. This script has some caveats, namely that it writes test outputs to source tree. See lib/tests/README.md for more.

The only windows failures are now in directories with missing windows packages: lib/tests/tar,lib/tests/assert_archive_contains,lib/tests/zstd. Missing packages are unzip, bsdtar, zstd. Additionally, 3 bats tests timeout on windows, there seems to be some kind of concurrency issue. With tags=["exclusive"], they pass occasionally but most often still timeout.

@meteorcloudy @fmeum


Changes are visible to end-users: yes

  • Searched for relevant documentation and updated as needed: no
  • Breaking change (forces users to change their own code or config): no
  • Suggested release notes appear below: yes

Suggested release notes

  • Some aspect rules now support --noenable_runfiles on linux and windows (the default on windows)

Test plan

  • It would be good to get some CI runners enabled to ensure windows does not regress
  • the test.sh script could be called on all runners to test all combinations of test/run and enable/noenable runfiles

Test results: linux - before

[test enable runfiles  ] Executed 179 out of 187 tests: 179 tests pass and 8 were skipped.
[test noenable runfiles] Executed 179 out of 187 tests: 179 tests pass and 8 were skipped.
[run enable runfiles   ] Executed 179 out of 187 tests: 179 tests pass and 8 were skipped.
Updated 6 paths from the index
[run noenable runfiles ] Executed 179 out of 187 tests: 150 tests pass, 29 fail and 8 were skipped.
Updated 0 paths from the index

Test results: linux - after

[test enable runfiles  ] Executed 179 out of 188 tests: 179 tests pass and 9 were skipped.
[test noenable runfiles] Executed 175 out of 188 tests: 175 tests pass and 13 were skipped.
[run enable runfiles   ] Executed 179 out of 188 tests: 179 tests pass and 9 were skipped.
Updated 6 paths from the index
[run noenable runfiles ] Executed 175 out of 188 tests: 175 tests pass and 13 were skipped.
Updated 6 paths from the index

Note: an extra 4 are skipped as I have marked them as incompatible with noenable_runfiles

Test results: windows - before

[test enable runfiles  ] Executed 158 out of 181 tests: 156 tests pass, 9 fail to build, 2 fail locally and 14 were skipped.
[test noenable runfiles] Executed 158 out of 181 tests: 133 tests pass, 9 fail to build, 25 fail locally and 14 were skipped.
[run enable runfiles   ] Executed 167 out of 181 tests: 156 tests pass, 11 fail and 14 were skipped.
Updated 6 paths from the index
[run noenable runfiles ] Executed 167 out of 181 tests: 133 tests pass, 34 fail and 14 were skipped.
Updated 0 paths from the index

Baseline failures (test enable runfiles):

  • I have run with --deleted_packages=lib/tests/bats as these are hanging on windows, this removes 6 tests
  • 9 build failures: missing tar
  • 1 analysis failure: zstd
  • 1 failure: transitions 1 failure: zip

Test results: windows - after

[test enable runfiles  ] Executed 158 out of 182 tests: 156 tests pass, 9 fail to build, 2 fail locally and 15 were skipped.
[test noenable runfiles] Executed 156 out of 182 tests: 154 tests pass, 9 fail to build, 2 fail locally and 17 were skipped.
[run enable runfiles   ] Executed 167 out of 182 tests: 156 tests pass, 11 fail and 15 were skipped.
Updated 6 paths from the index
[run noenable runfiles ] Executed 165 out of 182 tests: 154 tests pass, 11 fail and 17 were skipped.
Updated 6 paths from the index

@peakschris peakschris changed the title Support --noenable_runfiles on linux and windows, new runfiles bat macro, fix tests for windows feat: support --noenable_runfiles on linux and windows, new runfiles bat macro, fix tests for windows Jun 22, 2024
@peakschris
Copy link
Author

@alexeagle could I get your thoughts on this? My rules_multirun pr will depend on this.

@peakschris
Copy link
Author

Alex wrote (keith/rules_multirun#43)

Sorry there's no time! Hair on fire! but I really appreciate your work on that and making Bazel better on Windows. Please keep pushing on us :)

No worries Alex!

@fmeum in the meantime, could you review windows_utils.bzl?
@gregmagolan could you review this if you have any cycles?
@meteorcloudy windows_utils.bzl might interest you

@alexeagle
Copy link
Collaborator

It's really hard to get enough focus time from any of the folks you @-mentioned here to review something that's larger than bite-sized. I don't want to lose your improvements though! And I've been working in Windows the last couple days.

Could you take a bit of time to carve out individual units from this which could land as standalone PRs? At least that way the easy ones get merged.

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.

2 participants