Add multiple cache storages feature #156

ikesyo · 2024-11-08T02:10:53Z

Resolves #148.

Runner.Options.CacheMode is replaced with [Runner.Options.CachePolicy]
- cacheMode: .disabled -> cachePolicies: []
- cacheMode: .project -> cachePolicies: [.project]
- cacheMode: .storage(storage, actors) -> cachePolicies: [.init(storage: storage, actors: actors)]
When saving, cache frameworks into all storages with CacheActorKind.producer
When restoring, storages with CacheActorKind.consumer are used in order
- If first storage failed to restore some caches, the failed ones are tried to be fetched from second (next) storage

- `Runner.Options.CacheMode.storages` is added - When saving, cache frameworks into all storages with `CacheActorKind.producer` - When restoring, storages with `CacheActorKind.consumer` are used in order - If first storage failed to restore some caches, the failed ones are tried to be fetched from second storage

ikesyo · 2024-11-11T05:41:18Z

Sources/ScipioKit/Producer/Cache/CacheSystem.swift

@@ -225,12 +229,6 @@ struct CacheSystem: Sendable {
        }
    }

-    private func fetchArtifacts(target: CacheTarget, to destination: URL) async throws {


This was unused.

ikesyo · 2024-11-11T05:43:16Z

Sources/ScipioKit/Producer/FrameworkProducer.swift

-            guard cacheMode.isConsumingCacheEnabled else {
-                return false
-            }


This is checked within restoreAllAvailableCachesIfNeeded so should not be needed.

ikesyo · 2024-11-11T07:01:25Z

Sources/ScipioKit/Producer/Cache/CacheSystem.swift

@@ -108,7 +108,6 @@ struct CacheSystem: Sendable {
    static let defaultParalellNumber = 8
    private let pinsStore: PinsStore
    private let outputDirectory: URL
-    private let storage: (any CacheStorage)?


Due to the introduction of cache policies, storages may be different on when saving caches and when restoring caches. So removed the property and passes by arguments instead.

ikesyo · 2024-11-11T07:03:12Z

Sources/ScipioKit/Producer/FrameworkProducer.swift

+        case .project:
+            // For `.project`, just checking whether the valid caches (already built frameworks under the project)
+            // exist or not (not restoring anything from external locations).
+            return await restoreCachesForTargets(
+                availableTargets,
+                cacheSystem: cacheSystem,
+                cacheStorage: nil
+            )


here:

Scipio/Sources/ScipioKit/Producer/FrameworkProducer.swift

Lines 245 to 259 in f6cbef1

if isValidCache && exists {

logger.info(

"✅ Valid \(product.target.name).xcframework (\(expectedCacheKeyHash)) is exists. Skip building.", metadata: .color(.green)

)

return true

} else {

if exists {

logger.warning("⚠️ Existing \(frameworkName) is outdated.", metadata: .color(.yellow))

logger.info("🗑️ Delete \(frameworkName)", metadata: .color(.red))

try fileSystem.removeFileTree(outputPath.absolutePath)

}

guard let cacheStorage else {

return false

}

ikesyo · 2024-11-11T07:06:27Z

Sources/ScipioKit/Producer/FrameworkProducer.swift

+            remainingTargets.subtract(restoredPerStorage)
+            if remainingTargets.isEmpty {
+                break
+            }


if all frameworks are successfully restored, we don't need to proceed to next cache storage.

[not mandatory] it's kindly to comment this

Sources/ScipioKit/Producer/Cache/CacheSystem.swift

giginet · 2024-11-12T02:54:36Z

Sources/ScipioKit/Producer/Cache/CacheSystem.swift

+            // Making the cache key compatible with 0.24.0 temporarily for easier debugging.
+            //
+            // TODO: revert this before merging
+            scipioVersion: "578ce98d236e79dad3e473cb11153e867be07174"


In the feature, we're better to add --disable-scipio-version-management option for this purpose.
It's useful for the development phase.

Disabling when developing is not a solution, because if excluding scipioVersion from cache keys, those are not compatible with the cache keys generated by release-versioned Scipio.

Ideally we should introduce some kind of Scipio's cache versioning and increment it when there are some incompatible changes to built frameworks (but it'll make Scipio's development and review process a bit complicated).

Sources/ScipioKit/Producer/FrameworkProducer.swift

Sources/scipio/scipio.docc/build-pipeline.md

Sources/ScipioKit/Runner.swift

Sources/ScipioKit/Producer/FrameworkProducer.swift

Sources/ScipioKit/Runner.swift

# Conflicts: # Sources/ScipioKit/Producer/FrameworkProducer.swift

Sources/ScipioKit/Runner.swift

giginet · 2024-11-12T09:33:06Z

Sources/scipio/scipio.docc/build-pipeline.md

+import ScipioS3Storage
+
+let s3Storage: some CacheStorage = ScipioS3Storage.S3Storage(config: ...)
+let localCacheStorage: some CacheStorage = LocalCacheStorage()


We can't instantiate LocalCacheStorage from external. Use CachePolicy.localDisk instead

LocalDiskCacheStorage is public type and can be initialized with custom cache directory. Do you mean that you'd like to make the type internal and expose the functionality through CachePolicy.localDisk only?

Yes!

Because we want to restrict their actors. e.g. for ProjectCache, users can only use [.producer] combination.

To conceal the storage constructed, we can prevent mistakes in usage.

giginet · 2024-11-13T01:21:17Z

Sources/ScipioKit/Producer/FrameworkProducer.swift

+        let storagesWithProducer = cachePolicies.storages(for: .producer)
+        if !storagesWithProducer.isEmpty {
+            await cacheSystem.cacheFrameworks(targets, to: storagesWithProducer)
+        }


[Q] I'm concerned that the way storages are handled is different for consumers and producers.

For consumers, storages are evaluated in order and the top ones are used.
On the other hand, producers try to write to all declared storages at the same time.

Is this difference in behavior intentional?

It's intentional. If we don't cache built frameworks to 2nd storages and later, the storages will not be able to restore the frameworks so saving to all storages will be required I think.

…cceeded` and `.failed` cases

giginet

Almost great!

Sources/ScipioKit/Runner.swift

Sources/ScipioKit/Producer/Cache/CacheSystem.swift

giginet · 2024-11-14T03:08:36Z

Sources/ScipioKit/Producer/FrameworkProducer.swift

+            remainingTargets.subtract(restoredPerStorage)
+            if remainingTargets.isEmpty {
+                break
+            }


[not mandatory] it's kindly to comment this

Sources/ScipioKit/Producer/FrameworkProducer.swift

giginet · 2024-11-14T05:00:10Z

Sources/ScipioKit/Producer/Cache/LocalDiskCacheStorage.swift

-        case custom(URL)
-    }
-
-    private let cacheDirectroy: CacheDirectory


👀 Directroy

This reverts commit bced0e2.

This reverts commit 0f1c98a and f6cbef1.

ikesyo added 2 commits November 8, 2024 15:03

Pass storages to CacheSystem.cacheFrameworks

5953d6c

ikesyo force-pushed the multiple-cache-storages branch from 9f7018b to 5953d6c Compare November 8, 2024 06:03

ikesyo added 4 commits November 8, 2024 15:30

Refactor to remove isProducingCacheEnabled

a24f7c9

Refactor to remove isConsumingCacheEnabled

d7bd9ff

Improve logging

e7b20c5

debugging

0f1c98a

ikesyo force-pushed the multiple-cache-storages branch from 106fc1b to 0f1c98a Compare November 8, 2024 08:38

Add test case for storages cache mode

8750153

ikesyo commented Nov 11, 2024

View reviewed changes

Update build-pipeline.md for .storages cache mode

8c7c16e

ikesyo force-pushed the multiple-cache-storages branch from ad5a600 to 8c7c16e Compare November 11, 2024 06:06

debugging

f6cbef1

ikesyo force-pushed the multiple-cache-storages branch from ae56332 to f6cbef1 Compare November 11, 2024 06:45

ikesyo commented Nov 11, 2024

View reviewed changes

ikesyo marked this pull request as ready for review November 11, 2024 07:08

ikesyo requested review from giuk-jung, mtj0928, freddi-kit and giginet November 11, 2024 07:08

giginet reviewed Nov 12, 2024

View reviewed changes

Add StorageConfig struct

413bb12

ikesyo mentioned this pull request Nov 12, 2024

Factor out the validation logic of existing frameworks in the output directory from Restorer #158

Merged

ikesyo added 7 commits November 12, 2024 15:52

Address naming convention issues

c3d9efa

Replace case storage with static func

25f5fcb

Replace case disabled with static func

82ec106

Merge branch 'main' into multiple-cache-storages

086970f

# Conflicts: # Sources/ScipioKit/Producer/FrameworkProducer.swift

Rename to LocalDiskCacheStorage

7434f6b

Introduce ProjectCacheStorage

a451614

Replace case project with static func (using ProjectCacheStorage)

c22ed6f

ikesyo force-pushed the multiple-cache-storages branch from 45b6089 to fa589b1 Compare November 12, 2024 08:28

Introduce CachePolicy and replace CacheMode with an array of CachePolicy

0006fbb

ikesyo force-pushed the multiple-cache-storages branch from fa589b1 to 0006fbb Compare November 12, 2024 08:45

ikesyo added 2 commits November 12, 2024 18:05

Update documentation

4d8abab

Avoid direct usages of type(of: storage)

76ae5af

giginet reviewed Nov 12, 2024

View reviewed changes

giginet reviewed Nov 13, 2024

View reviewed changes

ikesyo added 5 commits November 14, 2024 11:18

Make LocalDiskCacheStorage and ProjectCacheStorage internal

ff1d483

Use some over any for arguments

4712f89

[CI] macos-15 image is required to use Xcode 16

3d333e6

Add info log when a cache is not found when restoring as well as `.su…

0c81a2e

…cceeded` and `.failed` cases

Fix format

d71053b

giginet approved these changes Nov 14, 2024

View reviewed changes

giginet reviewed Nov 14, 2024

View reviewed changes

ikesyo added 5 commits November 14, 2024 15:43

Use some over any for arguments

ed52625

Add inline comment

4de26b5

Reduce duplication

bced0e2

Revert "Reduce duplication"

dfe443c

This reverts commit bced0e2.

Reduce duplication

acdd251

giginet approved these changes Nov 14, 2024

View reviewed changes

ikesyo added 2 commits November 14, 2024 17:40

Revert "debugging"

afafa8d

This reverts commit 0f1c98a and f6cbef1.

Add [Runner.Options.CachePolicy].disabled

2720cde

ikesyo force-pushed the multiple-cache-storages branch from 19be7d0 to 2720cde Compare November 15, 2024 08:11

giginet approved these changes Nov 18, 2024

View reviewed changes

ikesyo merged commit 6c1e30a into giginet:main Nov 18, 2024
2 checks passed

ikesyo deleted the multiple-cache-storages branch November 21, 2024 06:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add multiple cache storages feature #156

Add multiple cache storages feature #156

ikesyo commented Nov 8, 2024 •

edited

Loading

ikesyo Nov 11, 2024 •

edited

Loading

ikesyo Nov 11, 2024

ikesyo Nov 11, 2024 •

edited

Loading

ikesyo Nov 11, 2024

ikesyo Nov 11, 2024

giginet Nov 14, 2024

giginet Nov 12, 2024

ikesyo Nov 14, 2024 •

edited

Loading

giginet Nov 12, 2024

ikesyo Nov 12, 2024 •

edited

Loading

giginet Nov 13, 2024

giginet Nov 13, 2024

ikesyo Nov 14, 2024 •

edited

Loading

giginet left a comment

giginet Nov 14, 2024

giginet Nov 14, 2024

	if isValidCache && exists {
	logger.info(
	"✅ Valid \(product.target.name).xcframework (\(expectedCacheKeyHash)) is exists. Skip building.", metadata: .color(.green)
	)
	return true
	} else {
	if exists {
	logger.warning("⚠️ Existing \(frameworkName) is outdated.", metadata: .color(.yellow))
	logger.info("🗑️ Delete \(frameworkName)", metadata: .color(.red))
	try fileSystem.removeFileTree(outputPath.absolutePath)
	}

	guard let cacheStorage else {
	return false
	}

Add multiple cache storages feature #156

Add multiple cache storages feature #156

Conversation

ikesyo commented Nov 8, 2024 • edited Loading

ikesyo Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikesyo Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikesyo Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikesyo Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikesyo Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

giginet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikesyo commented Nov 8, 2024 •

edited

Loading

ikesyo Nov 11, 2024 •

edited

Loading

ikesyo Nov 11, 2024 •

edited

Loading

ikesyo Nov 14, 2024 •

edited

Loading

ikesyo Nov 12, 2024 •

edited

Loading

ikesyo Nov 14, 2024 •

edited

Loading