Fix dtrace and inhibit_all_warnings! #1560

Closed
wants to merge 10 commits into
from

Projects

None yet

4 participants

@swizzlr
Contributor
swizzlr commented Nov 7, 2013

This modification fixes CocoaPods/CocoaPods#1510, also brought up in ReactiveCocoa/ReactiveCocoa#866 and Cocoapods/Specs#4620.

@fabiopelosin
Member

Please add adequate tests for your patch and an entry to the changelog crediting yourself.

@swizzlr
Contributor
swizzlr commented Nov 8, 2013

Adding a dtrace file now causes several tests to fail, I think due to poorly written tests. I would be remiss if I didn't mention that it's damn stupid to have duplicated constants in multiple source files. In future the tests ought to read from the folder for checking source file presence.

@swizzlr
Contributor
swizzlr commented Nov 8, 2013

(I'll file a bug)

@coveralls

Coverage Status

Coverage remained the same when pulling 586a2b5 on swizzlr:fixdtrace into cb5e0db on CocoaPods:master.

@coveralls

Coverage Status

Coverage remained the same when pulling 586a2b5 on swizzlr:fixdtrace into cb5e0db on CocoaPods:master.

@alloy alloy and 1 other commented on an outdated diff Nov 8, 2013
...ds/installer/target_installer/pod_target_installer.rb
@@ -40,8 +40,11 @@ def add_files_to_build_phases
consumer = file_accessor.spec_consumer
flags = compiler_flags_for_consumer(consumer)
source_files = file_accessor.source_files
- file_refs = source_files.map { |sf| project.reference_for_path(sf) }
- target.add_file_references(file_refs, flags)
+ regular_file_refs = source_files.reject {|sf| sf.extname == ".d" }.map { |sf| project.reference_for_path(sf) }
+ target.add_file_references(regular_file_refs, flags)
+ regfiles = source_files.reject {|sf| sf.extname == ".d" }
@alloy
alloy Nov 8, 2013 Member

Instead of duplicating the filter code, can you define the list once? E.g.

all_source_files = file_accessor.source_files
source_files = all_source_files.reject { |sf| sf.extname == ".d" }

(Also note that we don’t do abbreviated var names like regfiles.)

@swizzlr
swizzlr Nov 8, 2013 Contributor

Can do. I've removed the change until I can get the tests to fail, I'll be sure to add it back in properly styled and factored.

@swizzlr

This is passing. I want it to add the -fobjc-arc flag to the source files indiscriminately – how can I get it to do that?

@coveralls

Coverage Status

Coverage remained the same when pulling 792a49b on swizzlr:fixdtrace into cb5e0db on CocoaPods:master.

@coveralls

Coverage Status

Coverage remained the same when pulling 355e27b on swizzlr:fixdtrace into cb5e0db on CocoaPods:master.

@swizzlr
Contributor
swizzlr commented Nov 8, 2013

I think this is good to merge. A neat squash is beyond me.

@coveralls

Coverage Status

Coverage remained the same when pulling 355e27b on swizzlr:fixdtrace into cb5e0db on CocoaPods:master.

@swizzlr
Contributor
swizzlr commented Nov 10, 2013

Bump!

@fabiopelosin
Member

@swizzlr I have squashed and merged this patch in 740dfc7. Thanks for the contribution!

I'm not sure if this intentional or not, but in the later case you might want to update the email git uses for your commits: https://help.github.com/articles/why-are-my-commits-linked-to-the-wrong-user

@swizzlr
Contributor
swizzlr commented Nov 11, 2013

Sorry about that – fixed it now (thanks for the heads up). Sorry I couldn't rebase, I tried it like five times to squash into three neat commits (tests, code, contrib) but managed to fork it up every time! Thanks for tidying that up for me :)

@swizzlr swizzlr deleted the swizzlr:fixdtrace branch Nov 11, 2013
@fabiopelosin
Member

Np, I squashed because you asked. Otherwise I would have merged as it is. 😄 Btw, you might interested in $ git rebase -i master to squash the commits or in something like:

$ git checkout master
$ git pull
$ git checkout -b new_branch
$ git merge --squash old_brach
# Make a new pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment