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

generator: header.rb: umbrella always imports UIKit on iOS, even if the pod doesn't depend on it. #6815

Closed
1 task done
r-mckay opened this issue Jun 18, 2017 · 34 comments
Closed
1 task done
Labels
s1:awaiting input Waiting for input from the original author t3:discussion These are issues that can be non-issues, and encompass best practices, or plans for the future.

Comments

@r-mckay
Copy link

r-mckay commented Jun 18, 2017

Report

What did you do?

I wanted to install pods that do not depend on UIKit, RxSwift for example.

What did you expect to happen?

I expected that these pods that do not depend on UIKit didn't import it.

What happened instead?

The pods imported UIKit.

Project that demonstrates the issue

Adding pod RxSwift, for example, demonstrates it. It is confirmed that it does not depend on UIKit, but if I'm not mistaken it imports it when installed via CocoaPods.
Please see: ReactiveX/RxSwift#1298

r-mckay added a commit to r-mckay/CocoaPods that referenced this issue Jun 18, 2017
This commit replaces the UIKit import made in the header generator
by a Foundation import when targetting iOS and tvOS.

This is done because we don't know if the framework that is built
depends or not on UIKit. For example, frameworks that do not use UI
do not need to import UIKit.

Issue: CocoaPods#6815
r-mckay added a commit to r-mckay/CocoaPods that referenced this issue Jun 18, 2017
This commit replaces the UIKit import made in the header generator
by a Foundation import when targetting iOS and tvOS.

This is done because we don't know if the framework that is built
depends or not on UIKit. For example, frameworks that do not use UI
do not need to import UIKit.

Issue: CocoaPods#6815
r-mckay added a commit to r-mckay/CocoaPods that referenced this issue Jun 18, 2017
This commit replaces the UIKit import made in the header generator
by a Foundation import when targetting iOS and tvOS.

This is done because we don't know if the framework that is built
depends or not on UIKit. For example, frameworks that do not use UI
do not need to import UIKit.

Issue: CocoaPods#6815
@stale stale bot added the s1:awaiting input Waiting for input from the original author label Sep 16, 2017
@stale
Copy link

stale bot commented Sep 16, 2017

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates.

@keith
Copy link
Member

keith commented Sep 16, 2017 via email

@stale stale bot removed the s1:awaiting input Waiting for input from the original author label Sep 16, 2017
@paulb777
Copy link
Member

What's a good way to fix this without breaking all of the existing podspecs that are unknowingly depending upon the CocoaPods generated pch file?

@dnkoutso
Copy link
Contributor

@paulb777 not sure......need to think about this. Perhaps a new #if COCOAPODS_1_5 that gets set only on a newer version? Wonder if it's also considered a breaking change and must be 2.0

@keith
Copy link
Member

keith commented Sep 16, 2017

In the meantime, I've done something horrible and created a CP plugin that just always uses "Foundation" in these headers. https://github.com/keith/cocoapods-foundation-headers

This could end up being a problem if someone developed a library only using CocoaPods and is somehow relying on this behavior. If that comes up I guess I'll take arguments and allow exclusions. But in the meantime this works on our project!

@paulb777
Copy link
Member

One possible solution would be adding to s.pch = false to the podspec to disable the pch file and option generation.

Would such Core and CocoaPod implementation PRs be accepted?

@paulb777
Copy link
Member

paulb777 commented Sep 17, 2017

I discovered that I was conflating two related issues. Both the generated umbrella header and the generated pch header have the issue of bringing in an import of UIKit.h. See also #1691 and #1746 for more detailed descriptions of the pch issue. And from me in June https://stackoverflow.com/questions/44401464/is-there-a-way-to-disable-the-default-pch-file-in-cocoapods

@keith
Copy link
Member

keith commented Sep 18, 2017

FWIW this same codepath is responsible for both of those.

@paulb777
Copy link
Member

@keith I saw that, but the umbrella case is a bit more complex because the UIKit import is just a part of it instead of the whole thing.

I made #7044 for the pch case, but maybe it should be extended or changed to a skip_cocoa option to cover both the umbrella and pch?

@amorde
Copy link
Member

amorde commented Sep 18, 2017

@paulb777 What about supporting podspecs specifying the entire PCH and umbrella header? That way those who do not want it could just provide an empty header, or maybe it could provide a default but allow setting it to nil

@paulb777
Copy link
Member

@amorde Hmm. It's already possible to specify a pch to inject into the generated pch - https://guides.cocoapods.org/syntax/podspec.html#prefix_header_file. It could get confusing if a new option also allows specifying pch's.

For umbrella headers, I suspect many users want the umbrella header to be generated, but without adding UIKit.h or Cocoa.h to it.

@amorde
Copy link
Member

amorde commented Sep 18, 2017

Ah, good catch. That's unfortunate. I'm just worried about having a bunch of skip_ attributes for the podspec to fix these kind of issues. Not sure what the best approach is then.

@orta
Copy link
Member

orta commented Sep 20, 2017

@amorde good feedback, that was exactly what I was thinking also.

@paulb777 I'm also wanting to avoid a skip_x DSL attribute, and have make it obvious that including this attribute on podspec may break a build (other pods could be relying on the pch, but as one pod doesn't says skip then all of them don't get it)

spec.pod_target_shared_pch = :skip

Which would allow the attribute to potentially evolve if there are other unanticipated cases. Make sense?

@paulb777
Copy link
Member

@orta I agree that skip_x is ugly - I just hadn't come up with something better that's expressive enough.

I'm not following the logic about breaking the build, since my understanding is that a separate pch is generated for each pod.

Instead of creating a new pod_target_shared_pch, would it be better to use the existing

spec.prefix_header_file = :skip

Still, any of these approaches would solve the unwanted pch file issue but don't provide a pathway towards turning off the unwanted additional imports in generated umbrella headers.

@dnkoutso
Copy link
Contributor

I like the ability to provide a different configuration for the prefix header generation. I am more in favor as well than adding a flag to the DSL.

@orta
Copy link
Member

orta commented Sep 21, 2017

Ah, I was conflating the two of these issues:

but don't provide a pathway towards turning off the unwanted additional imports in generated umbrella headers.

my understanding is that a separate pch is generated for each pod.

Into one attribute.

I agree with the addition of :skip to the existing spec.prefix_header_file 👍.

I think for the umbrella we'll have to add a new attribute for that.

@paulb777
Copy link
Member

Sounds good. I'll update #7044 accordingly.

@stale
Copy link

stale bot commented Dec 20, 2017

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates.

@stale stale bot added the s1:awaiting input Waiting for input from the original author label Dec 20, 2017
@keith
Copy link
Member

keith commented Dec 22, 2017

It looks like that attribute has been merged, so if we don't want to go down any other route of consumers of specs being able to also disable this, (without just altering the build setting, this can be closed.

@stale stale bot removed the s1:awaiting input Waiting for input from the original author label Dec 22, 2017
@stale
Copy link

stale bot commented Mar 22, 2018

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates.

@stale stale bot added the s1:awaiting input Waiting for input from the original author label Mar 22, 2018
@dnkoutso
Copy link
Contributor

@paulb777 I think we can close this now after the new attribute was merged, basically what @keith said?

@stale stale bot removed the s1:awaiting input Waiting for input from the original author label Mar 22, 2018
@r-mckay
Copy link
Author

r-mckay commented Mar 22, 2018

Hello!

Without interfering with your decision here, could you please explain me what the current status of this is?
I've read all the merged PR from @paulb777, the prefix_header_file, skip_pch etc.
Will prefix_header_file prevent the addition of UIKit in the final Pod? I tried with this field on a custom Specs repo but I failed to prevent this addition.

Thanks!

@dnkoutso
Copy link
Contributor

@r-mckay based on the change you can set spec.prefix_header_file = false in your podspec. For posterity here's the CocoaPods github PR #7044

@dnkoutso
Copy link
Contributor

@r-mckay this also seems available already in CocoaPods 1.4.0.

@r-mckay
Copy link
Author

r-mckay commented Mar 22, 2018

@dnkoutso Yes it seems so, that's what I tried.
However I still ended with the UIKit import. I'll work on it again, maybe I missed something.

@paulb777, sorry to bother you but could you confirm that the UIKit import, with spec.prefix_header_file = false in the podspec, will be avoided?
Thank you very much, I'm asking because I failed to use it in order to remove this import.

@paulb777
Copy link
Member

#7044 turns off prefix header generation. I don't think any of the discussed solutions to turn off the UIKit import in umbrella header's were implemented.

@keith
Copy link
Member

keith commented Mar 22, 2018

I implemented a quick CocoaPods plugin that imports foundation instead, it seems to work correctly for our project https://github.com/keith/cocoapods-foundation-headers

@r-mckay
Copy link
Author

r-mckay commented Mar 23, 2018

@keith nice thank you!
About closing this issue, I'm not sure that we should since the behaviour we're dealing with seems to still be topical.

@stale
Copy link

stale bot commented Jun 21, 2018

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates.

@stale stale bot added the s1:awaiting input Waiting for input from the original author label Jun 21, 2018
@amorde
Copy link
Member

amorde commented Jun 21, 2018

@stalebot not stale

@stale stale bot removed the s1:awaiting input Waiting for input from the original author label Jun 21, 2018
@amorde amorde added the t3:discussion These are issues that can be non-issues, and encompass best practices, or plans for the future. label Aug 6, 2018
@stale
Copy link

stale bot commented Nov 4, 2018

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates.

@stale stale bot added the s1:awaiting input Waiting for input from the original author label Nov 4, 2018
@stale
Copy link

stale bot commented Nov 11, 2018

This issue will be auto-closed because there hasn't been any activity for a few months. Feel free to open a new one if you still experience this problem 👍

@stale stale bot closed this as completed Nov 11, 2018
@ivanglushko
Copy link

ivanglushko commented Jun 15, 2023

Yeah so spec.prefix_header_file = false does nothing umbrella file still has a <UIKit/UIKit.h> import :/

@ivanglushko
Copy link

I've created a cocoapods plugin for removing autoimports of UIKit.h / Cocoa.h / Foundation.h thanks @keith for template to start off :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s1:awaiting input Waiting for input from the original author t3:discussion These are issues that can be non-issues, and encompass best practices, or plans for the future.
Projects
None yet
Development

No branches or pull requests

7 participants