[envpool] migrate Bazel Qt rules in-tree - #341
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7316cdc541
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| qt_autoconf = repository_rule( | ||
| implementation = _qt_autoconf_impl, | ||
| configure = True, | ||
| ) |
There was a problem hiding this comment.
Track BAZEL_RULES_QT_DIR in the repository rule
If a developer sets or changes BAZEL_RULES_QT_DIR after the first fetch, Bazel can keep reusing the cached @local_config_qt repository because qt_autoconf does not declare that environment variable as an input. Reading repository_ctx.os.environ directly here means the generated local_qt.bzl can stay pinned to a stale header path until the user manually cleans or syncs, which makes switching/fixing Qt installations unexpectedly fail.
Useful? React with 👍 / 👎.
| allow_empty = True, | ||
| ), | ||
| includes = ["."], | ||
| linkopts = ["-lQt5Core"], |
There was a problem hiding this comment.
Add the configured Qt libdir to the link flags
When BAZEL_RULES_QT_DIR points at a non-system Qt installation (for example /opt/Qt/...), this target still links with bare -lQt5Core/-lQt5Gui flags and never adds that prefix's lib directory. In that setup the headers resolve because new_local_repository is rooted at the include tree, but the final link still searches only the default system paths and fails to find the Qt shared libraries.
Useful? React with 👍 / 👎.
Summary
bazel_rules_qtrepository even though the repo only needs a very small local Qt wrapper for Procgen, and the current branch also needed a smallbox2dtyping fix to keep lint moving.bazel_rules_qtarchive with repo-local Qt configure/build files used by Procgen, and includes a narrowbox2d_correctness_test.pymypy fix. It does not changethreadpool,gym3_libenv, orbazel_clang_tidypins.box2d_correctness_test.pyno longer trips mypy on scalar/shape comparisons.This keeps the Qt dependency surface in-tree and removes a dead external Bazel rules dependency without changing Procgen's linked Qt libraries.
Technical Details
third_party/qtrepository rule that resolves the local Qt include directory and exposes theqt_core/qt_guilibraries EnvPool actually uses; then wireWORKSPACEandworkspace1.bzlto those local rules.third_party/qt/qt_configure.bzl: minimal local Qt repository rule that finds the Qt headers and templateslocal_qt.bzlthird_party/qt/qt.BUILD: repo-localqt_core/qt_guiBazel targets used by ProcgenWORKSPACE: rewires@qtto the in-repo Qt BUILD file and drops the external Qt toolchain registrationenvpool/box2d/box2d_correctness_test.py: swaps twoassert_allclosecalls forassertEqualto satisfy mypy on shape/discrete-cardinality checksjustbuchanan/bazel_rules_qtis archived and the previous pin was already at upstream head, so this is a replacement rather than a version bump.Test Plan
Automated
make buildifier: passmake addlicense: passmake mypy: passUSE_BAZEL_VERSION=8.6.0 bazelisk test //envpool/procgen:procgen_env_test //envpool/procgen:procgen_test --enable_workspace --noenable_bzlmod --config=test --test_output=errors --nocache_test_resultsondev-0: passmake bazel-testondev-0:30 / 30 tests passmake lintondev-0: in progress;flake8,py-format,clang-format,cpplint,buildifier,addlicense,mypy,docstyle, andspellinghave passed, and only the long-runningclang-tidytail remainsSuggested Manual
make linton a Linux devbox: confirm the remainingclang-tidytail finishes cleanly on the exact tree in this PR