bazel: take the doc build's Python from the toolchain, not the host - #11348
Conversation
//docs:man_pages shells out to docs/Makefile, whose preprocess step ran
`python3` off PATH. The action's environment is not the invoking shell's,
so a newer interpreter earlier on the user's PATH does not necessarily
reach it, and md_roff_compat.py needs Python >= 3.7 ('from __future__
import annotations'). On distributions that still ship 3.6 as
/usr/bin/python3 -- RHEL 8, for instance -- the build fails with
"SyntaxError: future feature annotations is not defined".
Resolve @rules_python//python:toolchain_type in the man_pages rule and
hand the interpreter to make as PYTHON, declaring the runtime's files as
action inputs; the Makefile keeps `python3` as its default so a non-Bazel
`make -C docs` is unchanged. The same toolchain lookup already backs
tcl_encode_or and the messages_txt genrules.
//:dup_id_test was the one remaining host-python user in the Bazel graph;
give it the interpreter through PYTHON3_ROOTPATH for the same reason.
Fixes The-OpenROAD-Project#11290
Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the build and test scripts to use the Python interpreter resolved by the Bazel toolchain instead of relying on the host's Python installation, ensuring compatibility on systems with older host Python versions. Feedback was provided to resolve two issues: first, in etc/find_dup_ids.sh, hardcoding the _main workspace name and prepending the runfiles directory can break if the workspace is renamed or if PYTHON3_ROOTPATH is already absolute; second, in bazel/man_pages.bzl, py3_runtime.files can be None under certain toolchain configurations, which would cause a runtime error when constructing the depset if not checked.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
PYTHON3_ROOTPATH is $(PYTHON3_ROOTPATH), which current_py_toolchain sets from the runtime's short_path for a hermetic toolchain but from the absolute interpreter_path for a platform one. Prefixing the runfiles directory turned the second form into a nonexistent path. Resolve it against the working directory instead, which for a test is the runfiles root that short_path is relative to, and which realpath leaves an absolute path alone in. That also drops the hardcoded _main repo name. Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
😌 |
2 participants
//docs:man_pages shells out to docs/Makefile, whose preprocess step ran
python3off PATH. The action's environment is not the invoking shell's, so a newer interpreter earlier on the user's PATH does not necessarily reach it, and md_roff_compat.py needs Python >= 3.7 ('from future import annotations'). On distributions that still ship 3.6 as /usr/bin/python3 -- RHEL 8, for instance -- the build fails with "SyntaxError: future feature annotations is not defined".Resolve @rules_python//python:toolchain_type in the man_pages rule and hand the interpreter to make as PYTHON, declaring the runtime's files as action inputs; the Makefile keeps
python3as its default so a non-Bazelmake -C docsis unchanged. The same toolchain lookup already backs tcl_encode_or and the messages_txt genrules.//:dup_id_test was the one remaining host-python user in the Bazel graph; give it the interpreter through PYTHON3_ROOTPATH for the same reason.
Fixes #11290