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

[Command::Project] No repo update by default on install #5017

Merged
merged 15 commits into from Mar 14, 2016

Conversation

Projects
None yet
3 participants
@mrackwitz
Member

mrackwitz commented Mar 10, 2016

Fixes #5004.

Will requires updates in because of the removal of Config.skip_repo_update:

mrackwitz added a commit that referenced this pull request Mar 10, 2016

@@ -8,7 +8,11 @@ To install release candidates run `[sudo] gem install cocoapods --pre`
##### Enhancements
* None.
* Running `pod install` doesn't imply an automatic spec repo update.

This comment has been minimized.

@segiddins

segiddins Mar 10, 2016

Member

Breaking imho

@@ -118,7 +112,14 @@ class Update < Command
CLAide::Argument.new('POD_NAMES', false, true),
]
def self.options
[
['--no-repo-update', 'Skip running `pod repo update` before install'],

This comment has been minimized.

@segiddins

segiddins Mar 10, 2016

Member

I thought we agreed pod update shouldn't update repos by default either?

This comment has been minimized.

@mrackwitz

mrackwitz Mar 10, 2016

Member

Did we? I thought we would keep it updating there.

@segiddins

This comment has been minimized.

Member

segiddins commented Mar 10, 2016

Doesn't

:skip_repo_update => false,
also need to be changed?

@segiddins

This comment has been minimized.

Member

segiddins commented Mar 10, 2016

This probably also warrants adding a --repo-update flag to pod outdated

@mrackwitz

This comment has been minimized.

Member

mrackwitz commented Mar 10, 2016

Doesn't

:skip_repo_update => false,
also need to be changed?

Yes, have seen that as well. The option doesn't really make sense anymore alone from it's naming, given that there two different defaults.

@segiddins

This comment has been minimized.

Member

segiddins commented Mar 10, 2016

Yeah, lets get rid of the config option

@segiddins

This comment has been minimized.

Member

segiddins commented Mar 10, 2016

@mrackwitz would you like me to pick this PR up?

mrackwitz added a commit to mrackwitz/integer that referenced this pull request Mar 12, 2016

@mrackwitz mrackwitz force-pushed the mr-no-repo-update-by-default branch from 70f0625 to 7dd57f8 Mar 12, 2016

mrackwitz added a commit that referenced this pull request Mar 12, 2016

@mrackwitz

This comment has been minimized.

Member

mrackwitz commented Mar 12, 2016

This probably also warrants adding a --repo-update flag to pod outdated

That existed already. I added now instead a --no-repo-update flag.

Yeah, lets get rid of the config option

The Installer has now instead a repo_update attribute. That will require some of our plugins to be updated. See the todo list in the top comment.

@mrackwitz mrackwitz changed the title from [Command::Project] No repo update by default on install to [WIP][Command::Project] No repo update by default on install Mar 12, 2016

@segiddins

This comment has been minimized.

Member

segiddins commented Mar 12, 2016

@mrackwitz maybe we make skip_repo_update an undocumented alias for the time being to avoid the breakage?

@mrackwitz

This comment has been minimized.

Member

mrackwitz commented Mar 12, 2016

@segiddins: We can't just define an undocumented alias as I removed the option completely from the Config and it's now only exposed in the Installer.

We could keep it there undocumented, print a warning whenever it is set, and initialize the installer with the configured value.

@segiddins

This comment has been minimized.

Member

segiddins commented Mar 12, 2016

@mrackwitz ah good point. No, in that case let's just break it now. I can make PRs updating the plugins once this is merged.

@@ -6,6 +6,17 @@ To install release candidates run `[sudo] gem install cocoapods --pre`
## Master
* Running `pod install` doesn't imply an automatic spec repo update.

This comment has been minimized.

@segiddins

segiddins Mar 12, 2016

Member

needs to be in a section

[#5004](https://github.com/CocoaPods/CocoaPods/issues/5004)
* Remove the configuration variable `skip_repo_update` as the default behavior
varies now between `pod install` and `pod (update|outdated)`.

This comment has been minimized.

@segiddins

segiddins Mar 12, 2016

Member

trailing two spaces

#
# @return [Installer]
#
def installer_for_config

This comment has been minimized.

@segiddins

segiddins Mar 12, 2016

Member

maybe def installer(config: self.config) so its overridable?

This comment has been minimized.

@mrackwitz

mrackwitz Mar 13, 2016

Member

I was trying to avoid calling it installer as disambiguation from local variable names. In advance I don't see any practical relevance for injecting a custom configuration.

The Xcode project file should be specified in your `Podfile` like this:
project 'path/to/XcodeProject'

This comment has been minimized.

@segiddins

segiddins Mar 12, 2016

Member

xcodeproj

@mrackwitz

This comment has been minimized.

Member

mrackwitz commented Mar 14, 2016

That's now ready for another review pass.

mrackwitz added a commit to CocoaPods/cocoapods-search that referenced this pull request Mar 14, 2016

@segiddins

This comment has been minimized.

Member

segiddins commented Mar 14, 2016

Specs are failing on Travis, seems to be due to the changes around chdir

mrackwitz added a commit to CocoaPods/cocoapods-packager that referenced this pull request Mar 14, 2016

mrackwitz added a commit to CocoaPods/cocoapods-packager that referenced this pull request Mar 14, 2016

@mrackwitz mrackwitz force-pushed the mr-no-repo-update-by-default branch from ce2597d to ec1e75e Mar 14, 2016

mrackwitz added a commit that referenced this pull request Mar 14, 2016

@mrackwitz

This comment has been minimized.

Member

mrackwitz commented Mar 14, 2016

Specs are failing on Travis, seems to be due to the changes around chdir

And that was not even necessary as run_command already ensures that the command is executed in the temporary directory. Fixed by deleting code.

module Options
def options
[
['--no-repo-update', 'Skip running `pod repo update` before install'],

This comment has been minimized.

@mrackwitz

mrackwitz Mar 14, 2016

Member

Another idea would be to generalize that further to --[no-]repo-update and interpret both variants here. So users wouldn't to memorize what is allowed per subcommand and could just enforce what they wish. It would also mean that we can easier change the default behavior, because both options are still supported. Actually both variants are already interpreted, the documented option should indicate the opposite of what the command does by default.

neonichu added a commit to CocoaPods/cocoapods-packager that referenced this pull request Mar 14, 2016

mrackwitz added a commit to CocoaPods/cocoapods-try that referenced this pull request Mar 14, 2016

mrackwitz added some commits Mar 14, 2016

[SpecHelper::TemporaryRepos] Fix issue with bacon
Methods are not properly inherited from contexts while setup and teardown callbacks are inherited. Fix that by calling #temorary_repos_path on the module.

@mrackwitz mrackwitz force-pushed the mr-no-repo-update-by-default branch from ec1e75e to 0054a3a Mar 14, 2016

@mrackwitz

This comment has been minimized.

Member

mrackwitz commented on 99d2ecf Mar 14, 2016

@alloy: Any ideas how to avoid such pesky runtime hackery?

This comment has been minimized.

Member

segiddins replied Mar 14, 2016

Switch away from bacon 😜

@mrackwitz

This comment has been minimized.

Member

mrackwitz commented on 99d2ecf Mar 14, 2016

@alloy: Any ideas how to avoid such pesky runtime hackery?

This comment has been minimized.

Member

segiddins replied Mar 14, 2016

Switch away from bacon 😜

@@ -90,7 +93,7 @@ def deprecated_pods
def spec_sets
@spec_sets ||= begin
analyzer.send(:update_repositories) unless config.skip_repo_update?
analyzer.send(:update_repositories) if repo_update?(:default => true)

This comment has been minimized.

@segiddins

segiddins Mar 14, 2016

Member

should this one really default to true? I can see pod outdated being used in scripts and stuff

This comment has been minimized.

@orta

orta Mar 14, 2016

Member

I think it makes sense to have pod outdated do the repo update - I really doubt it has the level of usage that install / update has

This comment has been minimized.

@mrackwitz

mrackwitz Mar 14, 2016

Member

I agree with @orta here. If you use it manually, you want probably always make sure that you're up-to-date.

@segiddins

This comment has been minimized.

Member

segiddins commented Mar 14, 2016

👍 other than the few questions. @orta could you please review this as well?

@orta

This comment has been minimized.

Member

orta commented Mar 14, 2016

cool, will look

@orta

This comment has been minimized.

Member

orta commented Mar 14, 2016

Aside form the pod outdated discussion, everything looks :shipit: to me

segiddins added a commit that referenced this pull request Mar 14, 2016

Merge pull request #5017 from CocoaPods/mr-no-repo-update-by-default
[Command::Project] No repo update by default on install

@segiddins segiddins merged commit 1f06a58 into master Mar 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@segiddins segiddins deleted the mr-no-repo-update-by-default branch Mar 14, 2016

segiddins added a commit to CocoaPods/cocoapods-search that referenced this pull request Mar 15, 2016

segiddins added a commit to CocoaPods/cocoapods-try that referenced this pull request Mar 15, 2016

@dantoml dantoml referenced this pull request May 4, 2016

Merged

[draft] Shallow clone post mortem. #129

1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment