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

Cocoapods 1.9 #619

Merged
merged 10 commits into from May 17, 2020
Merged

Cocoapods 1.9 #619

merged 10 commits into from May 17, 2020

Conversation

djbe
Copy link
Member

@djbe djbe commented Mar 1, 2019

2 things of note:

  • CDN sources, which allows us to get rid of the (still slow) step of downloading the full repo. We can now also cache the specs folder as it isn't as massive.
  • generate_multiple_pod_projects and incremental_installation, which should reduce the amount of changes during pod install/update.

@SwiftGen-Eve
Copy link

SwiftGen-Eve commented Mar 1, 2019

2 Warnings
⚠️ Big PR
⚠️ You should make sure that Podfile.lock and Package.resolved are changed in sync

Hey 👋 I'm Eve, the friendly bot watching over SwiftGen 🤖

Thanks a lot for your contribution!


Once you fix those tiny nitpickings above, we should be good to go! 🙌

ℹ️ I will update this comment as you add new commits

Generated by 🚫 Danger

@djbe
Copy link
Member Author

djbe commented Mar 1, 2019

Would you look at that, 4 seconds (without cache) 😆 🚗💨

Copy link
Collaborator

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also remove the deprecated .swift-version from the repo and use the swift_versions DSL attribute in the podspec instead

@djbe
Copy link
Member Author

djbe commented Mar 2, 2019

Uhm we can add the swift_versions bit, but some users seem to need that file to be there, see #555.

@djbe djbe force-pushed the feature/cocoapods-1.7-beta branch 2 times, most recently from d461d96 to 1db0c28 Compare March 9, 2019 13:49
@djbe djbe force-pushed the feature/cocoapods-1.7-beta branch from 1db0c28 to 6b1555c Compare April 12, 2019 20:59
@djbe djbe changed the title Cocoapods 1.7 Beta [WIP] Cocoapods 1.8 Nov 24, 2019
@djbe djbe marked this pull request as ready for review November 24, 2019 23:42
@djbe djbe changed the title Cocoapods 1.8 Cocoapods 1.9 Feb 27, 2020
@djbe djbe requested review from AliSoftware and a team March 25, 2020 14:46
@djbe djbe mentioned this pull request Mar 25, 2020
Copy link
Collaborator

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the fact that we should stop probably caching the CDN (and thus winning some more CI time by avoiding to restore it when not needed), LGTM

- save_cache:
key: cocoapods-{{ checksum "Podfile.lock" }}
paths:
- ~/.cocoapods
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With CDN, is it really still worth it to cache the ~/.cocoapods folder? From experience on other repos it doesn't really seem to be much necessary anymore.

We already commit the Pods/ folder (and if we didn't, we could cache it) so it should be quite rare to have the need for the spec repo to be present and downloaded locally anyway, as it is now supposed to only be necessary during pod update (or if pod install needs to actually install pods that were missing in Pods/ and were not at a version that was matching was was locked in Podfile.lock, which in our case should never happen on CI)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iow, if you remove those steps from CI, and as long as we still commit the Pods/ folder and that Podfile.lock and Pods/Manifest.lock are properly in sync, I think pod install is now a no-op that shouldn't even require the CDN to be cloned at all.
Maybe I'm wrong so don't hesitate to make some tests to validate that if unsure.

@@ -4,19 +4,19 @@ source 'https://rubygems.org'

# The bare minimum for building, e.g. in Homebrew
group :build do
gem 'rake', '~> 12.3'
gem 'rake', '~> 13.0'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 This should get rid of the security risk issue too 👍

Copy link
Collaborator

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the risk of using cache for specs repo was that cache restoration (download+unzip) and save (zip + upload) would risk taking more time than cloning the CDN. But after looking into it, I see that cache restoration only takes 3s. So we might as well keep it

@AliSoftware AliSoftware merged commit eb0853d into develop May 17, 2020
@djbe djbe deleted the feature/cocoapods-1.7-beta branch May 17, 2020 19:58
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