Skip to content

Fix std.Build.Step.Run directory caching #24438

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 2 commits into
base: master
Choose a base branch
from

Conversation

silversquirl
Copy link
Contributor

Currently, std.Build.Step.Run has no way to properly cache or watch a directory. run_step.addDirectoryArg is basically useless, as it only caches the name of the directory, not the contents.

There are two potential workarounds:

  • Traverse the directory and write a depfile. This doesn't detect new directory entries. Allowing depfiles to specify directory prerequisites could solve this problem, but would require making directories first-class within the cache system, which they currently aren't.
  • Use a WriteFile step to copy the entire directory into the cache. This works, but it results in a completely pointless copy, and absolutely blows up the size of the cache dir if the files in the directory change frequently.

This PR makes addDirectoryArg actually useful, by having it walk the directory and recursively add files to the cache manifest. I'm not certain that this is the intended behaviour, or even the best behaviour, but it's certainly what I would expect the function to do.

Another possible option for the behaviour of this function is to cache based on just the names and types of the files in the tree - ie. changes to the directory structure will trigger rebuilds, but changes to file contents will not.
This seems less useful, but would speed up building the cache manifest. The full caching behaviour could still be achieved by writing a depfile.
I'm tempted to split addDirectoryArg into two functions, or give it an options parameter, in order to allow selecting between these two behaviours. However, I still think the default should be what I've implemented in this PR, as it is the most reliable option, and the one least likely to surprise users.

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.

1 participant