Specification refactor #253

Merged
merged 48 commits into from May 23, 2012

Conversation

Projects
None yet
3 participants
Owner

fabiopelosin commented May 9, 2012

Attempt to refactor the specification class. It is still a work in progress.

Objective

Have a clean hierarchy of specifications nodes:

  • each specification inherits all its subspecs.
  • for specification dryness common attributes can be specified by a node and will be inherited by all its subspecs.
  • simplify code removing download only pods and wrappers [#223].
  • If a specification contains incompatible subspecs it can specify a main subspec

Discusion

This is an experiment performed in total freedom (even in code style) and might have unwanted features that can be rolled back.

SubSpec class

The subspec class was decoupling inheritance logic from attributes specification and thus was removed and replaced with meta programming. Every specification can have a parent. If no parent is provided it is a top level specification.

Moreover, the subspec class was requiring its parent and this logic has been removed, in favor of the inheritance logic specified at the attribute level.

Inheritance
dependency 'RestKit'

Will require result in the following specs being loaded:

 - RestKit
     - RestKit/Network
     - RestKit/ObjectMapping
       - RestKit/ObjectMapping/CoreData
       - RestKit/ObjectMapping/JSON
       - RestKit/ObjectMapping/XML
     - RestKit/UI

However, to handle incompatible different strategies it is possible to specify a main subspec:

dependency do |s|
        s.main_subspec = 'JSON'
        s.subspec 'JSON' do |js|
          js.dependency 'RestKit/Network'
          js.dependency 'RestKit/UI'
          js.dependency 'RestKit/ObjectMapping/JSON'
          js.dependency 'RestKit/ObjectMapping/CoreData'
        end
        s.subspec 'XML' do |js|
          js.dependency 'RestKit/Network'
          js.dependency 'RestKit/UI'
          js.dependency 'RestKit/ObjectMapping/XML'
          js.dependency 'RestKit/ObjectMapping/CoreData'
        end
[...]
end

Is not fully implemented yet but this will result in the RestKit spec returning RestKit/JSON instead of itself so pod installs tells to newbies that they are using a json specific strategy.

Attributes inheritance

Some attributes can be specified only at the top level. They are what uniquely identifies a pod, can't be assigned by pods that have a parent (raises) and always return the top level value.

Other attributes are resolved back in the chain. For example every spec inherits all the frameworks required by its parents. The same logic will be applied to source files and if there are any duplicates local pod will filter the unique values. Looking at the code should be clear how every attribute is handled.


Todo
  • deprecate clean paths
    • add readme_file attribute to specification.
    • nuke any file that is not specified in the podspec.
    • will dry podpecs.
    • in case if needed add a preserve_files attribute.
  • local pod will receive a all the specifications sharing a common parent
    • it will be able to remove duplicates (source files, frameworks, xconfig, ...).
    • it will be able to clean all unused files.
  • Speficiation#activate_for_platform activates a given platform so the attr readers return the value for the platform and clients will not need to be aware of which attributes are multiplatform.
  • update the 0.6 branch of the master repo

Related Issues
  • [#223] Pod subspec not adding other subspec dependencies to the project
  • [#141] Specifying a podspec which contains subspecs throws exception
  • [#87] Think of a good way to do clean_paths for a spec with subspecs
Owner

alloy commented May 9, 2012

Sweet.

I’ve at least read specification.rb and I think you’re on the right track.

One question, do you have an estimate of the timeframe for this? Can we do it in 0.6, or will that push the first RC release back too long?

If a specification contains incompatible subspecs it can specify a main subspec

dependency do |s|
        s.main_subspec = 'JSON'
        s.subspec 'JSON' do |js|
        end
        s.subspec 'XML' do |js|
        end
end

In your example, the value for main_subspec is more of a preference. Can we call that something like preferred_dependency?

add readme_file attribute to specification.

Is that for doc generation purposes too? Otherwise I would opt for just preserve_files, because we will need that anyways.

Owner

fabiopelosin commented May 9, 2012

I’ve at least read specification.rb and I think you’re on the right track.

Cool i was really looking for confirmation at least for that one.

One question, do you have an estimate of the timeframe for this? Can we do it in 0.6, or will that push the first RC release back too long?

Lets see at what's left tomorrow night, but I think that it could be done for 0.6. What worries me are regressions that might arise by cases covered in the specs.

In your example, the value for main_subspec is more of a preference. Can we call that something like preferred_dependency?

Agreed on the preferred part. However this subspec is not a dependency but it is the specification that you get, which naturally inherits from the parent. If you require RestKit but you get RestKit/JSON. This is so because in this way the installer, and all the clients list it as RestKit/JSON. In this way it is intuitive that there is RestKit/XML.

So preferred_subspec?

Is that for doc generation purposes too? Otherwise I would opt for just preserve_files, because we will need that anyways.

My idea was that LocalPod checks for [license, license.txt] and [readme.*] so you need to specify those files only if they are not available. The linter will warn if those files are not found and if not available the push will need to be done with allow-warnings.

For this reason I was tempted to specify the license file as license_file instead of using the current hash.

fabiopelosin added some commits May 10, 2012

Merge branch 'master' into spec-refactor
Get the cache for testing.

* master:
  Enable caching for specs and remove configurable git cache size.
  Save Git chaches in ~/Library/Caches/CocoaPods/Git.
  [Downloader::Git] Skip cache pull if ref is available.
  Restored Pod::Downloader::Git::GitHub.
  [Git] Fix for Travis errors.
  [Git] Increased default cache to 500Mb.
  [Downloader::Git] Completed cache support
  [Git<<Downloader] Fix for url hash
  [Downloader::Git] Cache repos
Owner

fabiopelosin commented May 10, 2012

State

The branch still needs a good clean up, specs, and some good testing. But the logic should be there.

I have tested it with ShareKit and it seems to be working. Here is the modified version of the spec. It doesn't require a Core subspec and it doesn't require an include all subspec.

dependency 'ShareKit'
$ pod install --no-update --no-integrate
Installing Facebook-iOS-SDK (1.2)
Installing JSONKit (1.5pre)
Installing Reachability (3.0.0)
Installing SBJson (2.2.3)
Installing SSKeychain (0.1.2)
Installing ShareKit (2.0)
 - Evernote
 - Facebook
 - Flickr
 - Foursquare
 - GoogleReader
 - Instapaper
 - LinkedIn
 - Pinboard
 - ReadItLater
 - Tumblr
 - Twitter
 - Vkontakte
Installing objectiveflickr (0.0.1)
Generating support files
dependency 'ShareKit/Facebook'
$ pod install --no-update --no-integrate
Installing Facebook-iOS-SDK (1.2)
Installing SBJson (2.2.3)
Installing ShareKit (2.0)
 - Facebook
Generating support files

Still open questions
  • preferred_spec name, and actually if it is a good idea to substitute it to the parent.
    • Another, alternative would be to have a simple attribute like exclude_from_parent_deps for the subspec that should not be automatically inherited.
  • license and preserve_paths
Owner

alloy commented May 10, 2012

Yeah that ShareKit spec does look good.

For this reason I was tempted to specify the license file as license_file instead of using the current hash.

Which hash? The Specification#license hash?

preferred_spec name

Actually, what do you think of default_spec?

and actually if it is a good idea to substitute it to the parent.

I’m not following. Is this about the question below?

Another, alternative would be to have a simple attribute like exclude_from_parent_deps for the subspec that should not be automatically inherited.

I don’t see a benefit in blacklisting vs whitelisting in this case. Naming one makes it much clearer, to the user of the spec, which specs are going to be used.

license and preserve_paths

Like with the hash, I’m not sure what the concrete question is regarding the license attribute. The preserve_paths attribute is definitely needed.

Owner

fabiopelosin commented May 10, 2012

Which hash? The Specification#license hash?

Yes, but I don't think it is a good idea anymore.

Actually, what do you think of default_spec?
I’m not following. Is this about the question below?

Now that I have better comprehension of the problem I see less need for substituting the parent with the default_spec. It adds a non necessary layer of complexity and doesn't adhere to the principle of least surprise. To keep it KISS, I'll change that part and simply make Specifications#dependencies return the default_spec if declared.

Taking into account that, your original suggestion (preferred_dependency) seems the best name.

I don’t see a benefit in blacklisting vs whitelisting in this case. Naming one makes it much clearer, to the user of the spec, which specs are going to be used.

Blacklisting does not require to keep the list of subspecs, but this is a minor nuisance.

Related: To be fair the RestKit example was approximative, as would result in a broken require RestKit/ObjectMapping. A more precise one would be:

dependency do |s|
        s.main_subspec = 'JSON'
        s.subspec 'ObjectMapping' do |s|
          preferred_dependency = 'JSON-Strategy'
          s.subspec 'JSON-Strategy' do |s|
            s.dependency = 'RestKit/ObjectMapping/JSON'
            s.dependency = 'RestKit/ObjectMapping/CoreData'
            ...
          end
          s.subspec 'XML-Strategy'

          s.subspec 'JSON'
          s.subspec 'XML'
          s.subspec 'CoreData'
          ...
        end
...
end

To present that there is a specific strategy being used I was planning to implement some logic in LocalPod#to_s or in Installer.
But is not perfect, yet. Any feedback?

Like with the hash, I’m not sure what the concrete question is regarding the license attribute. The preserve_paths attribute is definitely needed.

Good to know that preserve paths are needed. What I plan to do is simply avoid to clean any file matching [license*, readme*] in the root.


@alloy do you have any troublesome podspec to suggest for testing?

Owner

alloy commented May 12, 2012

Which hash? The Specification#license hash?

Yes, but I don't think it is a good idea anymore.

So what do you propose?

Taking into account that, your original suggestion (preferred_dependency) seems the best name.

Agreed, let's just go with that.

Related: To be fair the RestKit example was approximative, as would result in a broken require RestKit/ObjectMapping. A more precise one would be:

Do you have the actual specification somewhere? The condensed versions are making it harder to see what's going on.

To present that there is a specific strategy being used I was planning to implement some logic in LocalPod#to_s or in Installer.
But is not perfect, yet. Any feedback?

Something to the effect of “installing preferred JSON subspec”?

I wonder if it's really needed. If you know what you are doing such warnings only become noise. And besides, you can see from the list of specs being installed that it chose the JSON subspec, right?

@alloy do you have any troublesome podspec to suggest for testing?

I think RestKit is definitely one of the most advanced ones. Also note how the callbacks are being handled by extending it with a module. Is that already fixed by your simplification of a spec and its subspecs?

Contributor

jeanregisser commented May 13, 2012

Good to know that preserve paths are needed. What I plan to do is simply
avoid to clean any file matching [license*, readme*] in the root.

We should also avoid cleaning *.podspec at the root.

On master, if you use an external dependency (:git => 'GIT-URL') for a spec that contains s.clean_paths = FileList['*'].exclude(/(Source|README.markdown|MIT.LICENSE)$/), pod install works the firs time, but then fails the second time with an error saying it can't clone to a dir that's not empty.

The reason is that the second time it doesn't see the previously fetched (and cleaned) podspec, so it will try to fetch the dependency again to the same dir which is now not empty.

Owner

fabiopelosin commented May 14, 2012

We should also avoid cleaning *.podspec at the root.

@jeanregisser Thanks for pointing this out.

Owner

fabiopelosin commented May 14, 2012

The refactor is almost complete the remaining issues are:

  • it needs a good code review and testing ;-).
  • some specs are still missing (LocalPod, Installer) and the specs related to ASIHTTPRequest need to be restored.
  • RestKit has issues (apparently related to the header files and the extension).

Which hash? The Specification#license hash?
So what do you propose?

I was tempted to remove the file key from the Specification#license hash and have a separated attribute (Specification#license_file). However, now I think that the current implementation is good. (related comment)

Do you have the actual specification somewhere? The condensed versions are making it harder to see what's going on.

I changed my mind also on this one. The current proposals are available here.

Something to the effect of “installing preferred JSON subspec”?
I wonder if it's really needed. If you know what you are doing such warnings only become noise. And besides, you can see from the list of specs being installed that it chose the JSON subspec, right?

Good points that I missed on this update... however the current implementation should be fine. The installer output is Installing RestKit/JSON (0.10.0) while in the rest of the app it is referred as RestKit.

- yield self if block_given?
- end
-
- # TODO This is just to work around a MacRuby bug
@fabiopelosin

fabiopelosin May 14, 2012

Owner

MacRuby is not used anymore, so this is not needed... right?

@alloy

alloy May 14, 2012

Owner

Indeed! You can just merge the code with the normal #initialize method.

fabiopelosin added some commits May 14, 2012

Merge branch 'master' into spec-refactor
* master:
  Minor copy change
  A few small fixes related to integrating multiple targets/projects.
  Add rake ~> 0.9.0 (for rake/file_list.rb file) to gemspec. Closes #195.
  Fix for #141
  [Push] Bypass pre-commit hook.
  [Fix] GitHub downloader was downloading the repos twice.

Conflicts:
	spec/unit/resolver_spec.rb
Owner

alloy commented May 14, 2012

it needs a good code review and testing ;-).

I will update my clone just before I leave tonight and will read through it during my trip, I don’t know if I will get to much real world testing, but there are a few people currently using 0.6 from git, so these might catch any of such issues. Plus, because of the amount of changes we will do a RC, so that we are sure that 0.6.0 final will be solid :)

  • RestKit has issues (apparently related to the header files and the extension).

Do these issues exist with RestKit 0.10.0 and CocoaPods 0.5.1 as well?

Owner

alloy commented May 14, 2012

I changed my mind also on this one. The current proposals are available here.

The specs look good to me!

fabiopelosin added some commits May 14, 2012

[Specification] Improve handling of headers mappings.
New methods
- #header_mappings_dir: specifies the folder from which to preserve the name-spacing
- #exclude_headers: specifies the headers that should be excluded by the header mappings.

Currently those methods are used by RestKit and SSZipArchive.
Owner

fabiopelosin commented May 16, 2012

Docs

The documentation is generated for all the known subspecs and must be created before cleaning the local pod.

  • The --doc-force option is unsustainable because the dir is expected to be cleaned and would result in a documentation encompassing only the active subspecs.
  • The docs can still be generated once and work for all projects.
Compiler flags

The compiler flags are resolved per specification and can be used to silence compiler warnings. An example is available here. This is further discussed in #209.

Restkit

Restkit now compiles and lints. To avoid the need of creating an override two attributes have been added to Specification in 9027f20 and in Specs/3d7810d.

  • header_mappings_dir: preserves the headers name spacing from a given folder. ('.' can be passed -- SSZipArchive).
  • exclude_headers: specifies the header that should not be added to the search paths (a suggestion for a better name is appreciated).

Note: a podfile installing RestKit doesn't include the -all_load flag in the xcconfig, which was present in CP 0.5.1. I'm not sure why is happening and if this is an issue [Update: discussion at #234]

fabiopelosin added some commits May 16, 2012

[Specs] Fix for reinitialization of constant.
The actual warning:
./spec/unit/sandbox_spec.rb:4: warning: already initialized constant TMP_POD_ROOT
Pod::Sandbox
[Specs] Patch bacon to reset Pod::Config::Instance.
One of the tests wasn't restoring the changes it made to the config.
As the test was difficult to track down and the problem would have
become only worse in the future, the issues was solved starting with
a new config before each run.

The performance impact seems negligible and the testing environment is
more predictable.
[Documentation] Process only header files.
- Usually only the header files are commented.
- The header files don't include private categories.
- Performance gains, as doc generation is quite slow.
Owner

alloy commented May 23, 2012

Docs

The documentation is generated for all the known subspecs and must be created before cleaning the local pod.

  • The --doc-force option is unsustainable because the dir is expected to be cleaned and would result in a documentation encompassing only the active subspecs.
  • The docs can still be generated once and work for all projects.

Fair enough.

I think the issue I had was that I’m (currently) checking the Pods directory into SCM, which means that when I work on another machine, I don’t get the docs on a pod install. However, since we should try to actually use the Podfile.lock file ASAP anyways, this point becomes moot.

Compiler flags

The compiler flags are resolved per specification and can be used to silence compiler warnings. An example is available here. This is further discussed in #209.

Nice, that’s definitely the right approach.

Restkit

Restkit now compiles and lints. To avoid the need of creating an override two attributes have been added to Specification in 9027f20 and in Specs/3d7810d.

  • header_mappings_dir: preserves the headers name spacing from a given folder. ('.' can be passed -- SSZipArchive).
  • exclude_headers: specifies the header that should not be added to the search paths (a suggestion for a better name is appreciated).

I think we should totally get rid of the notion that people can override these methods, is that now the case?

Regarding the exclude_headers attribute name, I think it should be something like exclude_header_search_paths.

Owner

alloy commented May 23, 2012

Man, this PR is epic :D

Kudos!

Owner

fabiopelosin commented May 23, 2012

Regarding the documentation I have removed the force_doc option.

I think we should totally get rid of the notion that people can override these methods, is that now the case?

It should be as far I'm concerned.

Kudos!

:-)


The refactor is missing some specs for the LocalPod, needs to update the fixtures, and to update the specs repo. @alloy How do you suggest to proceed?

Owner

alloy commented May 23, 2012

The refactor is missing some specs for the LocalPod, needs to update the fixtures, and to update the specs repo. @alloy How do you suggest to proceed?

How about you write some disabled stub spec descriptions so that we know what has to be done? Then I can possibly work on them too.

I would do that in a separate merge, though, let's merge this bad boy asap.

Owner

fabiopelosin commented May 23, 2012

Ok, I've added the disabled specs.

Owner

alloy commented May 23, 2012

Cool. So, time to merge? :)

Owner

fabiopelosin commented May 23, 2012

Yes 🎉

If you don't have any objections I will also merge the sister branch in the specs repo to in the 0.6 branch.

fabiopelosin added a commit that referenced this pull request May 23, 2012

@fabiopelosin fabiopelosin merged commit 1801ce2 into master May 23, 2012

Owner

alloy commented May 23, 2012

🌟

Owner

alloy commented May 23, 2012

Go ahead. You don’t need to ask me anymore about these things, you are as much responsible for these areas of the code as much as I am nowadays :)

fabiopelosin added a commit that referenced this pull request Oct 25, 2014

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