Cache lld binaries to avoid redundant rebuilds
#35
Conversation
In subsequent PRs I'd like to introduce a SQLite-backed cache for storing hashes of generated artifacts. That will help us in avoiding redundant work and will make generator significantly quicker for re-runs that change only a few arguments.
This proceeds with converting more of the generation steps to a query-based architecture. Now `lld` doesn't need to be rebuilt if its source directory and build options don't change. Also renamed `DownloadQuery` to `DownloadArtifactQuery` and moved it to a separate file.
| let buildDirectory = self.sourcesDirectory.appending("build") | ||
| try await Shell.run("ninja", currentDirectory: buildDirectory) | ||
|
|
||
| return self.outputBinarySubpath.reduce(into: buildDirectory) { $0.append($1) } |
There was a problem hiding this comment.
This seems to me to read more easily and produces the same result, but maybe I'm missing something subtle
.
| return self.outputBinarySubpath.reduce(into: buildDirectory) { $0.append($1) } | |
| return buildDirectory.appending(outputBinarySubpath.components) |
There was a problem hiding this comment.
This was mostly about making it explicit that the query takes relative paths as arguments and not absolute ones.
There was a problem hiding this comment.
Yes, I was reading the docs for FilePath just now and I wondered if that was what you were intending to do, to make sure components such as bin/lld aren't mixed up with full paths.
There was a problem hiding this comment.
I've added a doc comment on that CMakeBuildQuery property explicitly mentioning relative paths, which I hope clarifies things a bit.
| let buildDirectory = self.sourcesDirectory.appending("build") | ||
| try await Shell.run("ninja", currentDirectory: buildDirectory) | ||
|
|
||
| return self.outputBinarySubpath.reduce(into: buildDirectory) { $0.append($1) } |
There was a problem hiding this comment.
The thing that originally drew my attention here was that I found the reduce(into:) hard to read.
The following still seems clearer to me, and it seems to be be able to append [FilePath.Component] - presumably using appending<C>(_ components: C)
https://developer.apple.com/documentation/system/filepath/appending(_:)-60fwk
However it's just a style suggestion, and I think it's fine to merge this now so the generator branch doesn't accumulate too many PRs.
| return self.outputBinarySubpath.reduce(into: buildDirectory) { $0.append($1) } | |
| return buildDirectory.appending(self.outputBinarySubpath) |
There was a problem hiding this comment.
Great, thanks, I'll update this to make it more readable! I'd be happy to merge after that, but prefer to merge PRs separately, so I need a review on the engine PR at the start of this PR chain: #31
There was a problem hiding this comment.
I see. I wondered if you were going to merge them all back down into max/engine and keep that branch going for a while, but I see you have now addressed Johannes's comments.
There was a problem hiding this comment.
Yeah, I prefer merging PRs from a stack separately, especially as I stick to "Squash and merge" that allows to preserve history of each PR in separate commits.
…/cache-lld # Conflicts: # Package.resolved # Package.swift # Sources/GeneratorEngine/Cache/CacheKeyProtocol.swift # Sources/GeneratorEngine/Cache/FileCacheRecord.swift # Sources/GeneratorEngine/Cache/SQLite.swift # Sources/SwiftSDKGenerator/Generator/SwiftSDKGenerator+Download.swift # Tests/GeneratorEngineTests/EngineTests.swift
|
@swift-ci test |
Depends on #34.
This proceeds with converting more of the generation steps to a query-based architecture. Now
llddoesn't need to be rebuilt if its source directory and build options don't change. This is implemented with a newCMakeBuildQuery, which takes source directory, and build options as arguments, and returns a freshly-built binary that can be cached by the generator engine.Also renamed
DownloadQuerytoDownloadArtifactQueryand moved it to a separate file.