Skip to content

Inhibit warnings by adding search paths as system headers. #1596

Merged
merged 14 commits into from Nov 18, 2013

4 participants

@swizzlr
swizzlr commented Nov 14, 2013

How in gods name do I get this to pass?

@coveralls

Coverage Status

Coverage remained the same when pulling 908f638 on swizzlr:system_header_warning_inhibition into 9f03ab9 on CocoaPods:master.

@fabiopelosin
CocoaPods member

I love this approach of writing the test to discuss the feature! However the test, at least to me, is not clear enough to document the final behavior.

@swizzlr
swizzlr commented Nov 14, 2013

Test driven development! Give me a sec.

@fabiopelosin
CocoaPods member

@swizzlr hey man given all your activity we are inviting to the CocoaPods campfire room, where a lot of the development is discussed. If you are interested please send me the email your would prefer to use (you can use the email in my profile if your prefer that channel).

@swizzlr
swizzlr commented Nov 14, 2013

This is essentially describing the behaviour found here. We can make Xcode suppress warnings by passing in header search paths as system headers (-isystem).

By the way, I'm sorry if I'm harping on about this problem, it's just something I've become acquainted with and feel confident I can solve – roadmaps not yet available.

As for testing first, if we can reach 100% coverage of Cocoapods then we can do some big gutting refactors.

@coveralls

Coverage Status

Coverage remained the same when pulling af1d19c on swizzlr:system_header_warning_inhibition into 9f03ab9 on CocoaPods:master.

@coveralls

Coverage Status

Coverage remained the same when pulling 3fa2c3b on swizzlr:system_header_warning_inhibition into 9f03ab9 on CocoaPods:master.

@swizzlr swizzlr referenced this pull request in CocoaPods/cocoapods-integration-specs Nov 15, 2013
Closed

Rerun specs over system header inhibition method. #4

@coveralls

Coverage Status

Coverage remained the same when pulling 84884ac on swizzlr:system_header_warning_inhibition into 9f03ab9 on CocoaPods:master.

@coveralls

Coverage Status

Coverage remained the same when pulling b17ddf5 on swizzlr:system_header_warning_inhibition into 9f03ab9 on CocoaPods:master.

@coveralls

Coverage Status

Coverage remained the same when pulling b17ddf5 on swizzlr:system_header_warning_inhibition into 9f03ab9 on CocoaPods:master.

@swizzlr
swizzlr commented Nov 16, 2013

Little guide here: I need a lot of people to test this! https://gist.github.com/swizzlr/7500176

@fabiopelosin fabiopelosin commented on an outdated diff Nov 16, 2013
@@ -19,4 +19,5 @@
url = https://github.com/tonymillion/Reachability.git
[submodule "spec/cocoapods-integration-specs"]
path = spec/cocoapods-integration-specs
- url = https://github.com/CocoaPods/cocoapods-integration-specs
+ url = https://github.com/swizzlr/cocoapods-integration-specs.git
@fabiopelosin
CocoaPods member
fabiopelosin added a note Nov 16, 2013

We don't want this to be merged :smile:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fabiopelosin fabiopelosin commented on an outdated diff Nov 16, 2013
lib/cocoapods/generator/xcconfig/aggregate_xcconfig.rb
@@ -60,6 +61,11 @@ def generate
file_accessor.vendored_libraries.each do |vendored_library|
XCConfigHelper.add_library_build_settings(vendored_library, @xcconfig, target.sandbox.root)
end
+ @xcconfig.merge!('OTHER_CFLAGS' => '$(inherited)')
+ if pod_target.target_definition.inhibits_warnings_for_pod?(file_accessor.spec.root.name)
@fabiopelosin
CocoaPods member
fabiopelosin added a note Nov 16, 2013

Also I would remove this, unless I'm missing something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fabiopelosin fabiopelosin commented on an outdated diff Nov 16, 2013
spec/unit/generator/xcconfig/aggregate_xcconfig_spec.rb
@@ -55,9 +55,14 @@ module XCConfig
@xcconfig.to_hash['PODS_ROOT'].should == '${SRCROOT}/Pods'
end
- it 'adds the sandbox public headers search paths to the xcconfig, with quotes' do
- expected = "\"#{config.sandbox.public_headers.search_paths.join('" "')}\""
- @xcconfig.to_hash['HEADER_SEARCH_PATHS'].should == expected
+# it 'adds the sandbox public headers search paths to the xcconfig, with quotes' do
@fabiopelosin
CocoaPods member
fabiopelosin added a note Nov 16, 2013

You can remove the commented part.

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

This looks great, I will test it a bit when I have some time. After the final things details have been ironed out I would suggest to add a note to the Changelog and create a new pull request (or merge directly) which squashes the above commits.

@coveralls

Coverage Status

Coverage remained the same when pulling 157bdb2 on swizzlr:system_header_warning_inhibition into 9f03ab9 on CocoaPods:master.

@coveralls

Coverage Status

Coverage remained the same when pulling 8d6b012 on swizzlr:system_header_warning_inhibition into 9f03ab9 on CocoaPods:master.

@coveralls

Coverage Status

Coverage remained the same when pulling 6c7c91f on swizzlr:system_header_warning_inhibition into 9f03ab9 on CocoaPods:master.

@coveralls

Coverage Status

Coverage remained the same when pulling 4c7f139 on swizzlr:system_header_warning_inhibition into 9f03ab9 on CocoaPods:master.

@coveralls

Coverage Status

Coverage remained the same when pulling 3dd544b on swizzlr:system_header_warning_inhibition into 9f03ab9 on CocoaPods:master.

@alloy
CocoaPods member
alloy commented Nov 16, 2013

@swizzlr Slightly off-topic, can you point me to where you previously discussed the issue that this solves and how it solves it?

@swizzlr
swizzlr commented Nov 16, 2013

This effectively is it. I never opened an issue because I realised that it couldn't be fixed by inhibiting warnings the way the usual flag does it. I discussed it privately with @orta. My interest stems from the dtrace file problems, and an earlier version was offered in #1572. This has the same effect without modifying source files.

Basic explanation here: #1572 (comment)

@alloy
CocoaPods member
alloy commented Nov 16, 2013

Wait, my message doesn’t make sense :) It’s supposed to say “can you point me to where you previously discussed what the abstract issue is that this solves and how it solves it”? I understand it’s related to inhibiting warnings, but how does -isysroot influence it?

@swizzlr
swizzlr commented Nov 16, 2013

It's sparse, so I might as well do it here:

take any project and switch on -Weverything. Now inhibit_all_warnings. you'll find that whatever source files are referenced by imported headers in your target have their warnings included.

By treating them as system headers (-isystem) all warnings are suppressed, but target based ones are. So warnings will remain when building the pod according to standard warning levels. But any warnings for the user target will be locked to the user's files.

@alloy
CocoaPods member
alloy commented Nov 18, 2013

After @swizzlr pointing me to http://clang.llvm.org/docs/UsersManual.html#controlling-diagnostics-in-system-headers, I’m no completely on the same page and onboard :+1:.

@swizzlr
swizzlr commented Nov 18, 2013

Okay, well @irrationalfab has a concern about my rebuild of the integration tests. I'll update the submodule to point back to the regular repo. The travis build will break. We'll merge it, and then Fabio can rebuild the integration tests as he would like?

@alloy
CocoaPods member
alloy commented Nov 18, 2013

:shipit:

@coveralls

Coverage Status

Coverage remained the same when pulling 48891df on swizzlr:system_header_warning_inhibition into 9f03ab9 on CocoaPods:master.

@swizzlr
swizzlr commented Nov 18, 2013

May I, @irrationalfab ?

@fabiopelosin
CocoaPods member

Just tested! The patch is working really nicely. Amazing investigative work @swizzlr! :+1:

A final consideration: this should be enabled as well for the Pods targets. I.e. they should not include the warnings of their dependencies (as would just duplicate them). For a given Pod-target the headers of this dependencies should be considered system headers as well.

I don't thin that it is possible to implement it at the moment because we make all the headers visible to all the Pods. This mean that we would end up marking the headers of the Pod (the one build by the target) as system. Although Xcode might still check the warnings as for those headers the file references have been added to the target.

@fabiopelosin
CocoaPods member

Please go ahead!

@swizzlr swizzlr merged commit 02d2bb9 into CocoaPods:master Nov 18, 2013

1 check failed

Details default The Travis CI build failed
@swizzlr
swizzlr commented Nov 18, 2013

WOOHOO

:beers:

@fabiopelosin
CocoaPods member

Noooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo... you forgot about the changelog! :smile:

@swizzlr
swizzlr commented Nov 18, 2013

I know I know I'm doing it now oh my god please revoke my push access

@fabiopelosin
CocoaPods member

lol!!!

@alloy
CocoaPods member
alloy commented Nov 19, 2013

Hahaha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.