Skip to content

Disallow output bases under GC-able directories #26138

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented May 23, 2025

We have had user's report spurious build failures due to the disk cache GC collecting files under the output base when output base and disk cache were set up at the same path.

We have had user's report spurious build failures due to the disk cache GC collecting files under the output base when output base and disk cache were set up at the same path.
@fmeum fmeum requested a review from a team as a code owner May 23, 2025 09:25
@fmeum fmeum requested review from Wyverald and tjgq and removed request for a team May 23, 2025 09:25
@github-actions github-actions bot added team-Remote-Exec Issues and PRs for the Execution (Remote) team awaiting-review PR is awaiting review from an assigned reviewer labels May 23, 2025
@fmeum
Copy link
Collaborator Author

fmeum commented May 23, 2025

We might need something more elaborate here as repo contents and disk cache probably also shouldn't share a directory. Maybe BlazeModules should report their managed directories?

Copy link
Contributor

@tjgq tjgq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error messages don't match the condition we're testing for.

To be clear: a cache inside the output base should be ok (though perhaps ill-advised), only the output base inside a cache is a problem?

Separately - should we canonicalize paths so that the check isn't defeated by a symlink?

@fmeum fmeum requested a review from tjgq May 26, 2025 16:16
@fmeum fmeum marked this pull request as draft May 26, 2025 16:27
@fmeum
Copy link
Collaborator Author

fmeum commented May 26, 2025

Canonicalizing paths requires more care as symlinks may not exist, will mark as ready for review again when that is fixed.

@fmeum fmeum force-pushed the managed-directories branch from 58d31c5 to 5a8197e Compare May 26, 2025 17:07
@fmeum fmeum marked this pull request as ready for review May 27, 2025 08:05
@fmeum
Copy link
Collaborator Author

fmeum commented May 27, 2025

CI is green now. I also added comments on why intermediate symlinks don't need to be checked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review PR is awaiting review from an assigned reviewer team-Remote-Exec Issues and PRs for the Execution (Remote) team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants