framework: stage tool runtime closures into EXT_BUILD_DEPS#1529
Draft
framework: stage tool runtime closures into EXT_BUILD_DEPS#1529
Conversation
novas0x2a
force-pushed
the
stage-tool-runfiles
branch
3 times, most recently
from
c0a270a to
2db65a5
Compare
2 tasks
novas0x2a
force-pushed
the
stage-tool-runfiles
branch
9 times, most recently
from
0c41f30 to
f639ee0
Compare
The framework was staging tools by symlinking from the exec root and guessing sidecar paths (.runfiles, .runfiles_manifest). On Windows with bzlmod, the exec-root paths to tool runfiles exceed MAX_PATH — for example, meson's Python runtime can't load its own DLLs because the path is too long. Fix this by: - Adding staged_path, runtime_files, and runfiles metadata fields to ToolInfo so toolchains can declare that a tool's runtime closure should be staged into EXT_BUILD_DEPS (which uses short paths on Windows) - Copying the declared runtime closure and recreating the runfiles tree under EXT_BUILD_DEPS instead of symlinking from the exec root - Setting staged_path on meson and cmake toolchains so their runtimes are staged when used by foreign rules Based on 0198230 and 177a45af from fix-examples-windows.
When a tool is staged into EXT_BUILD_DEPS, its runfiles tree is copied alongside it, but the Python runfiles library (used by the meson wrapper) had no way to discover the staged tree. Fix this by exporting RUNFILES_DIR in _copy_deps_and_tools, after the short-path aliases have been applied. This ensures that on Windows RUNFILES_DIR uses the short temp path (avoiding MAX_PATH), while on Linux/RBE it uses the standard EXT_BUILD_DEPS path. Do NOT set RUNFILES_MANIFEST_FILE or copy the manifest separately: the manifest contains absolute paths from the original build machine (local Bazel cache paths on RBE, long exec-root paths on Windows) that are not valid in the staged tree. _repo_mapping is inside the .runfiles tree and is already covered by the bulk directory copy.
The find+touch that flattens timestamps after a directory copy is the dominant cost of tool-runfiles staging (~60s on macOS, ~115s on Windows), but it only matters for autotools source trees where timestamp ordering prevents regeneration. Add a flatten_timestamps parameter so callers can opt out. Existing callers pass True (preserving current behavior); the runfiles staging copy passes False since nothing in a Python runfiles tree is timestamp-sensitive.
novas0x2a
force-pushed
the
stage-tool-runfiles
branch
from
f639ee0 to
11c7ff4
Compare
There was a problem hiding this comment.
Pull request overview
Updates rules_foreign_cc’s tool staging so tool runtime closures (notably runfiles trees) can be staged under EXT_BUILD_DEPS (short paths on Windows), avoiding MAX_PATH failures when tools (e.g., Meson’s Python runtime) resolve DLLs via absolute runfiles paths.
Changes:
- Extend
ToolInfoto describe staged invocation paths plus runtime/runfiles closures, and enable toolchains (CMake/Meson) to opt into staged execution. - Rework framework staging to copy tool runtime files and (optionally) bulk-copy runfiles trees, exporting
RUNFILES_DIRfor staged tools. - Add a
flatten_timestampsflag tocopy_dir_contents_to_dirand introducecopy_file/copy_file_to_dircommands across platform toolchains.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| toolchains/private/BUILD.bazel | Adds staged_path for CMake/Meson toolchains to enable staged invocation. |
| toolchains/prebuilt_toolchains.py | Switches CMake tool path to $(execpath :cmake_bin) in the generator template. |
| toolchains/prebuilt_toolchains.bzl | Switches CMake tool path to $(execpath :cmake_bin) in the generated repo macro. |
| toolchains/native_tools/tool_access.bzl | Resolves tool invocation paths via invoke_path and supports staged tool path selection. |
| toolchains/native_tools/native_tools_toolchain.bzl | Extends ToolInfo with staged/runfiles/runtime metadata and collects runtime/runfiles closures. |
| foreign_cc/private/run_shell_file_utils.bzl | Updates directory-copy directive to pass the new flatten_timestamps parameter. |
| foreign_cc/private/framework/toolchains/windows_commands.bzl | Adds quote-stripping, new copy helpers, and flatten_timestamps support. |
| foreign_cc/private/framework/toolchains/macos_commands.bzl | Adds quote-stripping, new copy helpers, and flatten_timestamps support. |
| foreign_cc/private/framework/toolchains/linux_commands.bzl | Adds quote-stripping, new copy helpers, and flatten_timestamps support. |
| foreign_cc/private/framework/toolchains/freebsd_commands.bzl | Adds quote-stripping, new copy helpers, and flatten_timestamps support. |
| foreign_cc/private/framework/toolchains/commands.bzl | Extends command metadata for flatten_timestamps and adds file copy commands. |
| foreign_cc/private/framework.bzl | Reworks tool/runtime staging into EXT_BUILD_DEPS, bulk-copies staged runfiles trees, exports RUNFILES_DIR. |
| foreign_cc/private/configure_script.bzl | Updates directory-copy directives to pass flatten_timestamps=True. |
| foreign_cc/meson.bzl | Enables staged Meson tool wrapper invocation (notably on Windows). |
| foreign_cc/built_tools/private/built_tools_framework.bzl | Updates directory-copy directive to pass flatten_timestamps=True. |
| foreign_cc/built_tools/meson_tool_wrapper.py | Improves diagnostics when runfiles resolution fails. |
| foreign_cc/boost_build.bzl | Updates directory-copy directive to pass flatten_timestamps=True. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+121
to
+124
| # tolerate "File exists" errors. Runfiles trees contain repo-mapping | ||
| # symlinks (apparent → canonical) that create duplicate destination | ||
| # paths; cp errors on the second copy but the file is already present. | ||
| return """cp -r --no-target-directory "{source}" "{target}" 2>&1 | grep -v "File exists" >&2 || true""".format( |
There was a problem hiding this comment.
When flatten_timestamps is "False", the cp ... | grep ... || true pipeline will mask all copy failures (because || true forces success even if cp exits non-zero under pipefail). This can hide real errors (missing source dir, permissions, IO errors) and proceed with an incomplete runfiles tree. Recommend preserving cp’s exit status and only ignoring the specific duplicate-path "File exists" case (e.g., capture stderr, filter it, and fail if any remaining errors or if cp failed for other reasons).
Suggested change
| # tolerate "File exists" errors. Runfiles trees contain repo-mapping | |
| # symlinks (apparent → canonical) that create duplicate destination | |
| # paths; cp errors on the second copy but the file is already present. | |
| return """cp -r --no-target-directory "{source}" "{target}" 2>&1 | grep -v "File exists" >&2 || true""".format( | |
| # tolerate only the expected "File exists" errors. Runfiles trees | |
| # contain repo-mapping symlinks (apparent → canonical) that create | |
| # duplicate destination paths; cp errors on the second copy but the | |
| # file is already present. Preserve cp's exit status for all other | |
| # failures. | |
| return """err_file=$(mktemp) && \ | |
| cp -r --no-target-directory "{source}" "{target}" 2>"$err_file"; \ | |
| cp_status=$?; \ | |
| if grep -F -v "File exists" "$err_file" >&2; then \ | |
| has_other_errors=1; \ | |
| else \ | |
| has_other_errors=0; \ | |
| fi; \ | |
| if grep -F "File exists" "$err_file" >/dev/null; then \ | |
| has_file_exists=1; \ | |
| else \ | |
| has_file_exists=0; \ | |
| fi; \ | |
| rm -f "$err_file"; \ | |
| if [ "$has_other_errors" -ne 0 ]; then \ | |
| if [ "$cp_status" -ne 0 ]; then \ | |
| exit "$cp_status"; \ | |
| fi; \ | |
| exit 1; \ | |
| fi; \ | |
| if [ "$cp_status" -ne 0 ] && [ "$has_file_exists" -eq 0 ]; then \ | |
| exit "$cp_status"; \ | |
| fi""".format( |
Comment on lines
+143
to
+146
| # tolerate "File exists" errors. Runfiles trees contain repo-mapping | ||
| # symlinks (apparent → canonical) that create duplicate destination | ||
| # paths; cp errors on the second copy but the file is already present. | ||
| return """cp -r --no-target-directory "{source}" "{target}" 2>&1 | grep -v "File exists" >&2 || true""".format( |
There was a problem hiding this comment.
When flatten_timestamps is "False", the cp ... | grep ... || true pipeline will mask all copy failures (because || true forces success even if cp exits non-zero under pipefail). This can hide real errors (missing source dir, permissions, IO errors) and proceed with an incomplete runfiles tree. Recommend preserving cp’s exit status and only ignoring the specific duplicate-path "File exists" case (e.g., capture stderr, filter it, and fail if any remaining errors or if cp failed for other reasons).
Suggested change
| # tolerate "File exists" errors. Runfiles trees contain repo-mapping | |
| # symlinks (apparent → canonical) that create duplicate destination | |
| # paths; cp errors on the second copy but the file is already present. | |
| return """cp -r --no-target-directory "{source}" "{target}" 2>&1 | grep -v "File exists" >&2 || true""".format( | |
| # tolerate only the duplicate-path "File exists" errors. Runfiles trees | |
| # contain repo-mapping symlinks (apparent → canonical) that create | |
| # duplicate destination paths; cp errors on the second copy but the file | |
| # is already present. Preserve cp's exit status for all other failures. | |
| return """err_file="$(mktemp)" && {{ cp -r --no-target-directory "{source}" "{target}" 2>"$err_file"; cp_status=$?; if grep -qv "File exists" "$err_file"; then grep -v "File exists" "$err_file" >&2; rm -f "$err_file"; if [ "$cp_status" -eq 0 ]; then false; else (exit "$cp_status"); fi; else rm -f "$err_file"; test "$cp_status" -eq 0 || true; fi; }}""".format( |
Comment on lines
+135
to
+139
| if [[ -d "{source}" ]]; then | ||
| cp -R "{source}"/. "{target}" 2>&1 | grep -v "File exists" >&2 || true | ||
| else | ||
| cp -R "{source}" "{target}" 2>&1 | grep -v "File exists" >&2 || true | ||
| fi |
There was a problem hiding this comment.
When flatten_timestamps is "False", the cp ... | grep ... || true pipeline will mask all copy failures (because || true forces success even if cp exits non-zero under pipefail). This can hide real errors (missing source dir, permissions, IO errors) and proceed with an incomplete runfiles tree. Recommend preserving cp’s exit status and only ignoring the specific duplicate-path "File exists" case (e.g., capture stderr, filter it, and fail if any remaining errors or if cp failed for other reasons).
Suggested change
| if [[ -d "{source}" ]]; then | |
| cp -R "{source}"/. "{target}" 2>&1 | grep -v "File exists" >&2 || true | |
| else | |
| cp -R "{source}" "{target}" 2>&1 | grep -v "File exists" >&2 || true | |
| fi | |
| cp_err_file="$(mktemp)" | |
| cp_filtered_err_file="$(mktemp)" | |
| if [[ -d "{source}" ]]; then | |
| cp -R "{source}"/. "{target}" 2>"$cp_err_file" | |
| cp_status=$? | |
| else | |
| cp -R "{source}" "{target}" 2>"$cp_err_file" | |
| cp_status=$? | |
| fi | |
| grep -v "File exists" "$cp_err_file" >"$cp_filtered_err_file" || true | |
| if [[ -s "$cp_filtered_err_file" ]]; then | |
| cat "$cp_filtered_err_file" >&2 | |
| fi | |
| if [[ $cp_status -ne 0 && -s "$cp_filtered_err_file" ]]; then | |
| rm -f "$cp_err_file" "$cp_filtered_err_file" | |
| exit $cp_status | |
| fi | |
| rm -f "$cp_err_file" "$cp_filtered_err_file" |
Comment on lines
+144
to
+148
| if [[ -d "{source}" ]]; then | ||
| cp -R "{source}"/. "{target}" 2>&1 | grep -v "File exists" >&2 || true | ||
| else | ||
| cp -R "{source}" "{target}" 2>&1 | grep -v "File exists" >&2 || true | ||
| fi |
There was a problem hiding this comment.
When flatten_timestamps is "False", the cp ... | grep ... || true pipeline will mask all copy failures (because || true forces success even if cp exits non-zero under pipefail). This can hide real errors (missing source dir, permissions, IO errors) and proceed with an incomplete runfiles tree. Recommend preserving cp’s exit status and only ignoring the specific duplicate-path "File exists" case (e.g., capture stderr, filter it, and fail if any remaining errors or if cp failed for other reasons).
Suggested change
| if [[ -d "{source}" ]]; then | |
| cp -R "{source}"/. "{target}" 2>&1 | grep -v "File exists" >&2 || true | |
| else | |
| cp -R "{source}" "{target}" 2>&1 | grep -v "File exists" >&2 || true | |
| fi | |
| _copy_dir_contents_stderr="$(mktemp)" | |
| _copy_dir_contents_status=0 | |
| if [[ -d "{source}" ]]; then | |
| cp -R "{source}"/. "{target}" 2>"${{_copy_dir_contents_stderr}}" || _copy_dir_contents_status=$? | |
| else | |
| cp -R "{source}" "{target}" 2>"${{_copy_dir_contents_stderr}}" || _copy_dir_contents_status=$? | |
| fi | |
| if grep -qv "File exists" "${{_copy_dir_contents_stderr}}"; then | |
| grep -v "File exists" "${{_copy_dir_contents_stderr}}" >&2 | |
| rm -f "${{_copy_dir_contents_stderr}}" | |
| exit "${{_copy_dir_contents_status:-1}}" | |
| fi | |
| if [[ "${{_copy_dir_contents_status}}" -ne 0 ]] && ! grep -q "File exists" "${{_copy_dir_contents_stderr}}"; then | |
| rm -f "${{_copy_dir_contents_stderr}}" | |
| exit "${{_copy_dir_contents_status}}" | |
| fi | |
| rm -f "${{_copy_dir_contents_stderr}}" |
Comment on lines
+1168
to
+1180
| if tool.target and getattr(tool, "stage_runtime", False): | ||
| tools_files += tool.runtime_files.to_list() | ||
| runfiles_files = tool.runfiles_files.to_list() | ||
| runfiles_manifest = tool.runfiles_manifest | ||
| repo_mapping_manifest = tool.repo_mapping_manifest | ||
| if runfiles_files or runfiles_manifest or repo_mapping_manifest: | ||
| tools_runfiles.append(struct( | ||
| files = runfiles_files, | ||
| invoke_path = tool.invoke_path, | ||
| runfiles_manifest = runfiles_manifest, | ||
| repo_mapping_manifest = repo_mapping_manifest, | ||
| staged_path = tool.path, | ||
| )) |
There was a problem hiding this comment.
_define_inputs() now only stages tool artifacts into tools_files when tool.stage_runtime is true. This drops non-staged tools (e.g. the ninja wrapper) from $EXT_BUILD_DEPS/bin and therefore from PATH, which can break builds that rely on PATH discovery (Meson’s CMake fallback is one example). Consider restoring the previous behavior for non-staged tools (symlink/copy the tool launcher into $EXT_BUILD_DEPS/bin) while keeping the new runtime-closure staging for staged tools.
Suggested change
| if tool.target and getattr(tool, "stage_runtime", False): | |
| tools_files += tool.runtime_files.to_list() | |
| runfiles_files = tool.runfiles_files.to_list() | |
| runfiles_manifest = tool.runfiles_manifest | |
| repo_mapping_manifest = tool.repo_mapping_manifest | |
| if runfiles_files or runfiles_manifest or repo_mapping_manifest: | |
| tools_runfiles.append(struct( | |
| files = runfiles_files, | |
| invoke_path = tool.invoke_path, | |
| runfiles_manifest = runfiles_manifest, | |
| repo_mapping_manifest = repo_mapping_manifest, | |
| staged_path = tool.path, | |
| )) | |
| if tool.target: | |
| if getattr(tool, "stage_runtime", False): | |
| tools_files += tool.runtime_files.to_list() | |
| runfiles_files = tool.runfiles_files.to_list() | |
| runfiles_manifest = tool.runfiles_manifest | |
| repo_mapping_manifest = tool.repo_mapping_manifest | |
| if runfiles_files or runfiles_manifest or repo_mapping_manifest: | |
| tools_runfiles.append(struct( | |
| files = runfiles_files, | |
| invoke_path = tool.invoke_path, | |
| runfiles_manifest = runfiles_manifest, | |
| repo_mapping_manifest = repo_mapping_manifest, | |
| staged_path = tool.path, | |
| )) | |
| else: | |
| executable = tool.target.files_to_run.executable | |
| if executable: | |
| tools_files.append(executable) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On Windows with bzlmod, exec-root paths to tool runfiles exceed MAX_PATH. The framework was staging tools by symlinking
.runfilesand.runfiles_manifestfrom the exec root, so a tool like meson — whose Python runtime uses those paths to load DLLs — would fail because the absolute paths were too long. This fixes the root cause by staging the entire runtime closure intoEXT_BUILD_DEPS, which uses short paths on Windows.Three changes, in dependency order:
Stage tool runtimes into EXT_BUILD_DEPS (
framework: stage tool runtime closures into EXT_BUILD_DEPS): Addsstaged_path,runtime_files, andrunfilesfields toToolInfo. When set, the framework copies the declared runtime closure and recreates the runfiles tree underEXT_BUILD_DEPS(a short-path location on Windows) instead of symlinking from the exec root. Setsstaged_pathon the meson and cmake toolchains. The old symlink-based staging path is still used for tools that don't declare a staged_path.Export RUNFILES_DIR for staged tools (
framework: export RUNFILES_DIR for staged tools, drop manifest copy): After the runfiles tree is copied, exportsRUNFILES_DIRpointing at the staged tree so the Python runfiles library can find it. Does not setRUNFILES_MANIFEST_FILEor copy the manifest — manifests contain absolute paths from the build machine (exec-root paths on Windows, Bazel cache paths on RBE) that are not valid in the staged location._repo_mappingis already inside the.runfilestree and doesn't need separate handling.Skip timestamp flattening for runfiles copies (
copy_dir_contents_to_dir: add flatten_timestamps flag): Thefind -exec touch -rthat follows every directory copy costs ~60s on macOS and ~115s on Windows. It exists to prevent autotools from re-running generators when timestamps are out of order, but nothing in a Python runfiles tree is timestamp-sensitive. Adds aflatten_timestampsparameter; existing callers passTrue(no behavior change), the runfiles staging copy passesFalse. This also fixes a Windows/MSYS2File existscrash: runfiles trees contain repo-mapping symlinks where both the apparent and canonical names resolve to the same path;cp -Lwould dereference both and fail on the second. Theflatten_timestamps=Falsepath drops-Land suppresses the benign error.Note: this pops the remote cache for all foreign rules actions due to the generated shell script changes.