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

Make runfiles incrementally correct with --nobuild_runfile_links #20695

Closed
wants to merge 2 commits into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Dec 29, 2023

Fixes three separate but related issues with --nobuild_runfile_links:

  • On all OSes, flipping --enable_runfiles from on to off did not result in runfiles being cleared due to a missing call to SymlinkTreeHelper#clearRunfilesDirectory that was only added to SymlinkTreeStrategy in f84329e.
  • On Windows only, flipping --enable_runfiles from off to on did not result in runfiles being created since the logic in RunfilesTreeUpdater would see a copy of the manifest instead of a symlink. This is fixed by additionally checking whether the runfiles directory contains the first runfile on Windows. If runfiles were disabled before, it won't, otherwise it will.
  • On all OSes, --noenable_runfiles was ignored by bazel run, with runfiles always being created. This is fixed by using RunfilesTreeUpdater instead of custom and incorrect logic.

With the fixed behavior, the runfiles tree for tests is now cleared right before the test spawn is executed, which makes the test action unable to create its working directory, the subdirectory of the runfiles directory corresponding to the workspace name. To fix this and also get rid of the inconsistency of having another action write into the runfiles tree, this logic is moved into the SymlinkTreeHelper and thus applied to all runfiles trees.

Work towards #20676
Fixes #19333

@fmeum fmeum force-pushed the 20676-runfiles-incrementality branch 7 times, most recently from 5d04948 to 33790b8 Compare December 29, 2023 15:09
@fmeum fmeum requested a review from tjgq December 29, 2023 15:50
@fmeum fmeum marked this pull request as ready for review December 29, 2023 15:50
@fmeum fmeum requested review from a team as code owners December 29, 2023 15:50
@fmeum fmeum requested review from aiuto, a team and gregestren and removed request for a team, aiuto and gregestren December 29, 2023 15:50
@github-actions github-actions bot added team-Performance Issues for Performance teams team-Configurability platforms, toolchains, cquery, select(), config transitions team-Remote-Exec Issues and PRs for the Execution (Remote) team awaiting-review PR is awaiting review from an assigned reviewer labels Dec 29, 2023
@fmeum
Copy link
Collaborator Author

fmeum commented Dec 29, 2023

@tjgq This turned out to be an unexpectedly deep rabbit hole of tangled issues. I added a bunch of new tests and got them to pass, ignoring what I think are flakes. I can definitely try to split this into smaller bits, but some of the fixes cause regressions in isolation.

As I'm ultimately trying to fix #20676 by getting rid of RUNFILES_MANIFEST_ONLY, I'm wondering what you would think of replacing it with a marker runfile (e.g. an empty top-level _runfiles_manifest_only file) created only by SymlinkTreeHelper#clearRunfilesDirectory and used as a signal to use the manifest by runfiles libraries. This could also be a less hacky replacement for the up-to-dateness check in RunfilesTreeUpdater on Windows. I don't remember seeing any breakages when we added _repo_mapping.

@fmeum fmeum force-pushed the 20676-runfiles-incrementality branch from 33790b8 to b8c9614 Compare January 2, 2024 13:26
@tjgq tjgq removed the awaiting-review PR is awaiting review from an assigned reviewer label Jan 2, 2024
@tjgq tjgq added the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Jan 2, 2024
@tjgq
Copy link
Contributor

tjgq commented Jan 3, 2024

Note: changed RunfilesTree#getWorkspaceName on import to return String instead of PathFragment, as it seems a more natural choice.

@copybara-service copybara-service bot closed this in 84d1a72 Jan 3, 2024
@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Jan 3, 2024
@tjgq
Copy link
Contributor

tjgq commented Jan 3, 2024

@tjgq This turned out to be an unexpectedly deep rabbit hole of tangled issues. I added a bunch of new tests and got them to pass, ignoring what I think are flakes. I can definitely try to split this into smaller bits, but some of the fixes cause regressions in isolation.

As I'm ultimately trying to fix #20676 by getting rid of RUNFILES_MANIFEST_ONLY, I'm wondering what you would think of replacing it with a marker runfile (e.g. an empty top-level _runfiles_manifest_only file) created only by SymlinkTreeHelper#clearRunfilesDirectory and used as a signal to use the manifest by runfiles libraries. This could also be a less hacky replacement for the up-to-dateness check in RunfilesTreeUpdater on Windows. I don't remember seeing any breakages when we added _repo_mapping.

Sorry, I missed this when reviewing your PR. I think what you describe is quite reasonable. (I'd suggest inverting the meaning of the marker to have it signal that the symlinks have been created; that seems more natural to me, but maybe there are reasons why it has to be the other way round.)

@tjgq
Copy link
Contributor

tjgq commented Jan 3, 2024

(Thanks for working on this, though - I set out to fix the incrementality issues with the runfiles flags a while ago, but could never quite find the time/patience to get to the end of it :))

@fmeum fmeum deleted the 20676-runfiles-incrementality branch January 4, 2024 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Configurability platforms, toolchains, cquery, select(), config transitions team-Performance Issues for Performance teams team-Remote-Exec Issues and PRs for the Execution (Remote) team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--enable_runfiles is not applied correctly without manual cleaning
2 participants