Skip to content
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

Improve submodule performance when --use-submodules is not used #2795

Merged

Conversation

CosynPa
Copy link
Contributor

@CosynPa CosynPa commented May 26, 2019

By using the repository cache instead of directly cloning the submodules from remote.

Source/CarthageKit/Project.swift Outdated Show resolved Hide resolved
Source/CarthageKit/Git.swift Outdated Show resolved Hide resolved
Source/CarthageKit/Git.swift Outdated Show resolved Hide resolved
Source/CarthageKit/Git.swift Outdated Show resolved Hide resolved
Source/CarthageKit/Git.swift Show resolved Hide resolved
@stale
Copy link

stale bot commented Jun 28, 2019

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.

@stale stale bot added the stale label Jun 28, 2019
@stale stale bot closed this Jul 5, 2019
@tmspzz tmspzz reopened this Jul 5, 2019
@stale stale bot removed the stale label Jul 5, 2019
@CosynPa CosynPa force-pushed the submodule-performance-no-use-submodule branch from 3670c0f to 1fc4003 Compare July 16, 2019 09:40
@CosynPa
Copy link
Contributor Author

CosynPa commented Jul 16, 2019

To make Git.swift independent of the Carthage-specifics, I've moved git related procedure of cloneOrFetch to Git.swift. The movement is in one commit, and the modification of cloneOrFetch in another, so reviewing individual commits should be easier than reviewing the whole PR.

I also added a cacheURLMap parameter to cloneSubmoduleInWorkingDirectory, so it is able to know how a git URL is mapped to a cache repository path without introducing any Carthage-specifics into Git.swift.

@tmspzz
Copy link
Member

tmspzz commented Jul 19, 2019

@CosynPa

Do I understand correctly that you want to check if there is already a clone containing the branch & commit of the repository in Carthage's cache? If so then skip the cloning from remote?

@CosynPa
Copy link
Contributor Author

CosynPa commented Jul 20, 2019

Yes, and even if the commit does not exist in the cache repository, we only need to fetch in the cache repository, no need to clone the whole every time.

@CosynPa CosynPa force-pushed the submodule-performance-no-use-submodule branch from 1fc4003 to 8aebf21 Compare July 20, 2019 11:09
@CosynPa
Copy link
Contributor Author

CosynPa commented Jul 20, 2019

I removed the last commit which tried to solve the race condition problem because I found that commit did not solve the problem completely. The complete solution would be a little complex, so I think it's better to put it in another pull request.

@tmspzz
Copy link
Member

tmspzz commented Jul 25, 2019

@CosynPa please update the branch from master to fix the CI

@CosynPa CosynPa force-pushed the submodule-performance-no-use-submodule branch from 8aebf21 to 366a573 Compare July 26, 2019 04:26
@CosynPa
Copy link
Contributor Author

CosynPa commented Jul 26, 2019

Rebased onto master, but a test failed. Any idea?

@tmspzz
Copy link
Member

tmspzz commented Jul 26, 2019

Yes, it's a test trying to get the remote version. It's unrelated to you PR so for me this is good.

@tmspzz tmspzz merged commit 68abf2d into Carthage:master Jul 26, 2019
@tmspzz
Copy link
Member

tmspzz commented Jul 26, 2019

Thanks! 🎉

jdhealy added a commit that referenced this pull request Jun 17, 2020
jdhealy added a commit that referenced this pull request Jun 17, 2020
PRs «Fix fetching race condition (#2832)» and «Improve submodule performance when --use-submodules is not used (#2795)» provide benefits, but also major risk of cache clashes in the vein of <github.com//issues/569>. Basically, submodules before those two PRs had their bare git repos cached in way dissimilar to `~/Library/Caches/org.carthage.CarthageKit/dependencies` — that is they’re either “cached” how a git submodule init/fetch stores them or re-clone from remote with the subsequent checkout following a removal of the worktree.

This regression slipped past our unit (and small number of integration) tests. And, upon further testing out of popular frameworks in combination, caused major issues.

Follow up revert of associated commit <68abf2d73791720457f5be1aab4b57df86483f88> incoming.

This reverts commit 55e416a.
jdhealy added a commit that referenced this pull request Jun 17, 2020
…ed (#2795)"

PRs «Fix fetching race condition (#2832)» and «Improve submodule performance when --use-submodules is not used (#2795)» provide benefits, but also major risk of cache clashes in the vein of <github.com//issues/569>. Basically, submodules before those two PRs had their bare git repos cached in way dissimilar to `~/Library/Caches/org.carthage.CarthageKit/dependencies` — that is they’re either “cached” how a git submodule init/fetch stores them or re-clone from remote with the subsequent checkout following a removal of the worktree.

This regression slipped past our unit (and small number of integration) tests. And, upon further testing out of popular frameworks in combination, caused major issues.

This reverts commit 68abf2d.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants