fix(toolchain): fix crash on Windows when precompiling is enabled (#4082)
`python/private/py_executable.bzl`'s `_maybe_add_test_main_validation`
fix (#4079) noted `precompile.bzl` as a remaining user of the same
`exec_interpreter` relocation issue, needing the same migration.
There was no existing test exercising `_precompile`'s action at all:
`tests/base_rules/precompile`'s suite is `analysis_test`-only, checking
declared providers, never actually running the precompiler.
Reproducing this on Windows therefore required a real `bazel build`, via
the new `test_precompile_enabled_succeeds`:
```
bazel test \
//tests/base_rules/precompile:test_precompile_enabled_succeeds
...
ERROR: .../tests/base_rules/precompile/BUILD.bazel:3:22: Python
precompiling .../test_precompile_enabled_succeeds_main.py into
.../test_precompile_enabled_succeeds_main.cpython-311.pyc
failed: Worker process did not return a WorkResponse:
---8<---8<--- Start of log, file at
.../multiplex-worker-1-PyCompile.log ---8<---8<---
(empty)
---8<---8<--- End of log ---8<---8<---
```
The worker crashes at startup, unable to find its DLLs, before it can
write anything to its own log or respond over the worker protocol.
`_precompile` now uses `actions_run()` with `exec_runtime`, exactly as
`_maybe_add_test_main_validation` does, instead of
`exec_tools_info.exec_interpreter[DefaultInfo].files_to_run`.
Reproducing and fixing this also uncovered two more problems, both
specific to the precompiler's worker mode and unrelated to
`exec_interpreter`.
First, `tools/precompiler/precompiler.py`'s persistent worker reads each
JSON request as a single line via `asyncio.StreamReader`, whose default
64KiB limit is exceeded once every interpreter distribution file,
previously hidden by relocation into a much smaller symlink tree, shows
up as an actual, individually-digested action input:
```
ValueError: Separator is not found, and chunk exceed the limit
```
A CPython 3.11 distribution's ~2,260 inputs measure ~470KiB this way; `1
<< 22` (4MiB) leaves ample headroom.
Second, the worker's default implementation, `_AsyncPersistentWorker`,
can't start on Windows at all: `asyncio`'s `ProactorEventLoop` fails to
wrap `stdin`/`stdout` as pipe transports, with:
```
OSError: [WinError 6] The handle is invalid
```
Bazel gives workers anonymous pipes (`CreatePipe`) for stdio, which
never support overlapped I/O, so `asyncio`'s `ProactorEventLoop` can't
register them with an I/O completion port.
This is unrelated to precompiling's relocation bug: nothing exercises
this worker on Windows today.
`_SerialPersistentWorker`, the blocking-I/O alternative already present
in the file, has no such issue, so `--worker_impl` now defaults to
`serial` on Windows.
`tests/base_rules/precompile:test_precompile_enabled_succeeds` is a
real, executing `py_test` with `precompile = "enabled"`, added alongside
the analysis-only suite to close this gap: it forces the precompiler
action to actually run, and needs no CI wiring since it carries no tag
excluding it from the existing Windows job's default test sweep.
diff --git a/news/4082.fixed.md b/news/4082.fixed.md
new file mode 100644
index 0000000..12c6a51
--- /dev/null
+++ b/news/4082.fixed.md
@@ -0,0 +1,3 @@
+(toolchain) Fixed a crash on Windows when precompiling is enabled:
+the precompiler's interpreter couldn't find its DLLs once relocated
+([#4082](https://github.com/bazel-contrib/rules_python/issues/4082)).
diff --git a/python/private/precompile.bzl b/python/private/precompile.bzl
index c12882b..898dc3e 100644
--- a/python/private/precompile.bzl
+++ b/python/private/precompile.bzl
@@ -15,6 +15,7 @@
load("@bazel_skylib//rules:common_settings.bzl", "BuildSettingInfo")
load(":attributes.bzl", "PrecompileAttr", "PrecompileInvalidationModeAttr", "PrecompileSourceRetentionAttr")
+load(":common.bzl", "actions_run")
load(":flags.bzl", "PrecompileFlag")
load(":py_interpreter_program.bzl", "PyInterpreterProgramInfo")
load(":toolchain_types.bzl", "EXEC_TOOLS_TOOLCHAIN_TYPE", "TARGET_TOOLCHAIN_TYPE")
@@ -108,25 +109,8 @@
exec_tools_info = ctx.toolchains[EXEC_TOOLS_TOOLCHAIN_TYPE].exec_tools
target_toolchain = ctx.toolchains[TARGET_TOOLCHAIN_TYPE].py3_runtime
- # These args control starting the precompiler, e.g., when run as a worker,
- # these args are only passed once.
- precompiler_startup_args = ctx.actions.args()
-
- env = {}
- tools = []
-
precompiler = exec_tools_info.precompiler
- if PyInterpreterProgramInfo in precompiler:
- precompiler_executable = exec_tools_info.exec_interpreter[DefaultInfo].files_to_run
- program_info = precompiler[PyInterpreterProgramInfo]
- env.update(program_info.env)
- precompiler_startup_args.add_all(program_info.interpreter_args)
- default_info = precompiler[DefaultInfo]
- precompiler_startup_args.add(default_info.files_to_run.executable)
- tools.append(default_info.files_to_run)
- elif precompiler[DefaultInfo].files_to_run:
- precompiler_executable = precompiler[DefaultInfo].files_to_run
- else:
+ if PyInterpreterProgramInfo not in precompiler and not precompiler[DefaultInfo].files_to_run:
fail(("Unrecognized precompiler: target '{}' does not provide " +
"PyInterpreterProgramInfo nor appears to be executable").format(
precompiler,
@@ -159,12 +143,6 @@
else:
invalidation_mode = PrecompileInvalidationModeAttr.CHECKED_HASH
- # Though --modify_execution_info exists, it can only set keys with
- # empty values, which doesn't work for persistent worker settings.
- execution_requirements = {}
- if testing.ExecutionInfo in precompiler:
- execution_requirements.update(precompiler[testing.ExecutionInfo].requirements)
-
# These args are passed for every precompilation request, e.g. as part of
# a request to a worker process.
precompile_request_args = ctx.actions.args()
@@ -188,20 +166,13 @@
python_version = "{}.{}".format(version_info.major, version_info.minor)
precompile_request_args.add("--python_version", python_version)
- ctx.actions.run(
- executable = precompiler_executable,
- arguments = [precompiler_startup_args, precompile_request_args],
+ actions_run(
+ ctx,
+ executable = precompiler,
+ arguments = [precompile_request_args],
inputs = [src],
outputs = [pyc],
mnemonic = "PyCompile",
progress_message = "Python precompiling %{input} into %{output}",
- tools = tools,
- env = env | {
- "PYTHONHASHSEED": "0", # Helps avoid non-deterministic behavior
- "PYTHONNOUSERSITE": "1", # Helps avoid non-deterministic behavior
- "PYTHONSAFEPATH": "1", # Helps avoid incorrect import issues
- },
- execution_requirements = execution_requirements,
- toolchain = EXEC_TOOLS_TOOLCHAIN_TYPE,
)
return pyc
diff --git a/tests/base_rules/precompile/precompile_tests.bzl b/tests/base_rules/precompile/precompile_tests.bzl
index bff994a..d2c1da6 100644
--- a/tests/base_rules/precompile/precompile_tests.bzl
+++ b/tests/base_rules/precompile/precompile_tests.bzl
@@ -14,6 +14,7 @@
"""Tests for precompiling behavior."""
+load("@bazel_skylib//rules:write_file.bzl", "write_file")
load("@rules_testing//lib:analysis_test.bzl", "analysis_test")
load("@rules_testing//lib:test_suite.bzl", "test_suite")
load("@rules_testing//lib:truth.bzl", "matching")
@@ -510,6 +511,24 @@
_tests.append(_test_precompile_attr_inherit_pyc_collection_disabled_precompile_flag_enabled)
+# buildifier: disable=function-docstring-header
+def _test_precompile_enabled_succeeds(name):
+ """Verify that a `py_test` target actually builds and runs with
+ precompiling (the above `analysis_test`s only check declared providers).
+ """
+ write_file(
+ name = name + "_main",
+ out = name + "_main.py",
+ )
+ py_test(
+ name = name,
+ srcs = [name + "_main.py"],
+ main = name + "_main.py",
+ precompile = "enabled",
+ )
+
+_tests.append(_test_precompile_enabled_succeeds)
+
def runfiles_contains_at_least_predicates(runfiles, predicates):
for predicate in predicates:
runfiles.contains_predicate(predicate)
diff --git a/tools/precompiler/precompiler.py b/tools/precompiler/precompiler.py
index f83dd15..7c44a32 100644
--- a/tools/precompiler/precompiler.py
+++ b/tools/precompiler/precompiler.py
@@ -34,7 +34,11 @@
parser.add_argument("--persistent_worker", action="store_true")
parser.add_argument("--log_level", default="ERROR")
- parser.add_argument("--worker_impl", default="async")
+ # Bazel workers use anonymous pipes for stdio, which don't support
+ # overlapped I/O required by asyncio on Windows.
+ parser.add_argument(
+ "--worker_impl", default="serial" if sys.platform == "win32" else "async"
+ )
return parser
@@ -167,7 +171,9 @@
outstream: "typing.TextIO", # noqa: F821
) -> "tuple[asyncio.StreamReader, asyncio.StreamWriter]":
loop = asyncio.get_event_loop()
- reader = asyncio.StreamReader()
+ # Cap reader at 4 MiB, leaving enough headroom over the default 64 KiB
+ # for request lines with numerous inputs (~470 KiB as of CPython 3.11).
+ reader = asyncio.StreamReader(limit=1 << 22)
protocol = asyncio.StreamReaderProtocol(reader)
await loop.connect_read_pipe(lambda: protocol, instream)
