Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Validation of subspecs #1588

Closed
wants to merge 2 commits into from

7 participants

@yalp

The validation done by pod spec lint and pod lib lint commands now checks that all subspecs build independently.
This new behavior can be disabled with --no-subspecs.
The validation of a single subspec can be done with --subspec=NAME.

lib/cocoapods/command/spec.rb
@@ -61,11 +61,13 @@ class Lint < Spec
def self.options
[ ["--quick", "Lint skips checks that would require to download and build the spec"],
["--only-errors", "Lint validates even if warnings are present"],
+ ["--subspecs", "Lint validates each subspec independenty"],
@orta Owner
orta added a note

independenty -> independently

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

http://i.imgur.com/7FwXb.gif

@fabiopelosin

Really nice :+1:, do you think that we should enable this by default? Also pod lib lint would benefit from the same treatment.

Please don't forget to update the changelog crediting yourself.

@yalp

Not sure this should be enabled by default, its quite slow on our internal podspec with 25 subspecs... but quite useful. I just fixed the misspelled description.

@fabiopelosin

Also please, don't forget about specs :smile:

@yalp

Just checked specs in functionnal/command/spec_spec.rb, it seems that all tests are launched with --quick (except the first one to test the missing spec) which prevents the execution of perform_extensive_analysis.

Maybe I'm missing something here but it seems the default behavior is not tested, and I don't know how to write this test correctly. Would be nice if there was the test for the default behavior so that I can extend it to test the subspec case...

@yalp

Just added --subspecs to pod lib lint.

Added another commit to aggregate the results to print subspecs that triggered the errors.

@coveralls

Coverage Status

Coverage remained the same when pulling 1f7bf6c on yalp:add-subspecs-lint into 9f03ab9 on CocoaPods:master.

@fabiopelosin

Maybe I'm missing something here but it seems the default behavior is not tested, and I don't know how to write this test correctly. Would be nice if there was the test for the default behavior so that I can extend it to test the subspec case...

Indeed you are right, this class was introduced in an important refactor, and as you can see from the incomplete comments it still needs a lot of love. However, although the tests are far from perfect they are testing the core behavior (I cleaned them up in 5aada47 & in 4a0d609). Note thought, that there is a lot of stubbing going on to keep a reasonable performance (also we don't need to text the output of the xcodebuild command line tool).

Maybe the original Specification::Linter::Result class could have this @subspecs ?

This could indeed be an option but it would require some changes to the Linter. However, if you intend to do so, I would prefer it to be a subsequent pull request.

@alloy
Owner

This should be the default option imo, even if it’s slower.

@alloy
Owner

As a matter of fact, do we even really need an option for it at all? Because a spec is either valid (in its entirety) or not. If we need an option that optimised for speed, maybe it should be --subspec NAME so you can test just one of the subspecs?

@fabiopelosin

I agree that the default should look for subspecs (and the option --no-subspecs should be provided). However, I think that supporting the specification of the name of the subspec is not worth in terms of implementation complexity and usefulness.

@rivera-ernesto

But then you may have a specific subspec that you try to get working by trial and error.

@alloy
Owner

@rivera-ernesto You mean you are in favour of @irrationalfab’s suggestion or my suggestion?

@yalp

@irrationalfab Checking a huge podspec like I have is quite long, having the option to check a given subspec with --subspec NAME is be quite usefull for debug like @rivera-ernesto said (I currently have to comment all subspecs in the spec to test a single one).
So if everybody is ok with this, I can modify my PR to :

  • always enable the subspecs check,
  • add the --subspec NAME option (I could also make a new PR for this one).
@fabiopelosin

@yalp Sounds great! :beers:

@yalp

Ok, I'm done.

@irrationalfab The added complexity for the --subspec option seems quite low to me (as it's just using the subspec_by_name function).

@fabiopelosin
Owner

Looks good, can you add a note to the changelog, merge master and squash everything in a single commit pull request?

@alloy
Owner

Awesome. Thanks!

Marc4Orange added some commits
@Marc4Orange Marc4Orange Validation of subspecs
The validation done by `pod spec lint` and `pod lib lint` commands
now checks that all subspecs build independently.

This new behavior can be disabled with `--no-subspecs`.

The validation of a single subspec can be done checked with `--subspec=NAME`.
fc7a717
@Marc4Orange Marc4Orange Merge branch 'master' 79b9b0e
@fabiopelosin
Owner

Merged via 5bf1445

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 5, 2013
  1. @Marc4Orange

    Validation of subspecs

    Marc4Orange authored
    The validation done by `pod spec lint` and `pod lib lint` commands
    now checks that all subspecs build independently.
    
    This new behavior can be disabled with `--no-subspecs`.
    
    The validation of a single subspec can be done checked with `--subspec=NAME`.
  2. @Marc4Orange

    Merge branch 'master'

    Marc4Orange authored
This page is out of date. Refresh to see the latest.
View
8 CHANGELOG.md
@@ -34,6 +34,14 @@ To install or update CocoaPods see this [guide](http://docs.cocoapods.org/guides
[kra Larivain/OpenTable](https://github.com/opentable)
[#1635](https://github.com/CocoaPods/CocoaPods/pull/1635)
+* Validation of all subspecs
+ The validation done by `pod spec lint` and `pod lib lint` commands
+ now checks that all subspecs build independently.
+ This new behavior can be disabled with `--no-subspecs`.
+ The validation of a single subspec can be done with `--subspec=NAME`.
+ [Marc C](https://github.com/yalp)
+ [#1588](https://github.com/CocoaPods/CocoaPods/pull/1588)
+
###### Bug Fixes
* Fixed a bug which resulted in `pod lib lint` not being able to find the
View
12 lib/cocoapods/command/lib.rb
@@ -90,13 +90,17 @@ class Lint < Lib
def self.options
[ ["--quick", "Lint skips checks that would require to download and build the spec"],
["--only-errors", "Lint validates even if warnings are present"],
+ ["--subspec=NAME","Lint validates only the given subspec"],
+ ["--no-subspecs", "Lint skips validation of subspecs"],
["--no-clean", "Lint leaves the build directory intact for inspection"] ].concat(super)
end
def initialize(argv)
- @quick = argv.flag?('quick')
- @only_errors = argv.flag?('only-errors')
- @clean = argv.flag?('clean', true)
+ @quick = argv.flag?('quick')
+ @only_errors = argv.flag?('only-errors')
+ @clean = argv.flag?('clean', true)
+ @subspecs = argv.flag?('subspecs', true)
+ @only_subspec = argv.option('subspec')
@podspecs_paths = argv.arguments!
super
end
@@ -114,6 +118,8 @@ def run
validator.quick = @quick
validator.no_clean = !@clean
validator.only_errors = @only_errors
+ validator.no_subspecs = !@subspecs || @only_subspec
+ validator.only_subspec = @only_subspec
validator.validate
unless @clean
View
12 lib/cocoapods/command/spec.rb
@@ -61,13 +61,17 @@ class Lint < Spec
def self.options
[ ["--quick", "Lint skips checks that would require to download and build the spec"],
["--only-errors", "Lint validates even if warnings are present"],
+ ["--subspec=NAME","Lint validates only the given subspec"],
+ ["--no-subspecs", "Lint skips validation of subspecs"],
["--no-clean", "Lint leaves the build directory intact for inspection"] ].concat(super)
end
def initialize(argv)
- @quick = argv.flag?('quick')
- @only_errors = argv.flag?('only-errors')
- @clean = argv.flag?('clean', true)
+ @quick = argv.flag?('quick')
+ @only_errors = argv.flag?('only-errors')
+ @clean = argv.flag?('clean', true)
+ @subspecs = argv.flag?('subspecs', true)
+ @only_subspec = argv.option('subspec')
@podspecs_paths = argv.arguments!
super
end
@@ -80,6 +84,8 @@ def run
validator.quick = @quick
validator.no_clean = !@clean
validator.only_errors = @only_errors
+ validator.no_subspecs = !@subspecs || @only_subspec
+ validator.only_subspec = @only_subspec
validator.validate
invalid_count += 1 unless validator.validated?
View
69 lib/cocoapods/validator.rb
@@ -46,7 +46,7 @@ def file
#-------------------------------------------------------------------------#
- # Lints the specification adding a {Specification::Linter::Result} for any
+ # Lints the specification adding a {Result} for any
# failed check to the {#results} list.
#
# @note This method shows immediately which pod is being processed and
@@ -56,13 +56,21 @@ def file
#
def validate
@results = []
- UI.print " -> #{spec ? spec.name : file.basename}\r" unless config.silent?
+
+ # Replace default spec with a subspec if asked for
+ a_spec = spec
+ if spec && @only_subspec
+ a_spec = spec.subspec_by_name(@only_subspec)
+ @subspec_name = a_spec.name
+ end
+
+ UI.print " -> #{a_spec ? a_spec.name : file.basename}\r" unless config.silent?
$stdout.flush
perform_linting
- perform_extensive_analysis if spec && !quick
+ perform_extensive_analysis(a_spec) if a_spec && !quick
- UI.puts " -> ".send(result_color) << (spec ? spec.to_s : file.basename.to_s)
+ UI.puts " -> ".send(result_color) << (a_spec ? a_spec.to_s : file.basename.to_s)
print_results
validated?
end
@@ -79,12 +87,22 @@ def print_results
platform_message = "[OSX] "
end
+ subspecs_message = ""
+ if result.is_a?(Result)
+ subspecs = result.subspecs.uniq
+ if subspecs.count > 2
+ subspecs_message = "[" + subspecs[0..2].join(', ') + ", and more...] "
+ elsif subspecs.count > 0
+ subspecs_message = "[" + subspecs.join(',') + "] "
+ end
+ end
+
case result.type
when :error then type = "ERROR"
when :warning then type = "WARN"
when :note then type = "NOTE"
else raise "#{result.type}" end
- UI.puts " - #{type.ljust(5)} | #{platform_message}#{result.message}"
+ UI.puts " - #{type.ljust(5)} | #{platform_message}#{subspecs_message}#{result.message}"
end
UI.puts
end
@@ -116,6 +134,14 @@ def local?; @local; end
#
attr_accessor :only_errors
+ # @return [String] name of the subspec to check, if nil all subspecs are checked.
+ #
+ attr_accessor :only_subspec
+
+ # @return [Bool] Whether the validator should validate all subspecs
+ #
+ attr_accessor :no_subspecs
+
#-------------------------------------------------------------------------#
# !@group Lint results
@@ -167,9 +193,9 @@ def perform_linting
@results.concat(linter.results)
end
+ # Perform analysis for a given spec (or subspec)
#
- #
- def perform_extensive_analysis
+ def perform_extensive_analysis(spec)
spec.available_platforms.each do |platform|
UI.message "\n\n#{spec} - Analyzing on #{platform} platform.".green.reversed
@consumer = spec.consumer(platform)
@@ -179,9 +205,20 @@ def perform_extensive_analysis
check_file_patterns
tear_down_validation_environment
end
+ perform_extensive_subspec_analysis(spec) unless @no_subspecs
+ end
+
+ # Recurively perform the extensive analysis on all subspecs
+ #
+ def perform_extensive_subspec_analysis(spec)
+ spec.subspecs.each do |subspec|
+ @subspec_name = subspec.name
+ perform_extensive_analysis(subspec)
+ end
end
attr_accessor :consumer
+ attr_accessor :subspec_name
def setup_validation_environment
validation_dir.rmtree if validation_dir.exist?
@@ -283,10 +320,24 @@ def note(message)
def add_result(type, message)
result = results.find { |r| r.type == type && r.message == message }
unless result
- result = Specification::Linter::Result.new(type, message)
+ result = Result.new(type, message)
results << result
end
result.platforms << consumer.platform_name if consumer
+ result.subspecs << subspec_name if subspec_name && !result.subspecs.include?(subspec_name)
+ end
+
+ # Specialized Result to support subspecs aggregation
+ #
+ class Result < Specification::Linter::Result
+
+ def initialize(type, message)
+ super(type, message)
+ @subspecs = []
+ end
+
+ attr_reader :subspecs
+
end
#-------------------------------------------------------------------------#
@@ -302,7 +353,7 @@ def add_result(type, message)
# in local mode.
#
def podfile_from_spec(platform_name, deployment_target)
- name = spec.name
+ name = subspec_name ? subspec_name : spec.name
podspec = file.realpath
local = local?
podfile = Pod::Podfile.new do
Something went wrong with that request. Please try again.