Skip to content

Cache DownloadableArtifacts with GeneratorEngine #34

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

Merged
merged 20 commits into from
Nov 3, 2023

Conversation

MaxDesiatov
Copy link
Contributor

@MaxDesiatov MaxDesiatov commented Oct 30, 2023

Depends on #31.

New GeneratorEngine allows us to cache all items in DownloadableArtifacts in a generic way and to allow redundant re-downloads for artifacts that don't have their checksums hardcoded (e.g. LLVM sources for lld).

This now allows us to delete all hardcoded checksums and also to cache other artifacts in the future, like lld binary and even whole artifact bundles.

@MaxDesiatov MaxDesiatov added the enhancement New feature or request label Oct 30, 2023
let isPrebuilt: Bool
}

let hostSwift: Item
private(set) var hostLLVM: Item
let targetSwift: Item

let allItems: [Item]
private let shouldUseDocker: Bool
var allItems: [Item] {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing a bug here, where allItems wasn't recomputed after running useLLVMSources.

Comment on lines +101 to +107
do {
try await generator.generateBundle(shouldGenerateFromScratch: !self.incremental)
try await generator.shutDown()
} catch {
try await generator.shutDown()
throw error
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice add a withGenerator wrapper which could take care of shutting down and rethrowing if necessary.

private let knownLLVMBinariesVersions: [ArtifactOS: [String: CPUMapping]] = [
.macOS: [
"15.0.7": [
Triple.CPU.arm64: "867c6afd41158c132ef05a8f1ddaecf476a26b91c85def8e124414f9a9ba188d",
Copy link
Contributor

Choose a reason for hiding this comment

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

A side effect of using these hashes was that we also verified that the upstream tarballs hadn't changed from the known versions. LLVM signs its binaries using GPG, so in future we could have an option to verify those signatures. This would require the user to download and validate LLVM's public key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, checking signatures is the right way to do it. But code on main that handled checksums was inconsistent and didn't warn users on checksums mismatch. It would only redownload new archives from remote URLs on a mismatch or if a checksum was absent. From that perspective, we didn't regress, and the caching behaviour is actually improved.

@MaxDesiatov MaxDesiatov self-assigned this Nov 1, 2023
Base automatically changed from maxd/engine to main November 3, 2023 10:16
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov MaxDesiatov enabled auto-merge (squash) November 3, 2023 10:37
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov MaxDesiatov merged commit 1ef53e8 into main Nov 3, 2023
@MaxDesiatov MaxDesiatov deleted the maxd/engine-downloads branch November 3, 2023 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants