Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

fix for erroneous linting on 0.21.0.rc1 #1130

Merged
merged 4 commits into from Jun 24, 2013

Conversation

Projects
None yet
5 participants
Contributor

HBehrens commented Jun 21, 2013

Linting the apparently valid podspec Transit 0.0.2 fails on 0.21.0.rc1 and hinders publishing the spec since travis aborts the build consequently.

The output

NoMethodError - undefined method `spec_consumer' for nil:NilClass
/Library/Ruby/Gems/1.8/gems/cocoapods-0.21.0.rc1/lib/cocoapods/validator.rb:253:in `check_file_patterns'
/Library/Ruby/Gems/1.8/gems/cocoapods-0.21.0.rc1/lib/cocoapods/validator.rb:251:in `each'
/Library/Ruby/Gems/1.8/gems/cocoapods-0.21.0.rc1/lib/cocoapods/validator.rb:251:in `check_file_patterns'
/Library/Ruby/Gems/1.8/gems/cocoapods-0.21.0.rc1/lib/cocoapods/validator.rb:179:in `perform_extensive_analysis'
/Library/Ruby/Gems/1.8/gems/cocoapods-0.21.0.rc1/lib/cocoapods/validator.rb:173:in `each'
/Library/Ruby/Gems/1.8/gems/cocoapods-0.21.0.rc1/lib/cocoapods/validator.rb:173:in `perform_extensive_analysis'
/Library/Ruby/Gems/1.8/gems/cocoapods-0.21.0.rc1/lib/cocoapods/validator.rb:63:in `validate'
/Library/Ruby/Gems/1.8/gems/cocoapods-0.21.0.rc1/lib/cocoapods/command/spec.rb:86:in `run'
/Library/Ruby/Gems/1.8/gems/cocoapods-0.21.0.rc1/lib/cocoapods/command/spec.rb:80:in `each'
/Library/Ruby/Gems/1.8/gems/cocoapods-0.21.0.rc1/lib/cocoapods/command/spec.rb:80:in `run'
/Library/Ruby/Gems/1.8/gems/claide-0.3.2/lib/claide/command.rb:206:in `run'
/Library/Ruby/Gems/1.8/gems/cocoapods-0.21.0.rc1/lib/cocoapods/command.rb:49:in `run'
/Library/Ruby/Gems/1.8/gems/cocoapods-0.21.0.rc1/bin/pod:19
/usr/bin/pod:23:in `load'
/usr/bin/pod:23

is caused by and empty @file_accessor instance variable in the validator since install_pod expects a specific order when building file_accessors:

def install_pod
  podfile = podfile_from_spec(consumer.platform_name, spec.deployment_target(consumer.platform_name))
  sandbox = Sandbox.new(config.sandbox_root)
  installer = Installer.new(sandbox, podfile)
  installer.install!


  # here: value of installer.aggregate_targets.first.pod_targets is
  # [<Pod::PodTarget name=Pods-SBJson platform=iOS 5.0>, <Pod::PodTarget name=Pods-Transit platform=iOS 5.0>]

  file_accessors = installer.aggregate_targets.first.pod_targets.first.file_accessors
  @file_accessor = file_accessors.find { |accessor| accessor.spec == spec }
  config.silent
end

At least in my case, the desired target (Pods-Transit) cames last not first. Building file_accessors as combined list of all files_accessors of every target seems to solve this.

Coverage Status

Coverage remained the same when pulling 53f5c31 on HBehrens:master into 2fa29fb on CocoaPods:master.

Owner

alloy commented Jun 21, 2013

Thanks! Can you add a test case that would fail without your patch? This to ensure we never regress on this again.

Contributor

HBehrens commented Jun 21, 2013

Ruby is not exactly my first language. Can you point me to a commit with a simple change like mine that contains another test so I can learn how to set things up? And... how do I run the test suite?

Owner

alloy commented Jun 21, 2013

The tests for the validator that we have are here.

After setting up the dev env as described here, you should be able to run just that test with: $ bundle exec bacon spec/unit/validator_spec.rb.

Looking at the existing specs, though, I don’t see an easy example to re-use, as the spec that’s used has no dependencies itself. I guess you could create a podspec in memory, serialise it as YAML to a tempfile, and then run the validator against it like the other specs do.

@irrationalfab Were you planning on doing something different with the validator specs or do you have a better suggestion for @HBehrens ?

Contributor

sirlantis commented Jun 21, 2013

@alloy I tried to pin down the issue for @HBehrens: it only occures if spec.name > dependency.name for any of your dependencies.

file_accessors = installer.aggregate_targets.first.pod_targets.first.file_accessors assumes that your main spec file is the first element in pod_targets. However cached_specs.values.sort_by(&:name) sorts them by name.

I'm going to figure out how to test this now.

Contributor

HBehrens commented Jun 21, 2013

Then, most pods that depend on AFNetworking will fail. Thank you for taking over @sirlantis Highly appreciated.

Owner

fabiopelosin commented Jun 21, 2013

Good catch so this basically assumed one target for the hole project and didn't take into account the Pod targets. Note thought that the original check looks incomplete. It should check every spec (including any eventual subspec) of the Pod. It could be implemented like:

      file_accessors = installer.aggregate_targets.first.pod_targets.map { |target| target.file_accessors }.flatten
      @file_accessors = file_accessors.select { |accessor| accessor.spec.root.name == spec.root.name }

Regarding the tests, those appear to be outdated and precedent to the validator partial (and incomplete) refactor of 0.17. There should be no need to serialize the spec and so an instance can be created and just passed. A starting point could be:

 it "is robust against deps" do
      spec = Specification.new
      spec.name = 'testPod'
      spec.dependency 'otherPod'
      validator = Validator.new(pod)
      validator.validate
      validator.validated?.should.be.true
    end
Contributor

sirlantis commented Jun 21, 2013

Since the Validator complained about an undefined realpath when creating just a Spec instance (crashing with an Exception, different issue) I now followed some of the other tests and used the stub_podspec to reproduce the issue (commits attached, 6eba865 has the regression test). I also added @irrationalfab's recommendation and got rid of all the firsts.

Coverage Status

Coverage remained the same when pulling 568b87f on HBehrens:master into 2fa29fb on CocoaPods:master.

[Validator] Improve regression test for `undefined method `spec_consu…
…mer'`

- let spec in "validates a podspec with dependencies" validate
- fixed a bug in `write_podspec`

Coverage Status

Coverage remained the same when pulling 62ddb8e on HBehrens:master into 2fa29fb on CocoaPods:master.

alloy added a commit that referenced this pull request Jun 24, 2013

Merge pull request #1130 from HBehrens/master
fix for erroneous linting on 0.21.0.rc1

@alloy alloy merged commit cc2826b into CocoaPods:master Jun 24, 2013

1 check passed

default The Travis CI build passed
Details
Owner

alloy commented Jun 24, 2013

Thanks!

Owner

fabiopelosin commented Jun 25, 2013

👍

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