[SR-12851] System-wide cache of SwiftPM dependencies

[tgymnich]

I created a draft for the implementation of a system wide cache for swift packages. I would love to get some of your feedback.

In particular I would like some feedback about the choice of the directory for the cache: .swiftpm/cache/repositories
and its maximum size of 20 GB.

SR-12851

[MaxDesiatov]

This refers to SR-12581, but the description of that issue seems unrelated to me, could the reference be wrong?

[tgymnich]

Thanks for the heads up @MaxDesiatov I had the numbers twisted 😅. I updated the description accordingly.

[MaxDesiatov]

This change looks useful to me, at least in theory. I have no impact on the actual roadmap of SwiftPM and I don't know if the development team is interested in this feature though. I've left some coding style suggestions and nitpicks, but I don't think you should address them until you're sure that the SwiftPM team is interested in an implementation of this feature in principle.

If it is merged at some point, one thing I'd like to be included is configurable cache size, otherwise 20 GB seems like too big as a default, I don't remember having that much free space on any of my devices and would be frustrated to discover that all that free space was consumed by SwiftPM without my knowledge or control.

[MaxDesiatov]

I think the error should still be reported somehow, maybe just printing it would be an improvement.

[tgymnich]

@MaxDesiatov thanks for your feedback. I've already fixed some of the formatting issues.

If it is merged at some point, one thing I'd like to be included is configurable cache size, otherwise 20 GB seems like too big as a default, I don't remember having that much free space on any of my devices and would be frustrated to discover that all that free space was consumed by SwiftPM without my knowledge or control.

As far as I know there is no swiftpm config file yet. I can think of 3 possible solutions without setting up a config file:

• Using environment variables to control caching behavior
• Using command line arguments to control caching behavior
• Relying on the OS to manage and purge caches if needed. Using the ~/.cache directory on linux and ~/Library/Caches on macOS. further reading: macOS Directories, Accessing Files and Directories. (the linux directory is not automatically purged as far as I know)

I prefer the last approach.

[abertelrud]

First of all, thanks a lot for this contribution! I think this is a great idea, and I know it's been discussed as something desirable (though I don't think there was a JIRA for it already, and certainly not a PR). I'll want to look closer at the diffs, and in particular to see how it would be used from an IDE (though libSwiftPM), but the approach looks sound.

[abertelrud]

@swift-ci please test

[abertelrud]

Actually: since RepositoryProvider already assumes Git semantics, and the GitRepositoryProvider is just the implementation that uses the 'git' command line tool, a more scalable approach might be to do this outside the RepositoryProvider. This would allow the repository provider to do its thing to fill the cache, and then a shared implementation would then clone the repo out of the cache into the desired location. That would make the cache work for all repository providers.

[SDGGiesbrecht]

@TG908, it’s probably best to ask Foundation’s FileManager for the .cachesDirectory directory, and then append com.apple.swiftpm to it. That will:

• Allow a single, simple cross‐platform implementation (resulting in virtually the same macOS and Linux paths you specified).
• Ensure everything lands wherever the platform is allowed to purge (regardless of whether it actually does), even when XDG has been customized.
• Properly satisfy all platform’s guidelines for organizing files within the cache.
• Already handle Windows and Android correctly (even though SwiftPM isn’t stable on either yet).

[aciidgh]

I like @SDGGiesbrecht's suggestion of using FileManager.default.urls(for: .cachesDirectory, in: .userDomainMask) but I think we should append org.swift.swiftpm.

[tgymnich]

I've addressed some of the changes suggested by @aciidb0mb3r and @SDGGiesbrecht. Additionally I've fixed the build issue on Linux (The old git version. on ubuntu 16.0.4 didn't support git clone --reference-if-able). I am currently looking into doing the caching outside the RepositoryProvider.

[jakepetroules]

Thanks for your contribution, this is a great idea!

One thing I'm a little concerned about is how this will handle concurrent invocations of swift build on the same system, doing package resolution at the same time. Have you considered any strategies for how we can ensure that one process won't corrupt the cache being written by another?

[neonichu]

Thanks for the PR, @TG908

I haven't had time to dig into the details, but two high level comments:

• I'm not sure I like the approach of adding an additional cache to do this. Conceptually, the existing "repositories" directory can already been seen as a cache and it would be nice if we could just make that global. I had started a branch to do so a while ago here: https://github.com/neonichu/swift-package-manager/tree/shared-repository-cache
• I would also reiterate @jakepetroules' point that in order to land this, we would need some kind of strategy to deal with concurrent access to the cache.

[tgymnich]

I'm not sure I like the approach of adding an additional cache to do this. Conceptually, the existing "repositories" directory can already been seen as a cache and it would be nice if we could just make that global.

I wasn't aware that the "repositories" folders only purpose is caching. I like the idea of replacing it with a global cache.

@neonichu @jakepetroules concurrent cache accesses will be protected by a FileLock. There are 3 critical sections:

1. Initializing the cache (cloning) / performing a fetch on the cached repository (exclusive lock)
2. Purging the cache (exclusive lock)
3. Cloning the actual repository from the cached repository (shared lock)

To make this more performant I would need to add shared locks to swift-tools-support-core, which I already did in my fork. I could use some help testing this on Windows though.

[tgymnich]

How should we handle some of the existing swift package subcommands and options like:

• reset -> resetting the global cache would be unexpected when running this on a package
• --skip-update -> fetch state of repositories can be modified by other packages when using a global cache

I went ahead and split the option --build-path into two options:
1) --build-path (path for .build)
2) --cache-path (path for repositories). When this flag is not used we are defaulting to the global cache directory. Setting the flag to anything else would allow to 'opt out' of global caching. Problem: subcommands like reset will depending on the flag reset the global cache or reset the user defined cache

Should locking of the repository directories be an implementation detail of Repository/RepositoryProvider or should this be handled in RepositoryManager? The latter approach would require an implementation of FileLock for every FileSystem (right now it is not backed by any FileSystem and only works for the local file system, breaking some unit tests using InMemoryFileSystem).

I also decided that repositories checked out with editable: false will no longer be checked out as --shared. If the global cache were to be deleted at a later point in time, all the checkouts having a reference to the cached repository would be corrupted.

Adding a subcommand for explicitly cleaning the cache would be useful as well.

[neonichu]

Thanks again for the PR @TG908 and sorry that it took a while to get back to you. I'll review this soon and get back you with suggestions on your questions.

[neonichu]

I think we should look into FileLock essentially being a no op when using the in-memory fs instead of doing this workaround.

[tgymnich]

I can see two possible ways of achieving this:

1. make FileLock part of FileSystem --> More extensible
2. make FileLock aware of the FileSystem being used --> Fewer changes to the API required

Shouldn't lock() behave like a normal lock when using the in-memory fs?

[abertelrud]

#1 here seems like the best solution, although it would of course involve more changes, as you point out. It makes sense to me that for an in-memory file system, locking would be just normal locking (not a no-op). For this case, it seems that maybe all that would be needed is a new function on FileSystem that lets you execute a block while holding a lock on a file, e.g. withLock(on: path) { /* do something */ }. The local-file-system implementation is straightforward, but the in-memory file system implementation would involve a locked map of paths to FileLocks (though that's not technically sufficient either because of symlinks, but the implementation there could be refined over time).

[neonichu]

Not sure I understand why we are doing this nesting

[tgymnich]

When I added the workaround to only lock when we are on the local filesystem, I probably copy pasted the block including the lock and added another lock.

[neonichu]

I think this generally looks good, but I need to think through the locking behaviour a bit more. I'm actually not super familiar with the access patterns of SwiftPM w.r.t. the repository cache here, e.g. one scenario I'm worried about is a second client doing a fetch during the resolution process of a first client.

The locking as its implemented in the PR avoids direct conflicts, but I want to make sure we're not opening ourselves up to side effects over the course of the whole resolution process. For example a scenario like this:

• Client A populates the cache for repo X and runs resolution
• Repo X is updated
• Client B happens to update the cache for repo X
• Client A fetches from the cache again during the initial resolution and would get a different result than during the first step

I am not sure this scenario actually happens in practice, but I'd like to spend a bit more time figuring out if it could cause problems down the line.

[neonichu]

I have thought about this a bit more and I think we should actually go back to an approach which keeps the per-project cache as well to avoid some of the side effects I was describing here.

How about this:

• all clone/fetch operations happen on the shared cache, protected by locks (this is what the do in the PR right now).
• once the clone/fetch for a repo happened, we additionally copy it into the per-project repositories directory and this will be the source of truth until the end of the current operation. This will ensure that further fetches that occur on the global cache by other clients won't affect the state of the world as seen by each other client which is essentially the same behaviour as we have today.
• once we finish an operation, we can clean up the additional per-project copies of repos.

We could potentially record the commit hash instead of making a copy, but I'm not 100% sure this would prevent us from seeing issues when that commit gets deleted remotely and we fetch. I think we could explore something like that as a later optimization to copying clones after fetching.

[abertelrud]

Apparently not having looked closely enough at the code of this PR earlier, I actually just wrote up a reply which was essentially "why not simplify things and use --reference --dissociate?". Which it turns out is exactly what this proposal is, so this is my modified reply.

I think using --reference would work great, and I don't think we need a second per-package copy. Because of --dissociate, there would not be issues with dangling references to remote objects. One nice thing about --reference is also that it keeps the remote pointing at the original repo, so that things like Git repo viewer apps and whatnot still work as expected on the clones, regardless of the cache.

So I think the essential approach should be:

• do a git clone --mirror into the global cache if it's not already there, or a git fetch --all if it is
• do a git clone --reference --dissociate --no-checkout to the package's working copy
• do a git checkout to get materialize the right versions of the files

The no-checkout thing both saves time (to avoid an unnecessary materialization of files in the checkout) and also prevents errors in case of things like incorrectly set default-branch references in remote repos (happening more often recently with renaming to main branch).

Git does locking at the repository level, though that might manifest as a failure to perform the operation if it's already locked rather than waiting for it to finish. So a SwiftPM-specific file lock around the repo in the cache might still be warranted.

I think the key here will be to keep the approach simple, and I think the approach of using --reference --dissociate is a good one. So I don't think we need another per-package cache of repositories in between. The worst that would happen if an object isn't in the local cache is that it would be fetched from the remote, not that it would result in a dangling reference.

[abertelrud]

Boris, I think you're right that there is a risk of a problem if we don't have an intermediate per-package repository. The package resolution does cat-file and ls-tree operations out of the bare repository, and so there are three issues:

• concurrent access to the same repository would cause lock contention
• in the unlikely case of a gc or repack operation on the bare clone, any ref could go away if it had become dangling
• also, of course, if someone removes the shared cache during dependency graph resolution, that will cause problems

So after thinking this through I've changed my mind, and now agree with you Boris. I think it would make things safer and simpler to see the global cache as strictly a way to quickly populate the local one, so I think the changes here would be to the fetch operation, not clone. And I suppose that rather than --reference --dissociate, we could just do a --local clone here and then fix up the origin to point to the original URL. That would allow the cloning from the shared cache to the package-specific one use hard links.

[shahmishal]

We moved our default branch to main and deleted master branch, GitHub automatically closed the PR. Please update the base branch to main. More detail about the branch update - https://forums.swift.org/t/updating-branch-names/40412