Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix for erroneous linting on 0.21.0.rc1 #1130

Merged
merged 4 commits into from
Jun 24, 2013
Merged

Conversation

HBehrens
Copy link
Contributor

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.

@coveralls
Copy link

Coverage Status

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

@alloy
Copy link
Member

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.

@HBehrens
Copy link
Contributor Author

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?

@alloy
Copy link
Member

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 ?

@mrcljx
Copy link
Contributor

mrcljx 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.

@HBehrens
Copy link
Contributor Author

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

@fabiopelosin
Copy link
Member

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

@mrcljx
Copy link
Contributor

mrcljx 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.

@coveralls
Copy link

Coverage Status

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

…mer'`

- let spec in "validates a podspec with dependencies" validate
- fixed a bug in `write_podspec`
@coveralls
Copy link

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
fix for erroneous linting on 0.21.0.rc1
@alloy alloy merged commit cc2826b into CocoaPods:master Jun 24, 2013
@alloy
Copy link
Member

alloy commented Jun 24, 2013

Thanks!

@fabiopelosin
Copy link
Member

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants