Skip to content
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

Do not create runfile trees unnecessarily when building without the bytes #18580

Closed
tjgq opened this issue Jun 5, 2023 · 1 comment
Closed
Labels
team-Remote-Exec Issues and PRs for the Execution (Remote) team type: bug

Comments

@tjgq
Copy link
Contributor

tjgq commented Jun 5, 2023

Currently, --remote_download_toplevel causes all runfiles symlink trees to be created (not just the top-level ones), while --remote_download_minimal implies --nobuild_runfile_symlinks and activates a separate code path (see RunfilesTreeUpdater) to create input runfiles trees just before local action execution.

Instead, we should:

  1. Return true from RemoteOutputService#canCreateSymlinks and provide an empty RemoteOutputService#createSymlinks implementation; this ensures the runfiles tree is never materialized as a direct result of running the SymlinkTreeAction.
  2. Have SpawnRunner#prefetchInputs create the runfiles tree before local action execution (as is already the case for other local action inputs).
  3. Have AbstractActionInputPrefetcher#finalizeAction create the runfiles tree for toplevel targets (as is already the case for other top-level outputs).

This would ensure a minimal set of runfile trees is created in all cases, and let us omit --nobuild_runfile_links from the expansion of --remote_download_minimal.

@tjgq
Copy link
Contributor Author

tjgq commented Aug 31, 2023

After spending a lot of time investigating this, I don't see a way to fix this issue that doesn't result in an uncomfortable amount of complexity to be added. The "build without the bytes" machinery only really works for remote actions, but runfiles are an internal (in-process) action, and tracking an "executed-but-not-yet-materialized" state for such an action is not a simple change. It would potentially be worth doing if extended to other in-process actions (FileWriteAction, TemplateExpand, etc) but I don't see evidence that this is necessary, and I'd prefer not to introduce it just for the sake of runfiles.

Instead, I think for now we should simply remove --nobuild_runfile_links from the expansion of --remote_download_minimal and move on. This is technically a regression, but:

  • Users can still manually set --nobuild_runfile_links if they care.
  • As a benefit, we no longer discard the entire analysis phase when switching between --remote_download_minimal and --remote_download_toplevel, or between a local and remote build, on account of --nobuild_runfile_links changing.
  • In the future, we should consider defaulting --build_runfile_links to false; runfile symlink trees are unlikely to be interesting output artifacts except for debugging purposes, and are best regarded strictly as a property of the action execution environment.

I acknowledge that this leaves us without a mechanism to selectively decide which targets to materialize runfile symlink trees for, but again, I'd like to see evidence that this is necessary. (Note that it will remain the case that runfile symlink trees are created just-in-time whenever required by a local action or a bazel run command.)

copybara-service bot pushed a commit that referenced this issue Aug 31, 2023
…d_minimal.

This is arguably not a "build without the bytes" feature; runfile symlink trees are always local (in-process) outputs and never produced remotely. Omitting the flag from the expansion avoids discarding most of the analysis phase when flipping between --remote_download_toplevel and --remote_download_minimal.

It remains possible to disable the creation of runfile symlink trees by manually setting --nobuild_runfile_links.

See also #18580.

RELNOTES: --remote_download_minimal no longer implies --nobuild_runfile_links.
PiperOrigin-RevId: 561655765
Change-Id: I1ec0ca3e493bccad0feb666987fef04d9c528b12
@tjgq tjgq closed this as not planned Won't fix, can't repro, duplicate, stale Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Remote-Exec Issues and PRs for the Execution (Remote) team type: bug
Projects
None yet
Development

No branches or pull requests

2 participants