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

rsc: Disallow hidden file dependencies #1649

Merged
merged 2 commits into from
Sep 24, 2024
Merged

Conversation

V-FEXrt
Copy link
Contributor

@V-FEXrt V-FEXrt commented Sep 19, 2024

When a job creates both a file and a symlink to that file but only outputs the symlink it creates a hidden dependency on that untracked file.

This is generally not a huge problem but now is since the RSC is moving to only track true dependencies. The symlink would be restored but not the "hidden" file.

This PR panics when that situation occurs for a cached job. Broken jobs can avoid this by opting out of caching or by also outputting the hidden file

Base automatically changed from stdlib-list-fns to master September 23, 2024 21:31
@V-FEXrt V-FEXrt force-pushed the rsc-disallowed-hidden-file branch from e6e6e15 to f56a6d5 Compare September 23, 2024 21:44
@V-FEXrt V-FEXrt changed the title rsc: disallowe hidden file rsc: Disallow hidden file dependencies Sep 23, 2024
Copy link
Contributor

@colinschmidt colinschmidt left a comment

Choose a reason for hiding this comment

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

Looks correct to me, just one question about error handling.

share/wake/lib/system/remote_cache_runner.wake Outdated Show resolved Hide resolved
symlinksUpload
| findFail
|< map getCachePostRequestOutputSymlinkPath
else True
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are unable to get the target for an output symlink, we are assuming that means this job didn't output it?
Or are we just choosing to ignore that error here and let some other part of the code handle that failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we just choosing to ignore that error here and let some other part of the code handle that failure

This one. The only failure here is a failure to readlink and if that fails then another part of the code (just a few lines below) will fully abort the job push

Copy link
Contributor

@colinschmidt colinschmidt left a comment

Choose a reason for hiding this comment

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

LGTM

@V-FEXrt V-FEXrt merged commit ad7ca2a into master Sep 24, 2024
12 checks passed
@V-FEXrt V-FEXrt deleted the rsc-disallowed-hidden-file branch September 24, 2024 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants