8000 [Downloader::Git] Cache repos by fabiopelosin · Pull Request #248 · CocoaPods/CocoaPods · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[Downloader::Git] Cache repos #248

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 9 commits into from
May 9, 2012
Merged

[Downloader::Git] Cache repos #248

merged 9 commits into from
May 9, 2012

Conversation

fabiopelosin
Copy link
Member

I think that we could take advantage of Git to reduce execution times and bandwidth usage.

The idea is to store a clone of the remote somewhere in the disk and the update and clone this copy as a cache.

* master:
  [Command::Spec::Linter] Fix - Skip documentation generation
  [Config#doc] Renamed to generate_docs
  [Command::Spec::Linter] Bug fix.
* master:
  [Installer] Don't generate documenation if already installed
@alloy
Copy link
Member
alloy commented May 3, 2012

True, and it seems that the logic for it is not complex, so I’m inclined to agree with this feature.

Just keep in mind that in the future we may switch to a real package system, i.e. have our own web service that stores and distributes packages with metadata, making us free from any SCM details, so try to keep this stuff as simple as possible.

Why did you remove the github tarball code btw?

@fabiopelosin
Copy link
Member Author

Just keep in mind that in the future we may switch to a real package system, i.e. have our own web service that stores and distributes packages with metadata, making us free from any SCM details, so try to keep this stuff as simple as possible.

I opened the pull only because it is a simple patch as I consider caching the pods playing with fire :-). Luckily git does all the heavy work.

Why did you remove the github tarball code btw?

I assumed the tarball logic was there to speed up downloads avoiding the whole git metadata. Leaving it would skip the proposed cache, which I think is way faster because once you download a pod any other version is a maximun an incremental git download.

@alloy
Copy link
Member
alloy commented May 3, 2012

I opened the pull only because it is a simple patch as I consider caching the pods playing with fire :-). Luckily git does all the heavy work.

True, git is pretty good at it.

Why did you remove the github tarball code btw?

I assumed the tarball logic was there to speed up downloads avoiding the whole git metadata. Leaving it would skip the proposed cache, which I think is way faster because once you download a pod any other version is a maximun an incremental git download.

Hmm, actually, I don’t know what the exact use case was for introducing it, @lukeredpath can you explain it?

- ensure that cache git repos are clean
- added cache size limit
* master:
  [Command::Spec::Linter] fix for #podspec_warnings.
  [Command::Spec] Template layout fix.
  [Platform] Removed options accessor.
  [Executable] Made executed command more prominent.
  Spelling correctioSpelling correctionn
  [Resolver] Support for deployment target check.
  [Specification#deployment_target=] Switch to Pod::Version
  [Platform#==] Check for deployment target.
  [Platform] Simplify deployment target specification.
  [#212] Check for deployment target
@fabiopelosin
Copy link
Member Author

@alloy @lukeredpath Any update on this? It speeds up a lot testing podfiles.

@alloy
Copy link
Member
alloy commented May 7, 2012

@irrationalfab I think the cache feature is a good idea!

Regarding the git zipballs, that’s really up-to @lukeredpath to answer, so, for now, I would say let’s leave that in.

@fabiopelosin
Copy link
Member Author

@alloy The problem is that if I leave the github tarball code this conditional intercepts all the github urls rendering the cache useless in 99% of the cases. Unless I misunderstood you and you are suggesting to leave the code and remove the conditional, but in that case the GitHub tarball is not used by anything.

@lukeredpath
Copy link
Contributor

The motiviation behind the Github downloader using the tarball was for speed.

I'd rather it was left in - it's not true that any github URLs will bypass the cache as the tarball download behaviour only happens if the :download_only option is true. If it isn't, then it should defer to its superclass (the Git downloader).

If, in any instance its not doing this, then that should be considered a bug.

@fabiopelosin
Copy link
Member Author

@lukeredpath Thank you for the reply and for the correction. I should have been more meticulous.

I've restored the the Pod::Downloader::Git::GitHub class and now I'll merge.

fabiopelosin added a commit that referenced this pull request May 9, 2012
[Downloader::Git] Cache repos
@fabiopelosin fabiopelosin merged commit 0d4dd44 into master May 9, 2012
@alloy
Copy link
Member
alloy commented May 9, 2012

@irrationalfab A couple more questions:

  • Why do we need a configurable cache size? (instead of just on/off)
  • I don't think we should have the cache in /var/tmp/CocoaPods/Git, but instead maybe something like ~/Library/Caches/CocoaPods?
  • I guess I missed this earlier, but I’d like to see a few specs for this please :)

@fabiopelosin
Copy link
Member Author

Why do we need a configurable cache size? (instead of just on/off)

Good point. Not sure even if we need on/off actually. I was thinking of reactivating the cache in the integration specs tests the whole thing.

I don't think we should have the cache in /var/tmp/CocoaPods/Git, but instead maybe something like ~/Library/Caches/CocoaPods?

Agreed as we purge the cache. The original ideas was to have something like homebrew that justs keeps adding caches (irrc).

I guess I missed this earlier, but I’d like to see a few specs for this please :)

Sure I was just thinking of a good way to do them. The TTD is slowing entering in my mindset thanks to Ruby :-)

@fabiopelosin
Copy link
Member Author

@alloy Do you think that I ask too much from people if I set the cache to 1Gb?

@alloy
Copy link
Member
alloy commented May 9, 2012

@irrationalfab I really don’t think we need to worry about it all atm. Does it grow that fast?

@fabiopelosin
Copy link
Member Author

I really don’t think we need to worry about it all atm. Does it grow that fast?

@alloy Sorry my bad, in the past the specs used to saturate the cache and I thought that this was the problem, instead is that setting the max cache size to 0 nukes everything.

I have removed any configuration (lets see if somebody complains) and enabled the cache in the specs (It shaves 1/2 mins).

@alloy
Copy link
Member
alloy commented May 10, 2012

Sorry my bad

No need for that :)

I have removed any configuration (lets see if somebody complains) and enabled the cache in the specs (It shaves 1/2 mins).

Yeah I’d rather have a complaint first, before we make it more complex for ourselves :)

Sweet, good work!

jzapater pushed a commit to jzapater/CocoaPods that referenced this pull request Sep 17, 2013
fabiopelosin added a commit that referenced this pull request Oct 25, 2014
[Downloader::Git] Cache repos
amorde pushed a commit that referenced this pull request Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0