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

Podfile.lock, pod update, pod outdated #447

Merged
merged 43 commits into from Aug 25, 2012

Conversation

Projects
None yet
6 participants
@fabiopelosin
Member

fabiopelosin commented Aug 8, 2012

No description provided.

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 8, 2012

This pull request fails (merged a4dbe7a into ab9b093).

travisbot commented Aug 8, 2012

This pull request fails (merged a4dbe7a into ab9b093).

@fabiopelosin

This comment has been minimized.

Show comment
Hide comment
@fabiopelosin

fabiopelosin Aug 8, 2012

Member

Tests are green and apparently this is not breaking existing behavior.

Member

fabiopelosin commented Aug 8, 2012

Tests are green and apparently this is not breaking existing behavior.

Show outdated Hide outdated lib/cocoapods/resolver.rb
@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Aug 8, 2012

Member

Sweet!!!!

One nitpick, a dictionary is called a hash in Ruby ;)

So you want to include this change in the RC together with the private/public headers change, right?

Member

alloy commented Aug 8, 2012

Sweet!!!!

One nitpick, a dictionary is called a hash in Ruby ;)

So you want to include this change in the RC together with the private/public headers change, right?

@fabiopelosin

This comment has been minimized.

Show comment
Hide comment
@fabiopelosin

fabiopelosin Aug 8, 2012

Member

One nitpick, a dictionary is called a hash in Ruby ;)

D'ho!

So you want to include this change in the RC together with the private/public headers change, right?

I've interpreted your comment as you were of the idea of releasing it. So the release 0.11.0 of today contains the private/public headers change.

Anyway this pull will definitely need RC there are going to be some important changes.

Member

fabiopelosin commented Aug 8, 2012

One nitpick, a dictionary is called a hash in Ruby ;)

D'ho!

So you want to include this change in the RC together with the private/public headers change, right?

I've interpreted your comment as you were of the idea of releasing it. So the release 0.11.0 of today contains the private/public headers change.

Anyway this pull will definitely need RC there are going to be some important changes.

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 9, 2012

This pull request fails (merged 33c9710 into ab9b093).

travisbot commented Aug 9, 2012

This pull request fails (merged 33c9710 into ab9b093).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 9, 2012

This pull request fails (merged c290bf9 into ab9b093).

travisbot commented Aug 9, 2012

This pull request fails (merged c290bf9 into ab9b093).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 9, 2012

This pull request fails (merged 0a244e6 into a24144f).

travisbot commented Aug 9, 2012

This pull request fails (merged 0a244e6 into a24144f).

@fabiopelosin

This comment has been minimized.

Show comment
Hide comment
@fabiopelosin

fabiopelosin Aug 9, 2012

Member

I think that I've opened the pull too early. Now we are in a good state where pod install, pod update, pod outdated work and the test are passing.


I didn't check the docs of bundler before implementing this patch because I didn't want to be influenced by it. Now I see that bundler takes a much simpler approach and the current behavior might be difficult to explain. So it is time to decide what those commands should actually do.

Current implementation

Note: if the folder of a resolved pod doesn't exist it is installed, any case (e.g. was deleted by the user.)

Pod Install
  • it installs new pods added to the podfile
  • it installs pods whose installed version is not compatible anymore with the podfile (specification changed by the user).
    • For example, if version 1.0 is installed changing the current requirements to < 1.1 doesn't triggers an install. On the other hand changing the requirements to > 1.1 will do.
  • it doesn't install deleted pods (the possibility to delete the folder is there but not implemented yet).
  • It doesn't update previously installed pods.
Pod Update
  • it installs pods with updates
  • it always installs head pods
  • it always installs pods from remote sources
  • it doesn't install new pods
  • it doesn't remove deleted pods

Bundlers way

Pod Install

Installs everything except already installed pods, if they are still compatible with the Podfile dependencies.

Pod Update

Make a new install regardless of the contents of Podfile.lock.

Memo for other tickets

  • The downloader should return the options necessary to restore the reference that it just downloaded so this information can be stored in the Lockfile and use for installs in other machines.
  • delete the folder of the removed Pods.
  • Add possibility to update only one pod, like pod update [NAME].

So @alloy what is your take?

Member

fabiopelosin commented Aug 9, 2012

I think that I've opened the pull too early. Now we are in a good state where pod install, pod update, pod outdated work and the test are passing.


I didn't check the docs of bundler before implementing this patch because I didn't want to be influenced by it. Now I see that bundler takes a much simpler approach and the current behavior might be difficult to explain. So it is time to decide what those commands should actually do.

Current implementation

Note: if the folder of a resolved pod doesn't exist it is installed, any case (e.g. was deleted by the user.)

Pod Install
  • it installs new pods added to the podfile
  • it installs pods whose installed version is not compatible anymore with the podfile (specification changed by the user).
    • For example, if version 1.0 is installed changing the current requirements to < 1.1 doesn't triggers an install. On the other hand changing the requirements to > 1.1 will do.
  • it doesn't install deleted pods (the possibility to delete the folder is there but not implemented yet).
  • It doesn't update previously installed pods.
Pod Update
  • it installs pods with updates
  • it always installs head pods
  • it always installs pods from remote sources
  • it doesn't install new pods
  • it doesn't remove deleted pods

Bundlers way

Pod Install

Installs everything except already installed pods, if they are still compatible with the Podfile dependencies.

Pod Update

Make a new install regardless of the contents of Podfile.lock.

Memo for other tickets

  • The downloader should return the options necessary to restore the reference that it just downloaded so this information can be stored in the Lockfile and use for installs in other machines.
  • delete the folder of the removed Pods.
  • Add possibility to update only one pod, like pod update [NAME].

So @alloy what is your take?

@fabiopelosin

This comment has been minimized.

Show comment
Hide comment
@fabiopelosin

fabiopelosin Aug 9, 2012

Member

Note of warning : This version of CocoaPods has a modified Podfile.lock which doesn't recognize the old Podfile.lock and skips it. This means that the first time pod install is run it will override all the existing Pods.

Member

fabiopelosin commented Aug 9, 2012

Note of warning : This version of CocoaPods has a modified Podfile.lock which doesn't recognize the old Podfile.lock and skips it. This means that the first time pod install is run it will override all the existing Pods.

@alloy alloy referenced this pull request Aug 10, 2012

Closed

RestKit 0.10.2 #454

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Aug 10, 2012

Member

Let’s see, if I understand it correctly, the difference between the current implementation and how Bundler does it is:

pod install

  • Current implementation does not delete pods removed from the Podfile, whereas Bundler does.

pod update

  • Current implementation does not install new pods, whereas Bundler does.
  • Current implementation does not delete pods removed from the Podfile, whereas Bundler does.

I’m leaning towards Bundler's approach. Like you say, it's much easier to explain, which in itself is a good indication that it's the better thing to do, especially if you’ll be able to update a single pod, people should be able to do everything they need, without having to think too much about it. I also can't imagine the case where you would want to install updated versions of pods you have installed, but not install new ones.

Imho, simpler is almost always better, especially if it's a user facing feature vs an internal one.

Member

alloy commented Aug 10, 2012

Let’s see, if I understand it correctly, the difference between the current implementation and how Bundler does it is:

pod install

  • Current implementation does not delete pods removed from the Podfile, whereas Bundler does.

pod update

  • Current implementation does not install new pods, whereas Bundler does.
  • Current implementation does not delete pods removed from the Podfile, whereas Bundler does.

I’m leaning towards Bundler's approach. Like you say, it's much easier to explain, which in itself is a good indication that it's the better thing to do, especially if you’ll be able to update a single pod, people should be able to do everything they need, without having to think too much about it. I also can't imagine the case where you would want to install updated versions of pods you have installed, but not install new ones.

Imho, simpler is almost always better, especially if it's a user facing feature vs an internal one.

@fabiopelosin

This comment has been minimized.

Show comment
Hide comment
@fabiopelosin

fabiopelosin Aug 10, 2012

Member

Install: Current implementation does not delete pods removed from the Podfile, whereas Bundler does.

It doesn't installs them but detects that were deleted. Just I'm not removing the folder for now for safety as the system is not proved to be reliable; there is an open ticket about it and I will implement it once tested.

The key difference is that it doesn't updates the Pods if they are still compatible with a changed dependency in the Podfile. If you wan't to update them you need to call pod update manually. I'm not sure of what bundler does in this case. Example.

pod "MyLib" = "= 1.0"
# Installs 1.0
pod "MyLib"
# Install keeps the 1.0 even if the 1.1 version is available. However `pod update` can update it.
pod "MyLib" ">1.0"
# 1.0 is not compatible anymore with the Podfile and 1.1 is installed.

In other words it locks dependencies to the installed version if still compatible with the new Podfile. The other alternative is when we detect a changed dependency in the podfile we could resolve the version regardless of the previous installed one.
With the current approach you need to be more explicit about your intentions. Makes sense?

I’m leaning towards Bundler's approach. Like you say, it's much easier to explain, which in itself is a good indication that it's the better thing to do, especially if you’ll be able to update a single pod, people should be able to do everything they need, without having to think too much about it. I also can't imagine the case where you would want to install updated versions of pods you have installed, but not install new ones.

Imho, simpler is almost always better, especially if it's a user facing feature vs an internal one.

Agreed.

Member

fabiopelosin commented Aug 10, 2012

Install: Current implementation does not delete pods removed from the Podfile, whereas Bundler does.

It doesn't installs them but detects that were deleted. Just I'm not removing the folder for now for safety as the system is not proved to be reliable; there is an open ticket about it and I will implement it once tested.

The key difference is that it doesn't updates the Pods if they are still compatible with a changed dependency in the Podfile. If you wan't to update them you need to call pod update manually. I'm not sure of what bundler does in this case. Example.

pod "MyLib" = "= 1.0"
# Installs 1.0
pod "MyLib"
# Install keeps the 1.0 even if the 1.1 version is available. However `pod update` can update it.
pod "MyLib" ">1.0"
# 1.0 is not compatible anymore with the Podfile and 1.1 is installed.

In other words it locks dependencies to the installed version if still compatible with the new Podfile. The other alternative is when we detect a changed dependency in the podfile we could resolve the version regardless of the previous installed one.
With the current approach you need to be more explicit about your intentions. Makes sense?

I’m leaning towards Bundler's approach. Like you say, it's much easier to explain, which in itself is a good indication that it's the better thing to do, especially if you’ll be able to update a single pod, people should be able to do everything they need, without having to think too much about it. I also can't imagine the case where you would want to install updated versions of pods you have installed, but not install new ones.

Imho, simpler is almost always better, especially if it's a user facing feature vs an internal one.

Agreed.

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Aug 10, 2012

Member

In other words it locks dependencies to the installed version if still compatible with the new Podfile.

I’ve just checked with Bundler and your examples is exactly how it does it and how I would expect it to work.

~/t/Test447 » cat Gemfile
source :rubygems
gem 'activesupport', '3.2.6'

~/t/Test447 » bundle install
Using i18n (0.6.0) 
Using multi_json (1.3.6) 
Using activesupport (3.2.6) 
Using bundler (1.1.4

~/t/Test447 » cat Gemfile.lock 
GEM
  remote: http://rubygems.org/
  specs:
    activesupport (3.2.6)
      i18n (~> 0.6)
      multi_json (~> 1.0)
    i18n (0.6.0)
    multi_json (1.3.6)

PLATFORMS
  ruby

DEPENDENCIES
  activesupport (= 3.2.6)
~/t/Test447 » cat Gemfile
source :rubygems
gem 'activesupport', '3.2.6'

~/t/Test447 » bundle install
Using i18n (0.6.0) 
Using multi_json (1.3.6) 
Using activesupport (3.2.6) 
Using bundler (1.1.4) 

~/t/Test447 » cat Gemfile.lock 
GEM
  remote: http://rubygems.org/
  specs:
    activesupport (3.2.6)
      i18n (~> 0.6)
      multi_json (~> 1.0)
    i18n (0.6.0)
    multi_json (1.3.6)

PLATFORMS
  ruby

DEPENDENCIES
  activesupport
~/t/Test447 » cat Gemfile
source :rubygems
gem 'activesupport', '>= 3.2.7'

~/t/Test447 » bundle install
Fetching gem metadata from http://rubygems.org/........
Using i18n (0.6.0) 
Using multi_json (1.3.6) 
Installing activesupport (3.2.8) 
Using bundler (1.1.4) 

~/t/Test447 » cat Gemfile.lock 
GEM
  remote: http://rubygems.org/
  specs:
    activesupport (3.2.8)
      i18n (~> 0.6)
      multi_json (~> 1.0)
    i18n (0.6.0)
    multi_json (1.3.6)

PLATFORMS
  ruby

DEPENDENCIES
  activesupport (>= 3.2.7)
Member

alloy commented Aug 10, 2012

In other words it locks dependencies to the installed version if still compatible with the new Podfile.

I’ve just checked with Bundler and your examples is exactly how it does it and how I would expect it to work.

~/t/Test447 » cat Gemfile
source :rubygems
gem 'activesupport', '3.2.6'

~/t/Test447 » bundle install
Using i18n (0.6.0) 
Using multi_json (1.3.6) 
Using activesupport (3.2.6) 
Using bundler (1.1.4

~/t/Test447 » cat Gemfile.lock 
GEM
  remote: http://rubygems.org/
  specs:
    activesupport (3.2.6)
      i18n (~> 0.6)
      multi_json (~> 1.0)
    i18n (0.6.0)
    multi_json (1.3.6)

PLATFORMS
  ruby

DEPENDENCIES
  activesupport (= 3.2.6)
~/t/Test447 » cat Gemfile
source :rubygems
gem 'activesupport', '3.2.6'

~/t/Test447 » bundle install
Using i18n (0.6.0) 
Using multi_json (1.3.6) 
Using activesupport (3.2.6) 
Using bundler (1.1.4) 

~/t/Test447 » cat Gemfile.lock 
GEM
  remote: http://rubygems.org/
  specs:
    activesupport (3.2.6)
      i18n (~> 0.6)
      multi_json (~> 1.0)
    i18n (0.6.0)
    multi_json (1.3.6)

PLATFORMS
  ruby

DEPENDENCIES
  activesupport
~/t/Test447 » cat Gemfile
source :rubygems
gem 'activesupport', '>= 3.2.7'

~/t/Test447 » bundle install
Fetching gem metadata from http://rubygems.org/........
Using i18n (0.6.0) 
Using multi_json (1.3.6) 
Installing activesupport (3.2.8) 
Using bundler (1.1.4) 

~/t/Test447 » cat Gemfile.lock 
GEM
  remote: http://rubygems.org/
  specs:
    activesupport (3.2.8)
      i18n (~> 0.6)
      multi_json (~> 1.0)
    i18n (0.6.0)
    multi_json (1.3.6)

PLATFORMS
  ruby

DEPENDENCIES
  activesupport (>= 3.2.7)
@fabiopelosin

This comment has been minimized.

Show comment
Hide comment
@fabiopelosin

fabiopelosin Aug 10, 2012

Member

Nice, so I just need to change pod update. Btw, do you known what Bundler is using for saving its lockfile? It looks like a clean YAML.

Member

fabiopelosin commented Aug 10, 2012

Nice, so I just need to change pod update. Btw, do you known what Bundler is using for saving its lockfile? It looks like a clean YAML.

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Aug 10, 2012

Member

No idea, but isn’t our version yet?

Member

alloy commented Aug 10, 2012

No idea, but isn’t our version yet?

@fabiopelosin

This comment has been minimized.

Show comment
Hide comment
@fabiopelosin

fabiopelosin Aug 10, 2012

Member

Did you see my comment about the Podfile.lock incompatibility?

Note of warning : This version of CocoaPods has a modified Podfile.lock which doesn't recognize the old Podfile.lock and skips it. This means that the first time pod install is run it will override all the existing Pods.

Just to be sure.

No idea, but isn’t our version yet?

It is, I'm just looking to get it right and clean so we don't need to create incompatibilities in the future. Here is a sample:

PODS:
- A2DynamicDelegate (2.0.1):
  - libffi
- AFNetworking (1.0RC1)
- BlockAlertsAnd-ActionSheets (1.0.0)
- BlocksKit (1.5.1):
  - A2DynamicDelegate
- DACircularProgress (2.0.0)
- InnerBand (HEAD from 0.0.1)
- JMImageCache (0.2.1)
- JRSwizzle (1.0)
- MWFeedParser/NSString+HTML (HEAD from 0.0.1)
- NoticeView (2.1)
- Objective-C-HMTL-Parser (0.0.1)
- PrettyKit (0.2.0)
- ReactiveCocoa (0.6.0):
  - JRSwizzle (~> 1.0)
- SVPullToRefresh (HEAD from 0.2)
- TTTAttributedLabel (1.3.0)
- Three20 (1.0.11):
  - Three20/UI (= 1.0.11)
- Three20/Core (1.0.11)
- Three20/Network (1.0.11):
  - Three20/Core
- Three20/Style (1.0.11):
  - Three20/Core
  - Three20/Network
- Three20/UI (1.0.11):
  - Three20/Core
  - Three20/Network
  - Three20/Style
  - Three20/UICommon
  - Three20/UINavigator
- Three20/UICommon (1.0.11):
  - Three20/Core
- Three20/UINavigator (1.0.11):
  - Three20/Core
  - Three20/UICommon
- Underscore.m (0.1.0)
- libffi (3.0.11)

DEPENDENCIES:
- AFNetworking
- BlockAlertsAnd-ActionSheets
- BlocksKit
- DACircularProgress
- InnerBand (HEAD)
- JMImageCache (from `https://github.com/jakemarsh/JMImageCache.git')
- MWFeedParser/NSString+HTML (HEAD)
- NoticeView
- Objective-C-HMTL-Parser
- PrettyKit (defined in Podfile)
- ReactiveCocoa (= 0.6.0)
- SVPullToRefresh (HEAD)
- TTTAttributedLabel
- Three20
- Underscore.m (from `https://github.com/CocoaPods/Specs/raw/master/Underscore.m/0.1.0/Underscore.m.podspec')

EXTERNAL SOURCES:
- JMImageCache:
    :git: https://github.com/jakemarsh/JMImageCache.git
- Underscore.m:
    :podspec: https://github.com/CocoaPods/Specs/raw/master/Underscore.m/0.1.0/Underscore.m.podspec

COCOAPODS: 0.11.1
Member

fabiopelosin commented Aug 10, 2012

Did you see my comment about the Podfile.lock incompatibility?

Note of warning : This version of CocoaPods has a modified Podfile.lock which doesn't recognize the old Podfile.lock and skips it. This means that the first time pod install is run it will override all the existing Pods.

Just to be sure.

No idea, but isn’t our version yet?

It is, I'm just looking to get it right and clean so we don't need to create incompatibilities in the future. Here is a sample:

PODS:
- A2DynamicDelegate (2.0.1):
  - libffi
- AFNetworking (1.0RC1)
- BlockAlertsAnd-ActionSheets (1.0.0)
- BlocksKit (1.5.1):
  - A2DynamicDelegate
- DACircularProgress (2.0.0)
- InnerBand (HEAD from 0.0.1)
- JMImageCache (0.2.1)
- JRSwizzle (1.0)
- MWFeedParser/NSString+HTML (HEAD from 0.0.1)
- NoticeView (2.1)
- Objective-C-HMTL-Parser (0.0.1)
- PrettyKit (0.2.0)
- ReactiveCocoa (0.6.0):
  - JRSwizzle (~> 1.0)
- SVPullToRefresh (HEAD from 0.2)
- TTTAttributedLabel (1.3.0)
- Three20 (1.0.11):
  - Three20/UI (= 1.0.11)
- Three20/Core (1.0.11)
- Three20/Network (1.0.11):
  - Three20/Core
- Three20/Style (1.0.11):
  - Three20/Core
  - Three20/Network
- Three20/UI (1.0.11):
  - Three20/Core
  - Three20/Network
  - Three20/Style
  - Three20/UICommon
  - Three20/UINavigator
- Three20/UICommon (1.0.11):
  - Three20/Core
- Three20/UINavigator (1.0.11):
  - Three20/Core
  - Three20/UICommon
- Underscore.m (0.1.0)
- libffi (3.0.11)

DEPENDENCIES:
- AFNetworking
- BlockAlertsAnd-ActionSheets
- BlocksKit
- DACircularProgress
- InnerBand (HEAD)
- JMImageCache (from `https://github.com/jakemarsh/JMImageCache.git')
- MWFeedParser/NSString+HTML (HEAD)
- NoticeView
- Objective-C-HMTL-Parser
- PrettyKit (defined in Podfile)
- ReactiveCocoa (= 0.6.0)
- SVPullToRefresh (HEAD)
- TTTAttributedLabel
- Three20
- Underscore.m (from `https://github.com/CocoaPods/Specs/raw/master/Underscore.m/0.1.0/Underscore.m.podspec')

EXTERNAL SOURCES:
- JMImageCache:
    :git: https://github.com/jakemarsh/JMImageCache.git
- Underscore.m:
    :podspec: https://github.com/CocoaPods/Specs/raw/master/Underscore.m/0.1.0/Underscore.m.podspec

COCOAPODS: 0.11.1
@fabiopelosin

This comment has been minimized.

Show comment
Hide comment
@fabiopelosin

fabiopelosin Aug 10, 2012

Member

@alloy One last question is it fine for pod update to remove pods deleted form the Podfile?

Member

fabiopelosin commented Aug 10, 2012

@alloy One last question is it fine for pod update to remove pods deleted form the Podfile?

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Aug 10, 2012

Member

Did you see my comment about the Podfile.lock incompatibility?

Yeah, I did see it, but then forgot about it :)

What part is incompatible exactly and why?

It is, I'm just looking to get it right and clean so we don't need to create incompatibilities in the future. Here is a sample:

What about the duplication of the external sources? Can we remove that from the DEPENDENCIES section and instead say something like - Underscore.m (external)?

One last question is it fine for pod update to remove pods deleted form the Podfile?

Yeah, pod update should just do a complete install as if there were no Podfile.lock file.

Member

alloy commented Aug 10, 2012

Did you see my comment about the Podfile.lock incompatibility?

Yeah, I did see it, but then forgot about it :)

What part is incompatible exactly and why?

It is, I'm just looking to get it right and clean so we don't need to create incompatibilities in the future. Here is a sample:

What about the duplication of the external sources? Can we remove that from the DEPENDENCIES section and instead say something like - Underscore.m (external)?

One last question is it fine for pod update to remove pods deleted form the Podfile?

Yeah, pod update should just do a complete install as if there were no Podfile.lock file.

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 13, 2012

This pull request passes (merged 9cbb2df into be222a0).

travisbot commented Aug 13, 2012

This pull request passes (merged 9cbb2df into be222a0).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 14, 2012

This pull request fails (merged 15e9f3b into be222a0).

travisbot commented Aug 14, 2012

This pull request fails (merged 15e9f3b into be222a0).

@fabiopelosin

This comment has been minimized.

Show comment
Hide comment
@fabiopelosin

fabiopelosin Aug 14, 2012

Member

What part is incompatible exactly and why?

Now only the external sources. As the Resolver checks if they changed to decide whether to install them and we were not storing their information in the past now they get installed on the first call to pod install. Apparently nothing else gets reinstalled.

What about the duplication of the external sources? Can we remove that from the DEPENDENCIES section and instead say something like - Underscore.m (external)?

For simplicity I'm using the to_s method of Dependency (so the resolver needs to be less aware of the specifics of its internals), this can be easily done if we change also the output of the verbose command. Otherwise we could have a another method that prepares it for serialization. What do you think?


Finally I'm looking to fine tune the output of the Lockfile:

  • I removed our custom writer in favor of the YAML built in, However I'm not sure that it was a good choice.
  • I've inspected bundler and they use a custom (not so big actually) parser, that's why their lockfile looks more clean, however I'm not sure if it is worth it.
  • currently the checksum of the podspecs gets serialized as a ugly binary by the YAML class and I'm not sure why.
  • ugliness in the symbols of the external sources.

A comparison of the previous Lockfile and the new one is available here.

@alloy Let me know what is your take on this.


A part from those minor details I think that the patch is ready for testing. What do you think?

Member

fabiopelosin commented Aug 14, 2012

What part is incompatible exactly and why?

Now only the external sources. As the Resolver checks if they changed to decide whether to install them and we were not storing their information in the past now they get installed on the first call to pod install. Apparently nothing else gets reinstalled.

What about the duplication of the external sources? Can we remove that from the DEPENDENCIES section and instead say something like - Underscore.m (external)?

For simplicity I'm using the to_s method of Dependency (so the resolver needs to be less aware of the specifics of its internals), this can be easily done if we change also the output of the verbose command. Otherwise we could have a another method that prepares it for serialization. What do you think?


Finally I'm looking to fine tune the output of the Lockfile:

  • I removed our custom writer in favor of the YAML built in, However I'm not sure that it was a good choice.
  • I've inspected bundler and they use a custom (not so big actually) parser, that's why their lockfile looks more clean, however I'm not sure if it is worth it.
  • currently the checksum of the podspecs gets serialized as a ugly binary by the YAML class and I'm not sure why.
  • ugliness in the symbols of the external sources.

A comparison of the previous Lockfile and the new one is available here.

@alloy Let me know what is your take on this.


A part from those minor details I think that the patch is ready for testing. What do you think?

@fabiopelosin

This comment has been minimized.

Show comment
Hide comment
@fabiopelosin

fabiopelosin Aug 16, 2012

Member

@alloy Did you miss the the previous comment?

Member

fabiopelosin commented Aug 16, 2012

@alloy Did you miss the the previous comment?

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Aug 20, 2012

Member

@irrationalfab Yes I missed it, sorry about that.

For simplicity I'm using the to_s method of Dependency (so the resolver needs to be less aware of the specifics of its internals), this can be easily done if we change also the output of the verbose command. Otherwise we could have a another method that prepares it for serialization. What do you think?

Meh, it’s not that important for now, we can clean that up later.

Finally I'm looking to fine tune the output of the Lockfile:

  • I removed our custom writer in favor of the YAML built in, However I'm not sure that it was a good choice.

What was the reason you decided to replace it and why are you unsure if it was a good idea?

  • I've inspected bundler and they use a custom (not so big actually) parser, that's why their lockfile looks more clean, however I'm not sure if it is worth it.

We don’t need a custom language, just YAML is good enough imo, that way we can just use the existing parser.

  • currently the checksum of the podspecs gets serialized as a ugly binary by the YAML class and I'm not sure why.

This seems to be an issue on Ruby 1.9 only. It can be fixed by encoding the result string as UTF-8:

irb(main):015:0> Digest::SHA1.hexdigest('secret').to_yaml
=> "--- !binary |-\n  ZTVlOWZhMWJhMzFlY2QxYWU4NGY3NWNhYWE0NzRmM2E2NjNmMDVmNA==\n"
irb(main):016:0> Digest::SHA1.hexdigest('secret').encode('UTF-8').to_yaml
=> "--- e5e9fa1ba31ecd1ae84f75caaa474f3a663f05f4\n...\n"
  • ugliness in the symbols of the external sources.

They could be converted to strings before/after serialising/deserialising, but I don’t think it’s a big enough issue to care about, nor do I find it really ugly myself :)

A part from those minor details I think that the patch is ready for testing. What do you think?

I’ll try the patch tonight!

Member

alloy commented Aug 20, 2012

@irrationalfab Yes I missed it, sorry about that.

For simplicity I'm using the to_s method of Dependency (so the resolver needs to be less aware of the specifics of its internals), this can be easily done if we change also the output of the verbose command. Otherwise we could have a another method that prepares it for serialization. What do you think?

Meh, it’s not that important for now, we can clean that up later.

Finally I'm looking to fine tune the output of the Lockfile:

  • I removed our custom writer in favor of the YAML built in, However I'm not sure that it was a good choice.

What was the reason you decided to replace it and why are you unsure if it was a good idea?

  • I've inspected bundler and they use a custom (not so big actually) parser, that's why their lockfile looks more clean, however I'm not sure if it is worth it.

We don’t need a custom language, just YAML is good enough imo, that way we can just use the existing parser.

  • currently the checksum of the podspecs gets serialized as a ugly binary by the YAML class and I'm not sure why.

This seems to be an issue on Ruby 1.9 only. It can be fixed by encoding the result string as UTF-8:

irb(main):015:0> Digest::SHA1.hexdigest('secret').to_yaml
=> "--- !binary |-\n  ZTVlOWZhMWJhMzFlY2QxYWU4NGY3NWNhYWE0NzRmM2E2NjNmMDVmNA==\n"
irb(main):016:0> Digest::SHA1.hexdigest('secret').encode('UTF-8').to_yaml
=> "--- e5e9fa1ba31ecd1ae84f75caaa474f3a663f05f4\n...\n"
  • ugliness in the symbols of the external sources.

They could be converted to strings before/after serialising/deserialising, but I don’t think it’s a big enough issue to care about, nor do I find it really ugly myself :)

A part from those minor details I think that the patch is ready for testing. What do you think?

I’ll try the patch tonight!

@fabiopelosin

This comment has been minimized.

Show comment
Hide comment
@fabiopelosin

fabiopelosin Aug 20, 2012

Member

What was the reason you decided to replace it and why are you unsure if it was a good idea?

Simplify the code by using the built in coder. I was unsure wether it was a good idea because the checksum issues and because we could have a little bit of more control on the formatting. However, as we will stay YAML compliant I think that we can clean up this part as well later, if necessary.

This seems to be an issue on Ruby 1.9 only. It can be fixed by encoding the result string as UTF-8:

Perfect!

I’ll try the patch tonight!

I'm looking forward to hear your impressions.

Member

fabiopelosin commented Aug 20, 2012

What was the reason you decided to replace it and why are you unsure if it was a good idea?

Simplify the code by using the built in coder. I was unsure wether it was a good idea because the checksum issues and because we could have a little bit of more control on the formatting. However, as we will stay YAML compliant I think that we can clean up this part as well later, if necessary.

This seems to be an issue on Ruby 1.9 only. It can be fixed by encoding the result string as UTF-8:

Perfect!

I’ll try the patch tonight!

I'm looking forward to hear your impressions.

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 20, 2012

This pull request fails (merged 0bc6cc4 into be222a0).

travisbot commented Aug 20, 2012

This pull request fails (merged 0bc6cc4 into be222a0).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 20, 2012

This pull request fails (merged 3d2b5a3 into be222a0).

travisbot commented Aug 20, 2012

This pull request fails (merged 3d2b5a3 into be222a0).

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Aug 20, 2012

Member

Unfortunately I won’t be able to finish checking this much awesomeness this evening ;)

But I was thinking that maybe we should release a new version first before we do an RC for this. Agreed?

Member

alloy commented Aug 20, 2012

Unfortunately I won’t be able to finish checking this much awesomeness this evening ;)

But I was thinking that maybe we should release a new version first before we do an RC for this. Agreed?

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Aug 20, 2012

Member

This should be ‘SPEC CHECKSUMS’ iirc. English isn’t my native tongue either, but I believe you pluralize the actual subject, which is ‘checksum’ in this case.

Member

alloy commented on lib/cocoapods/lockfile.rb in 3d2b5a3 Aug 20, 2012

This should be ‘SPEC CHECKSUMS’ iirc. English isn’t my native tongue either, but I believe you pluralize the actual subject, which is ‘checksum’ in this case.

@subdigital

This comment has been minimized.

Show comment
Hide comment
@subdigital

subdigital Aug 20, 2012

Contributor

Here's an area I can help with!

It depends on what is plural, yes. If it is many checksums, then "spec checksums" is correct. If it's a checksum of all of the specs, it would be "specs checksum".

Contributor

subdigital commented on 3d2b5a3 Aug 20, 2012

Here's an area I can help with!

It depends on what is plural, yes. If it is many checksums, then "spec checksums" is correct. If it's a checksum of all of the specs, it would be "specs checksum".

This comment has been minimized.

Show comment
Hide comment
@fabiopelosin

fabiopelosin Aug 20, 2012

Member

Good to know! Actually this is an area where I'm often in doubt. I'll change it to 'SPEC CHECKSUMS' as they are the checksums of all the specs.

@subdigital Is 'SPECS CHECKSUMS' a correct alternative?

Member

fabiopelosin replied Aug 20, 2012

Good to know! Actually this is an area where I'm often in doubt. I'll change it to 'SPEC CHECKSUMS' as they are the checksums of all the specs.

@subdigital Is 'SPECS CHECKSUMS' a correct alternative?

This comment has been minimized.

Show comment
Hide comment
@subdigital

subdigital Aug 20, 2012

Contributor
Contributor

subdigital replied Aug 20, 2012

This comment has been minimized.

Show comment
Hide comment
@fabiopelosin

fabiopelosin Aug 20, 2012

Member

That just sounds weird :)

I'm very glad of this discussion because that is a mistake that I often do. Thanks. :-)

Member

fabiopelosin replied Aug 20, 2012

That just sounds weird :)

I'm very glad of this discussion because that is a mistake that I often do. Thanks. :-)

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Aug 21, 2012

Member

:)

Member

alloy replied Aug 21, 2012

:)

@fabiopelosin

This comment has been minimized.

Show comment
Hide comment
@fabiopelosin

fabiopelosin Aug 20, 2012

Member

But I was thinking that maybe we should release a new version first before we do an RC for this. Agreed?

Yes, I think that it is a good idea. Isolating big patches in a single release makes bug tracking easier :-)

Member

fabiopelosin commented Aug 20, 2012

But I was thinking that maybe we should release a new version first before we do an RC for this. Agreed?

Yes, I think that it is a good idea. Isolating big patches in a single release makes bug tracking easier :-)

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 20, 2012

This pull request fails (merged 505f535 into be222a0).

travisbot commented Aug 20, 2012

This pull request fails (merged 505f535 into be222a0).

@fabiopelosin

This comment has been minimized.

Show comment
Hide comment
@fabiopelosin

fabiopelosin Aug 23, 2012

Member

I think that we can remove them, they are a left over of the old pod update logic.

Member

fabiopelosin commented Aug 23, 2012

I think that we can remove them, they are a left over of the old pod update logic.

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 23, 2012

This pull request passes (merged cb2d996 into 79089b4).

travisbot commented Aug 23, 2012

This pull request passes (merged cb2d996 into 79089b4).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 23, 2012

This pull request fails (merged 4954dbe into 79089b4).

travisbot commented Aug 23, 2012

This pull request fails (merged 4954dbe into 79089b4).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 23, 2012

This pull request passes (merged 4954dbe into 79089b4).

travisbot commented Aug 23, 2012

This pull request passes (merged 4954dbe into 79089b4).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 23, 2012

This pull request passes (merged a6323df into 79089b4).

travisbot commented Aug 23, 2012

This pull request passes (merged a6323df into 79089b4).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 23, 2012

This pull request passes (merged 8a61d2d into 79089b4).

travisbot commented Aug 23, 2012

This pull request passes (merged 8a61d2d into 79089b4).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 24, 2012

This pull request passes (merged 811f98c into 79089b4).

travisbot commented Aug 24, 2012

This pull request passes (merged 811f98c into 79089b4).

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Aug 24, 2012

Member

@irrationalfab So I think it’s getting in an RC state, what do you think?

Member

alloy commented Aug 24, 2012

@irrationalfab So I think it’s getting in an RC state, what do you think?

@fabiopelosin

This comment has been minimized.

Show comment
Hide comment
@fabiopelosin

fabiopelosin Aug 24, 2012

Member

I think so, as well. Are you going to merge it?

On 24/ago/2012, at 23:10, Eloy Durán notifications@github.com wrote:

@irrationalfab So I think it’s getting in an RC state, what do you think?


Reply to this email directly or view it on GitHub.

Member

fabiopelosin commented Aug 24, 2012

I think so, as well. Are you going to merge it?

On 24/ago/2012, at 23:10, Eloy Durán notifications@github.com wrote:

@irrationalfab So I think it’s getting in an RC state, what do you think?


Reply to this email directly or view it on GitHub.

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Aug 24, 2012

Member

I think it’s better if you do it :)

Member

alloy commented Aug 24, 2012

I think it’s better if you do it :)

@fabiopelosin fabiopelosin merged commit 811f98c into master Aug 25, 2012

1 check passed

default The Travis build passed
Details
@fabiopelosin

This comment has been minimized.

Show comment
Hide comment
@fabiopelosin

fabiopelosin Aug 25, 2012

Member

Done! Should we release the RC?

Member

fabiopelosin commented Aug 25, 2012

Done! Should we release the RC?

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Aug 25, 2012

Member

Before we release the RC, what was your roadmap plan with regards to these?

  • The SCM reference of head Pods is not preserved across machines.
  • Pods whose inline specification changed are not detected as modified. As a
    workaround, remove their folder stored in Pods.
  • Pods whose specification changed are not detected as modified. As a
    workaround, remove their folder stored in Pods.

I do think we really need those in the final version.

Member

alloy commented Aug 25, 2012

Before we release the RC, what was your roadmap plan with regards to these?

  • The SCM reference of head Pods is not preserved across machines.
  • Pods whose inline specification changed are not detected as modified. As a
    workaround, remove their folder stored in Pods.
  • Pods whose specification changed are not detected as modified. As a
    workaround, remove their folder stored in Pods.

I do think we really need those in the final version.

@fabiopelosin

This comment has been minimized.

Show comment
Hide comment
@fabiopelosin

fabiopelosin Aug 27, 2012

Member

Before we release the RC, what was your roadmap plan with regards to these?

I think that we should include them in a future release.

I do think we really need those in the final version.

I'm sorry, but this sentence is unclear to me. Do you mean in the final version of this release? Or that we should not include those features at all.

The SCM reference of head Pods is not preserved across machines.

Regarding the first one is more a matter of how much we want to support head deps. The more we support them the less the pressure for the community in versioning. So we could not implement this feature.

Pods whose specification changed are not detected as modified

This is discussed in CocoaPods/Specs#202.

Pods whose inline specification changed are not detected as modified.

This is to prevent that you edit the inline spec, call pod install and nothing changes. I was planning to do a checksum of Object#inspect. However, also this is questionable as we could start discouraging inline podspecs in favor of podspecs from an external source.

Member

fabiopelosin commented Aug 27, 2012

Before we release the RC, what was your roadmap plan with regards to these?

I think that we should include them in a future release.

I do think we really need those in the final version.

I'm sorry, but this sentence is unclear to me. Do you mean in the final version of this release? Or that we should not include those features at all.

The SCM reference of head Pods is not preserved across machines.

Regarding the first one is more a matter of how much we want to support head deps. The more we support them the less the pressure for the community in versioning. So we could not implement this feature.

Pods whose specification changed are not detected as modified

This is discussed in CocoaPods/Specs#202.

Pods whose inline specification changed are not detected as modified.

This is to prevent that you edit the inline spec, call pod install and nothing changes. I was planning to do a checksum of Object#inspect. However, also this is questionable as we could start discouraging inline podspecs in favor of podspecs from an external source.

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Aug 27, 2012

Member

I'm sorry, but this sentence is unclear to me. Do you mean in the final version of this release?

Yes, I meant in the final version of this release, but mainly the SCM reference point.

Regarding the first one is more a matter of how much we want to support head deps. The more we support them the less the pressure for the community in versioning. So we could not implement this feature.

I don’t worry about that too much, as in my experience one usually goes from very loose version conditions to very strict ones near the end of the development on a project, so these versions will still be necessary in most cases where somebody is currently using the HEAD version.

But I also think many people will use the :head option and thus they will run into unexpected issues.

Pods whose specification changed are not detected as modified
Pods whose inline specification changed are not detected as modified.

These ones are I think less likely to cause unexpected issues for users, so adding them later is fine by me.

Member

alloy commented Aug 27, 2012

I'm sorry, but this sentence is unclear to me. Do you mean in the final version of this release?

Yes, I meant in the final version of this release, but mainly the SCM reference point.

Regarding the first one is more a matter of how much we want to support head deps. The more we support them the less the pressure for the community in versioning. So we could not implement this feature.

I don’t worry about that too much, as in my experience one usually goes from very loose version conditions to very strict ones near the end of the development on a project, so these versions will still be necessary in most cases where somebody is currently using the HEAD version.

But I also think many people will use the :head option and thus they will run into unexpected issues.

Pods whose specification changed are not detected as modified
Pods whose inline specification changed are not detected as modified.

These ones are I think less likely to cause unexpected issues for users, so adding them later is fine by me.

@fabiopelosin

This comment has been minimized.

Show comment
Hide comment
@fabiopelosin

fabiopelosin Aug 27, 2012

Member

So should we release?

Member

fabiopelosin commented Aug 27, 2012

So should we release?

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Aug 28, 2012

Member

Yes, let’s release RC1! :)

I will work on the SCM part during the next week.

Member

alloy commented Aug 28, 2012

Yes, let’s release RC1! :)

I will work on the SCM part during the next week.

@fabiopelosin

This comment has been minimized.

Show comment
Hide comment
@fabiopelosin

fabiopelosin Aug 28, 2012

Member

🎷💃 Released! 💃🎷

Member

fabiopelosin commented Aug 28, 2012

🎷💃 Released! 💃🎷

@Palleas

This comment has been minimized.

Show comment
Hide comment
@Palleas

Palleas Aug 28, 2012

Great news \o/

Palleas commented Aug 28, 2012

Great news \o/

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Aug 28, 2012

Member

PARTY! 👻

Member

alloy commented Aug 28, 2012

PARTY! 👻

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Sep 3, 2012

Member

Sorry, didn’t get around to work on the SCM stuff this weekend, should happen this week.

Member

alloy commented Sep 3, 2012

Sorry, didn’t get around to work on the SCM stuff this weekend, should happen this week.

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Sep 5, 2012

Member

@irrationalfab I’ve been working and thinking off and on about this for the last couple of days, I’ll push a proof-of-concept hack tonight. But this is definitely turning out harder than I had expected, because the info we need is too fragmented over several classes. So even if my hacking will work, I think we’ll need to re-think some parts of the architecture of working in install and update mode, but at least we’ll have all the info we need :)

Member

alloy commented Sep 5, 2012

@irrationalfab I’ve been working and thinking off and on about this for the last couple of days, I’ll push a proof-of-concept hack tonight. But this is definitely turning out harder than I had expected, because the info we need is too fragmented over several classes. So even if my hacking will work, I think we’ll need to re-think some parts of the architecture of working in install and update mode, but at least we’ll have all the info we need :)

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Sep 5, 2012

Member

@irrationalfab Ok, here are the results of my experimentation: https://github.com/CocoaPods/CocoaPods/compare/lock-scm-commit.

Basically what happens is the following:

  1. After a pod that is in :head mode or is an external from a SCM (e.g. directly from a :git repo), the commit info is collected and is later on written to the lock file.
  2. During subsequent install, the lockfile returns a head/external dependency but with explicit version and commit info (source info).

Did you have a different idea on how to handle this? Or is the general idea in line with your ideas?

Member

alloy commented Sep 5, 2012

@irrationalfab Ok, here are the results of my experimentation: https://github.com/CocoaPods/CocoaPods/compare/lock-scm-commit.

Basically what happens is the following:

  1. After a pod that is in :head mode or is an external from a SCM (e.g. directly from a :git repo), the commit info is collected and is later on written to the lock file.
  2. During subsequent install, the lockfile returns a head/external dependency but with explicit version and commit info (source info).

Did you have a different idea on how to handle this? Or is the general idea in line with your ideas?

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Sep 5, 2012

Member

PS, here’s a lockfile example: https://gist.github.com/3645541. AFIncrementalStore and JMImageCache already have new commits at the time of writing, with this lockfile you should see it installs specific commits, not HEAD.

Member

alloy commented Sep 5, 2012

PS, here’s a lockfile example: https://gist.github.com/3645541. AFIncrementalStore and JMImageCache already have new commits at the time of writing, with this lockfile you should see it installs specific commits, not HEAD.

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Sep 6, 2012

Member

@irrationalfab I’m thinking wether or not we should maybe release 0.14.0 as is and postpone this to the next release. I now have a fair idea of what’s needed, but think that it might be more sensible to refactor and re-architect specific parts first, instead of after the fact, allowing for some more time to not rush it.

Member

alloy commented Sep 6, 2012

@irrationalfab I’m thinking wether or not we should maybe release 0.14.0 as is and postpone this to the next release. I now have a fair idea of what’s needed, but think that it might be more sensible to refactor and re-architect specific parts first, instead of after the fact, allowing for some more time to not rush it.

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Sep 6, 2012

Member

@irrationalfab Another issue to solve is how we can identify at what commit a pod is in order to determine wether or not it should be removed and updated during a normal install. Maybe we should add a REVISION file to the root of such pods and store the info there? Or have a manifest file in the Pods directory?

Member

alloy commented Sep 6, 2012

@irrationalfab Another issue to solve is how we can identify at what commit a pod is in order to determine wether or not it should be removed and updated during a normal install. Maybe we should add a REVISION file to the root of such pods and store the info there? Or have a manifest file in the Pods directory?

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Sep 6, 2012

Member

Note to self:

  • Currently we store the dependencies that lead to a specific version of a spec on the Specification::Set only, which means that afterwards it becomes much harder to re-determine those dependencies. So store the list of dependencies on the spec returned from the set.
  • In the resolver, we add the head flag to the version of a spec. It might be better to set this flag from the write method on the spec that will accept the list of dependencies that lead to it (the above) which then checks if any of the dependencies is in head mode. This should bring the related functionality more together.
  • It appears that an external spec from an SCM (in this case with the :git option) does not currently use the source info from the lockfile, this is probably because external dependencies are installed before resolve, which is where currently the specific dependencies are fetched from the lockfile.
Member

alloy commented Sep 6, 2012

Note to self:

  • Currently we store the dependencies that lead to a specific version of a spec on the Specification::Set only, which means that afterwards it becomes much harder to re-determine those dependencies. So store the list of dependencies on the spec returned from the set.
  • In the resolver, we add the head flag to the version of a spec. It might be better to set this flag from the write method on the spec that will accept the list of dependencies that lead to it (the above) which then checks if any of the dependencies is in head mode. This should bring the related functionality more together.
  • It appears that an external spec from an SCM (in this case with the :git option) does not currently use the source info from the lockfile, this is probably because external dependencies are installed before resolve, which is where currently the specific dependencies are fetched from the lockfile.
@fabiopelosin

This comment has been minimized.

Show comment
Hide comment
@fabiopelosin

fabiopelosin Sep 6, 2012

Member

@irrationalfab I’m thinking wether or not we should maybe release 0.14.0 as is and postpone this to the next release. I now have a fair idea of what’s needed, but think that it might be more sensible to refactor and re-architect specific parts first, instead of after the fact, allowing for some more time to not rush it.

I think that this is a good idea.

Regarding the patch I'm reviewing it and some comments are coming soon.

Member

fabiopelosin commented Sep 6, 2012

@irrationalfab I’m thinking wether or not we should maybe release 0.14.0 as is and postpone this to the next release. I now have a fair idea of what’s needed, but think that it might be more sensible to refactor and re-architect specific parts first, instead of after the fact, allowing for some more time to not rush it.

I think that this is a good idea.

Regarding the patch I'm reviewing it and some comments are coming soon.

@fabiopelosin

This comment has been minimized.

Show comment
Hide comment
@fabiopelosin

fabiopelosin Sep 6, 2012

Member

@irrationalfab Another issue to solve is how we can identify at what commit a pod is in order to determine wether or not it should be removed and updated during a normal install. Maybe we should add a REVISION file to the root of such pods and store the info there? Or have a manifest file in the Pods directory?

I vote for a manifest inside the pods folder (a copy of the lockfile?) So with pod install can compare if there is any change in the lockfile. And reinstall the pods that don't match. This copy should not be included in source control and should be used also to check if the version of a pod changed.

For example, I install a MyPod, "~> 0.1" a new version comes out and a collaborator runs pod update. I pull the changes and now my podfile is overridden but the copy in the Pods folder, the manifest, is not. The lockfile before checking for the changes with the Podfile checks for the changes with this manifest (easy because they are the same class). If for any pod there is any mismatch, we reinstall it, because the Podfile.lock should be the authoritative source about what the Pods libraries contain.

What do you think?

Member

fabiopelosin commented Sep 6, 2012

@irrationalfab Another issue to solve is how we can identify at what commit a pod is in order to determine wether or not it should be removed and updated during a normal install. Maybe we should add a REVISION file to the root of such pods and store the info there? Or have a manifest file in the Pods directory?

I vote for a manifest inside the pods folder (a copy of the lockfile?) So with pod install can compare if there is any change in the lockfile. And reinstall the pods that don't match. This copy should not be included in source control and should be used also to check if the version of a pod changed.

For example, I install a MyPod, "~> 0.1" a new version comes out and a collaborator runs pod update. I pull the changes and now my podfile is overridden but the copy in the Pods folder, the manifest, is not. The lockfile before checking for the changes with the Podfile checks for the changes with this manifest (easy because they are the same class). If for any pod there is any mismatch, we reinstall it, because the Podfile.lock should be the authoritative source about what the Pods libraries contain.

What do you think?

@fabiopelosin

This comment has been minimized.

Show comment
Hide comment
@fabiopelosin

fabiopelosin Sep 6, 2012

Member

Did you have a different idea on how to handle this? Or is the general idea in line with your ideas?

My idea was to store the SCM information in the version class. As I consider a specific commit a form of versioning (HEAD version based on 0.2 pointing at sha). My idea was that the Lockfile would load this information in the version class if available. Then the installer would pass this information to the downloader and store it back (in case of an install). Finally the lockfile would take this information directly form the specs without using the old lockfile.

However, I'm not sure about the possible complications.

Currently we store the dependencies that lead to a specific version of a spec on the Specification::Set only, which means that afterwards it becomes much harder to re-determine those dependencies. So store the list of dependencies on the spec returned from the set.

In the resolver, we add the head flag to the version of a spec. It might be better to set this flag from the write method on the spec that will accept the list of dependencies that lead to it (the above) which then checks if any of the dependencies is in head mode. This should bring the related functionality more together.

Good refactoring is good!

It appears that an external spec from an SCM (in this case with the :git option) does not currently use the source info from the lockfile, this is probably because external dependencies are installed before resolve, which is where currently the specific dependencies are fetched from the lockfile.

I don't fully understand your point, pods with external dependencies, if installed, should come from a Podfile dep. If they are not installed they should have their dependencies provided by Lockfile and by the Podfile and those two should match.

Member

fabiopelosin commented Sep 6, 2012

Did you have a different idea on how to handle this? Or is the general idea in line with your ideas?

My idea was to store the SCM information in the version class. As I consider a specific commit a form of versioning (HEAD version based on 0.2 pointing at sha). My idea was that the Lockfile would load this information in the version class if available. Then the installer would pass this information to the downloader and store it back (in case of an install). Finally the lockfile would take this information directly form the specs without using the old lockfile.

However, I'm not sure about the possible complications.

Currently we store the dependencies that lead to a specific version of a spec on the Specification::Set only, which means that afterwards it becomes much harder to re-determine those dependencies. So store the list of dependencies on the spec returned from the set.

In the resolver, we add the head flag to the version of a spec. It might be better to set this flag from the write method on the spec that will accept the list of dependencies that lead to it (the above) which then checks if any of the dependencies is in head mode. This should bring the related functionality more together.

Good refactoring is good!

It appears that an external spec from an SCM (in this case with the :git option) does not currently use the source info from the lockfile, this is probably because external dependencies are installed before resolve, which is where currently the specific dependencies are fetched from the lockfile.

I don't fully understand your point, pods with external dependencies, if installed, should come from a Podfile dep. If they are not installed they should have their dependencies provided by Lockfile and by the Podfile and those two should match.

@fabiopelosin

This comment has been minimized.

Show comment
Hide comment
@fabiopelosin

fabiopelosin Sep 6, 2012

Member

Last note currently if you don't run pod update the specs with head source remain unlocked, maybe we should print a warning.

Member

fabiopelosin commented Sep 6, 2012

Last note currently if you don't run pod update the specs with head source remain unlocked, maybe we should print a warning.

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Sep 10, 2012

Member

Hmm, for some reason I was waiting for more feedback, or I missed some of your feedback… I’ll read/answer tonight.

Also, do you want to release 0.14.0, or do you want me to do it?

Member

alloy commented Sep 10, 2012

Hmm, for some reason I was waiting for more feedback, or I missed some of your feedback… I’ll read/answer tonight.

Also, do you want to release 0.14.0, or do you want me to do it?

@fabiopelosin

This comment has been minimized.

Show comment
Hide comment
@fabiopelosin

fabiopelosin Sep 10, 2012

Member

I'm releasing it :-)

On 10/set/2012, at 18:14, Eloy Durán notifications@github.com wrote:

Hmm, for some reason I was waiting for more feedback, or I missed some of your feedback… I’ll read/answer tonight.

Also, do you want to release 0.14.0, or do you want me to do it?


Reply to this email directly or view it on GitHub.

Member

fabiopelosin commented Sep 10, 2012

I'm releasing it :-)

On 10/set/2012, at 18:14, Eloy Durán notifications@github.com wrote:

Hmm, for some reason I was waiting for more feedback, or I missed some of your feedback… I’ll read/answer tonight.

Also, do you want to release 0.14.0, or do you want me to do it?


Reply to this email directly or view it on GitHub.

@fabiopelosin

This comment has been minimized.

Show comment
Hide comment
@fabiopelosin

fabiopelosin Sep 10, 2012

Member

Done!

Member

fabiopelosin commented Sep 10, 2012

Done!

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Sep 10, 2012

Member

💥 😋 💥

Member

alloy commented Sep 10, 2012

💥 😋 💥

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Nov 8, 2013

Coverage Status

Changes Unknown when pulling 811f98c on pod_update into * on master*.

coveralls commented Nov 8, 2013

Coverage Status

Changes Unknown when pulling 811f98c on pod_update into * on master*.

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