Task / Enable root package target caching #146
Conversation
| func testParseClangVersion() async throws { | ||
| let hook = { arguments in | ||
| XCTAssertEqual(arguments, ["/usr/bin/xcrun", "clang", "--version"]) | ||
| return StubbableExecutorResult(arguments: arguments, success: self.clangVersion) | ||
| } | ||
| let clangParser = ClangChecker(executor: StubbableExecutor(executeHook: hook)) | ||
| let version = try await clangParser.fetchClangVersion() | ||
| XCTAssertEqual(version, "clang-1400.0.29.102") | ||
| } |
There was a problem hiding this comment.
I hope it's fine that I extracted the Clang version checking logic into a separate test case.
giginet
requested review from
giginet,
giuk-jung,
ikesyo,
mtj0928 and
freddi-kit
| fileprivate extension ResolvedPackage { | ||
|
|
||
| var pin: PinsStore.Pin? { |
There was a problem hiding this comment.
It should be private extension or fileprivate func.
I prefer to add the accessor on the function sides.
| fileprivate extension ResolvedPackage { | |
| var pin: PinsStore.Pin? { | |
| extension ResolvedPackage { | |
| fileprivate var pin: PinsStore.Pin? { |
|
|
||
| fileprivate extension ResolvedPackage { | ||
|
|
||
| var pin: PinsStore.Pin? { |
There was a problem hiding this comment.
I feel pin is too generic a property name, even if it's a fileprivate extension.
This property generates a pin it is using GitRepository
How about it be makePinByRevision() or something?
There was a problem hiding this comment.
Agree, I like using the optional factory-like makePinFromRevision() method for that. I just slightly changed the preposition.
| #if compiler(>=6.0) | ||
| guard let pin = pinsStore.pins[package.identity] ?? package.makePinFromRevision() else { |
There was a problem hiding this comment.
[Question] I'm not sure why the addition is gated by #if compiler(>=6.0). Is that change not compatible with Swift 5.10 (Xcode 15.3)?
There was a problem hiding this comment.
I understand that GitRepository.getCurrentTag is added in Swift 6.0 👍
Add git information to PD context swiftlang/swift-package-manager#7202
Package manifests can now access information about the Git repository the given package is in via the context object's gitInformation property. This allows to determine the current tag (if any), the current commit and whether or not there are uncommited changes.
There was a problem hiding this comment.
Thanks for the question. I should've mentioned that in the PR description. There are two reasons for this: GitRepository.getCurrentTag is only available in SwiftPM's release/6.0 branch. Reimplementing the function (with the respective tests) would, of course, be trivial. And here comes the second reason, as I understand it, support for Xcode 15 is going to be dropped soon, so I thought it wouldn't be worth the effort. Let me know if it makes sense.
Update: I just saw your previous post where you figured it out yourself :)
There was a problem hiding this comment.
Good point. it is correct it will cause the incompatibility.
But I told to @dmhts that we don't need to maintain the backward compatibility.
#140 (comment)
So, I think it's no problem if the compatibility is broken because Swift 5 support will be dropped soon. We don't need to make an effort to keep backward compatibility.
I'll drop the Swift 5 support.
Motivation:
Currently,
Scipiodoes not support caching root package (local, if you will) targets. Apparently, this is because the version requirement is typically known for the root package dependencies, but not for the root package's direct targets themselves (as they're not pinned in the underlyingPinStore, for obvious reasons). However, such a version requirement can be easily derived from the git repository that "wraps" the root package, and this is what this PR is about. Moreover, SwiftPM requires each package to be within a repository with the respective version requirement set.Context:
We're using
Scipioonly with the.createPackagemode where the runner goes over a list of 3rd party packages. The issue is reproducible specifically in that mode, and you also can see it happening in the integration tests if run in the.createPackagemode with the caching flag enabled; the reason is that fixture packages are not wrapped in git repositories (which normally shouldn't happen in real life), otherwise, this PR fixes a potential issue there too.Additional considerations:
The change is purely additive and doesn't alter any existing functionality.
Expected result:
Before:
⚠️ Can't create caches for .../TestingPackage/MyTarget.xcframework
🚀 Cache MyTarget.xcframework to cache storage
After:
🚀 Cache MyTarget.xcframework to cache storage
[No warning as cache created successfuly]
@giginet as always, I'd love to hear your thoughts on this.