-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Issue #2400 Shared cache for built dependencies #2716
base: master
Are you sure you want to change the base?
Conversation
Nice one @kenji21! How about some details in |
LGTM. My only concern is that this would be enabled by default. How does @Carthage/carthage feel about it? |
Hello. Merging of this PR can reduce our clean/build time from 22minutes to 5-6 minutes, our team really waits for it. |
For clarity, (as I understand it) this allows Carthage to share already built dependencies across projects. We have a workspace that has |
That's the point, here is how @blender nicely recap it : #2400 (comment) |
Anybody coming across this issue nowadays, please checkout my comment here. |
@mdiep ping for review :) |
@mdiep double ping ;) |
Looks pretty good overall, but I think we need a couple more items. |
02cdd14
to
a14f503
Compare
Hi, I updated code responding to reviews, and rebased my feature branch on top of current master, after a 3m1.871s for initial Full log:
|
a14f503
to
a6eb330
Compare
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Oups, didn't saw that CI was failing, I will update my commit again |
Source/CarthageKit/Xcode.swift
Outdated
/// ~/Library/Caches/org.carthage.CarthageKit/SharedCache/9.4.1_9F2000/SwiftyUserDefaults/3.0.1 | ||
internal func sharedCacheURLByXcodeVersionFor(dependency: Dependency, pinnedVersion: PinnedVersion) -> URL { | ||
let sharedCacheURL = Constants.userCachesURL.appendingPathComponent("SharedCache") | ||
let sharedCachePerXcode = sharedCacheURL.appendingPathComponent(XcodeVersion.makeString(), isDirectory: true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you'll want to use the swift toolchain version, not the xcode version. You could switch toolchains within xcode leading to incompatible builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're right, i've replaced Xcode version with the toolchain one if it is set (kept XcodeVersion if no toolchain set in BuildOptions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new version visible here : 23d433f#diff-f66644e8cfbcad3c3826395a690ddfbcR12
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just use the xcrun swift --version like other places in the code? Even if not set explicitly (default) it will yield the correct swift toolchain version used.
9c21079
to
23d433f
Compare
So two commits behind master, current tests are fine |
5c60486
to
4f2f56b
Compare
rebased to current master |
The reason I don't want to zip / unzip is that APFS makes copy almost instantly, so when a dependency at is compiled at version X.Y with swift version Y.Z for one project, it is available for another project using the same dependency (let's say Alamofire, which can be in many project for example) The other aim of the "shared cache" is to speed up Continuous Integration "clean" builds, by not having to recompile again and again dependencies that are not changing (see results in comment : #2400 (comment)) we discussed this in issue #2400 specifically well defined in this comment: #2400 (comment) |
@jdhealy do you want to give an opinion? I'll be reviewing again ASAP. |
This PR fails to build against official Carthage master because it's been updated for Swift 5.1 already.
Anyway, thanks for the good work. Build against Xcode 10.1 straight from the PR branch. Works like a charm! Regards |
4f2f56b
to
e9a33a6
Compare
it seems that using Xcode 10.2.1 wasn't a good idea... CI is down... |
e9a33a6
to
8e548ca
Compare
updated to current master, running
|
8e548ca
to
0570cb8
Compare
PR seems to be removed from the roadmap again. Any reason why? |
774b756
to
fe1e041
Compare
fe1e041
to
f61f66e
Compare
see discussion in issue #2400