-
Notifications
You must be signed in to change notification settings - Fork 209
fix(test): make //distro:packaging_test work with Bazel 9
#1009
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
base: main
Are you sure you want to change the base?
fix(test): make //distro:packaging_test work with Bazel 9
#1009
Conversation
dff982e to
248aac6
Compare
cgrindel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks reasonable to me. However, I noticed some CI failures.
Bazel 9 flipped `--incompatible_strict_action_env` to `true` by default (bazelbuild/bazel#27670), which means tests no longer inherit `PATH` from the host environment. This breaks subprocess calls that rely on `PATH` lookup: ``` ==================== Test output for //distro:packaging_test: E. ====================================================================== ERROR: testBuild (__main__.PackagingTest.testBuild) ---------------------------------------------------------------------- Traceback (most recent call last): [...] build_result = subprocess.check_output(['bazel', 'build', '--enable_workspace', ':dummy_tar']) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [...] FileNotFoundError: [Errno 2] No such file or directory: 'bazel' ``` This change therefore adds `env_inherit = ["PATH"]` to that very test, which matches the behavior of earlier Bazel versions and is consistent with the presence of a "noci" tag. Additionally, Bazel 9 removed `WORKSPACE` support entirely (bazelbuild/bazel#26131), requiring `bzlmod` with `MODULE.bazel`: ``` ==================== Test output for //distro:packaging_test: [...] WARNING: --enable_bzlmod is set, but no MODULE.bazel file was found at the workspace root. Bazel will create an empty MODULE.bazel file. Please consider migrating your external dependencies from WORKSPACE to MODULE.bazel. For more details, please refer to bazelbuild/bazel#18958. [...] ERROR: error loading package '': Unable to find package for @@[unknown repo 'not_named_rules_pkg' requested from @@]//pkg:tar.bzl: The repository '@@[unknown repo 'not_named_rules_pkg' requested from @@]' could not be resolved: No repository visible as '@not_named_rules_pkg' from main repository. [...] ERROR: Build did NOT complete successfully E. ====================================================================== ERROR: testBuild (__main__.PackagingTest.testBuild) ---------------------------------------------------------------------- [...] build_result = subprocess.check_output(['bazel', 'build'] + bazel_flags + [':dummy_tar']) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [...] subprocess.CalledProcessError: Command '['bazel', 'build', '--enable_workspace', ':dummy_tar']' returned non-zero exit status 1. ``` The test now detects the Bazel version at runtime and generates the appropriate setup: - Bazel 9+: `MODULE.bazel`, with `bazel_dep` and `archive_override`, - earlier versions, as before: `WORKSPACE`, with `http_archive` and `--enable_workspace` flag. For good measure, the change also addresses a leftover TODO by replacing the `tar` subprocess invocation with pure Python code (as otherwise done in the repo).
248aac6 to
385b4ee
Compare
cgrindel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. @aiuto Would you like to take a look?
aiuto
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine, but too much. We'll never build the distribution with anything less than bazel 8.
| with tarfile.open('bazel-bin/dummy_tar.tar.gz', 'r') as tar: | ||
| self.assertEqual(['etc', 'etc/BUILD'], tar.getnames()) | ||
|
|
||
| def _select_bazel_supported_setup(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only have to support building the distro from bazel 8+,
so a module is always available. The workspace test is essentaally dead code to maintain.
Bazel 9 flipped
--incompatible_strict_action_envtotrueby default (bazelbuild/bazel#27670), which means tests no longer inheritPATHfrom the host environment. This breaks subprocess calls that rely onPATHlookup:This change therefore adds
env_inherit = ["PATH"]to that very test, which matches the behavior of earlier Bazel versions and is consistent with the presence of a"noci"tag.Additionally, Bazel 9 removed
WORKSPACEsupport entirely (bazelbuild/bazel#26131), requiringbzlmodwithMODULE.bazel:The test now detects the Bazel version at runtime and generates the appropriate setup:
MODULE.bazel, withbazel_depandarchive_override,WORKSPACE, withhttp_archiveand--enable_workspaceflag.For good measure, the change also addresses a leftover TODO by replacing the
tarsubprocess invocation with pure Python code (as otherwise done in the repo).