Duplicate code in generated prefix header file for Pods with subspecs #1449

Closed
rivera-ernesto opened this Issue Oct 8, 2013 · 17 comments

Comments

Projects
None yet
9 participants
@rivera-ernesto
Contributor

rivera-ernesto commented Oct 8, 2013

The current prefix_header generator adds the prefix_header_contents or prefix_header_file multiple times:

  • Once for the Pod spec.
  • Additional times for each subspec.

For instance the following Podspec:

Pod::Spec.new do |s|
    s.name          = "NBUCore"
    # ...
    s. prefix_header_contents = '#import "NBUCorePrivate.h"'

    s.subspec 'UI' do |su|
        su.source_files = 'Source/UI/*.{h,m}'
    end

    s.subspec 'Helpers' do |sh|
        sh.source_files = 'Source/Helpers/*.{h,m}'
    end

    s.subspec 'Additions' do |sa|
        sa.source_files = 'Source/Additions/*.{h,m}'
    end

    s.subspec 'Dashboard' do |sd|
        sd.source_files = 'Source/Dashboard/*.{h,m}'
        sd.resources    = 'Source/Dashboard/*.{xib}'
    end
end

Results in the following prefix file:

#ifdef __OBJC__
#import <UIKit/UIKit.h>
#endif

#import "Pods-environment.h"
#import "NBUCorePrivate.h"

#import "NBUCorePrivate.h"

#import "NBUCorePrivate.h"

#import "NBUCorePrivate.h"

#import "NBUCorePrivate.h"
@fabiopelosin

This comment has been minimized.

Show comment
Hide comment
@fabiopelosin

fabiopelosin Oct 8, 2013

Member

The prefix headers are a build setting and thus they are inherited by subspecs. Indeed, it would be a good idea to use only unique entries in the list of the prefix headers. Are you interested in submitting a patch?

In any case I would discourage the usage shown above.

Member

fabiopelosin commented Oct 8, 2013

The prefix headers are a build setting and thus they are inherited by subspecs. Indeed, it would be a good idea to use only unique entries in the list of the prefix headers. Are you interested in submitting a patch?

In any case I would discourage the usage shown above.

@rivera-ernesto

This comment has been minimized.

Show comment
Hide comment
@rivera-ernesto

rivera-ernesto Oct 8, 2013

Contributor

I tried to setup a Ruby environment to do it now to try to test it but I have some things to finish first here.
It seems that it would suffice to stop using a loop here.

Also I would discourage it in most cases but could be useful for some projects.

Contributor

rivera-ernesto commented Oct 8, 2013

I tried to setup a Ruby environment to do it now to try to test it but I have some things to finish first here.
It seems that it would suffice to stop using a loop here.

Also I would discourage it in most cases but could be useful for some projects.

@fabiopelosin

This comment has been minimized.

Show comment
Hide comment
@fabiopelosin

fabiopelosin Oct 8, 2013

Member

I tried to setup a Ruby environment to do it now to try to test it but I have some things to finish first here.
It seems that it would suffice to stop using a loop here.

Doing that would just limit the import to the first Pod (and ignore any custom prefix header of the subspecs). Instead of writing them as we get them we should map them in an array, call #uniq, and write them to the file at that point.

Member

fabiopelosin commented Oct 8, 2013

I tried to setup a Ruby environment to do it now to try to test it but I have some things to finish first here.
It seems that it would suffice to stop using a loop here.

Doing that would just limit the import to the first Pod (and ignore any custom prefix header of the subspecs). Instead of writing them as we get them we should map them in an array, call #uniq, and write them to the file at that point.

@rivera-ernesto

This comment has been minimized.

Show comment
Hide comment
@rivera-ernesto

rivera-ernesto Oct 8, 2013

Contributor

The thing is that the prefix header is shared among/affects all subspecs, so probably it doesn't make sense to define them in a subspec.
The #uniq solution should be ok though.

Contributor

rivera-ernesto commented Oct 8, 2013

The thing is that the prefix header is shared among/affects all subspecs, so probably it doesn't make sense to define them in a subspec.
The #uniq solution should be ok though.

@fabiopelosin

This comment has been minimized.

Show comment
Hide comment
@fabiopelosin

fabiopelosin Oct 8, 2013

Member

The thing is that the prefix header is shared among/affects all subspecs, so probably it doesn't make sense to define them in a subspec.

Consider that a library might need a shared prefix header which should be included even if only a subspec is installed and that a single subspec might want to have a custom prefix header.

Member

fabiopelosin commented Oct 8, 2013

The thing is that the prefix header is shared among/affects all subspecs, so probably it doesn't make sense to define them in a subspec.

Consider that a library might need a shared prefix header which should be included even if only a subspec is installed and that a single subspec might want to have a custom prefix header.

@rivera-ernesto

This comment has been minimized.

Show comment
Hide comment
@rivera-ernesto

rivera-ernesto Oct 9, 2013

Contributor

Right

Contributor

rivera-ernesto commented Oct 9, 2013

Right

@frosty

This comment has been minimized.

Show comment
Hide comment
@frosty

frosty Jan 17, 2014

I'm interested in having a go at tackling this if nobody else is already. This would be my first contribution to Cocoapods - would people recommend I add a new test fixture for a Podspec like the example above?

frosty commented Jan 17, 2014

I'm interested in having a go at tackling this if nobody else is already. This would be my first contribution to Cocoapods - would people recommend I add a new test fixture for a Podspec like the example above?

@swizzlr

This comment has been minimized.

Show comment
Hide comment
@swizzlr

swizzlr Jan 17, 2014

Contributor

There may already be one – check the integration tests folder, write a new one if you have to, and familiarize yourself with the Rakefile.

Contributor

swizzlr commented Jan 17, 2014

There may already be one – check the integration tests folder, write a new one if you have to, and familiarize yourself with the Rakefile.

@CocoaPodsBot

This comment has been minimized.

Show comment
Hide comment
@CocoaPodsBot

CocoaPodsBot Mar 29, 2014

Issue has been confirmed by @segiddins

Issue has been confirmed by @segiddins

@luisdelarosa

This comment has been minimized.

Show comment
Hide comment
@luisdelarosa

luisdelarosa Mar 30, 2014

Contributor

I'm looking into this now.

Contributor

luisdelarosa commented Mar 30, 2014

I'm looking into this now.

luisdelarosa added a commit to luisdelarosa/CocoaPods that referenced this issue Mar 30, 2014

Added two tests for issue #1449 - however both are passing at the uni…
…t level so

I suspect that the issue will need to be tested at the integration test level.
@luisdelarosa

This comment has been minimized.

Show comment
Hide comment
@luisdelarosa

luisdelarosa Mar 31, 2014

Contributor

I confirmed it with the AWSiOSSDK pod.

Contributor

luisdelarosa commented Mar 31, 2014

I confirmed it with the AWSiOSSDK pod.

luisdelarosa added a commit to luisdelarosa/cocoapods-integration-specs that referenced this issue Mar 31, 2014

Add integration test that will reproduce the issue found in CocoaPods…
… issue #1449.


CocoaPods/CocoaPods#1449
"Duplicate code in generated prefix header file for Pods with subspecs".
@luisdelarosa

This comment has been minimized.

Show comment
Hide comment
@luisdelarosa

luisdelarosa Mar 31, 2014

Contributor

Failing test is available in the pull request: CocoaPods/cocoapods-integration-specs#8

Contributor

luisdelarosa commented Mar 31, 2014

Failing test is available in the pull request: CocoaPods/cocoapods-integration-specs#8

luisdelarosa added a commit to luisdelarosa/CocoaPods that referenced this issue Mar 31, 2014

luisdelarosa added a commit to luisdelarosa/CocoaPods that referenced this issue Mar 31, 2014

@luisdelarosa

This comment has been minimized.

Show comment
Hide comment
@luisdelarosa

luisdelarosa Mar 31, 2014

Contributor

Fix and integration test are available in the pull request: #1981

Contributor

luisdelarosa commented Mar 31, 2014

Fix and integration test are available in the pull request: #1981

luisdelarosa added a commit to luisdelarosa/cocoapods-integration-specs that referenced this issue Apr 8, 2014

Add integration test that will reproduce the issue found in CocoaPods…
… issue #1449.


CocoaPods/CocoaPods#1449
"Duplicate code in generated prefix header file for Pods with subspecs".

fabiopelosin added a commit that referenced this issue Apr 8, 2014

Merge branch 'issue_1449_duplicate_prefix_header_contents' of https:/…
…/github.com/luisdelarosa/CocoaPods into luisdelarosa-issue_1449_duplicate_prefix_header_contents

* 'issue_1449_duplicate_prefix_header_contents' of https://github.com/luisdelarosa/CocoaPods:
  Addressed comments from @irrationalfab: - simplified collection of unique prefix_header_contents - reduced sub specs in spec from 4 to 2 Removed inline documentation and added @notes and @todos. Added entry to Changelog.
  Fix issue #1449 by collecting the prefix_header_contents and uniq-ing them.
  Add integration test for issue #1449.
  Added two tests for issue #1449 - however both are passing at the unit level so I suspect that the issue will need to be tested at the integration test level.

Conflicts:
	CHANGELOG.md

@fabiopelosin fabiopelosin closed this in #1981 Apr 8, 2014

@rivera-ernesto

This comment has been minimized.

Show comment
Hide comment
@rivera-ernesto

rivera-ernesto Apr 9, 2014

Contributor

Thank you @luisdelarosa !

Contributor

rivera-ernesto commented Apr 9, 2014

Thank you @luisdelarosa !

@yairsz

This comment has been minimized.

Show comment
Hide comment
@yairsz

yairsz Feb 19, 2015

Hello, I'd like to report that the duplication behavior is still happening for prefix_header_file.
This is in version 0.35.0.
To work around it, I set the variable inside only one of the subspecs. The header file was injected only once into the pods .pch
Thank you @luisdelarosa . This information was very useful!

yairsz commented Feb 19, 2015

Hello, I'd like to report that the duplication behavior is still happening for prefix_header_file.
This is in version 0.35.0.
To work around it, I set the variable inside only one of the subspecs. The header file was injected only once into the pods .pch
Thank you @luisdelarosa . This information was very useful!

@lemonkey

This comment has been minimized.

Show comment
Hide comment
@lemonkey

lemonkey Mar 1, 2015

Confirmed this is happening in 0.35.0.

lemonkey commented Mar 1, 2015

Confirmed this is happening in 0.35.0.

@sstadelman

This comment has been minimized.

Show comment
Hide comment
@sstadelman

sstadelman Mar 17, 2015

confirmed also in 0.35.0. opening new issue for prefix_header_file.

confirmed also in 0.35.0. opening new issue for prefix_header_file.

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