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

[PodTarget] Ensure the deployment target is high enough for frameworks #4537

Merged
merged 5 commits into from Dec 1, 2015

Conversation

segiddins
Copy link
Member

closes #4552.

  • Documentation
  • CHANGELOG (fixing a bug introduced since 0.39.0)
  • Specs
  • Rebuild integration specs

This is really really gross, but more correct. Would appreciate suggestions on how to refactor it.

@segiddins
Copy link
Member Author

@CocoaPods/core thoughts on this one, please?

@neonichu
Copy link
Member

@segiddins I'm not quite sure what this even does :) Could you outline the problem a bit?

@segiddins
Copy link
Member Author

Makes sure the iOS deployment target is not less than 8.0 when using frameworks.

@neonichu
Copy link
Member

I thought that we would already do that, that's what confused me, but turns out it is only something we check when validating the deployment target of a Pod.

@@ -88,12 +88,30 @@ def label
end
end

# @return [Platform] the platform for this target.
# The deployment target for the pod target, which is the maximum of all
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This documentation probably wrongly copied.

@segiddins segiddins force-pushed the seg-deployment-target-8-frameworks branch from 604d4c9 to f6a9018 Compare November 30, 2015 19:49
@segiddins
Copy link
Member Author

I don't understand why the integration specs are failing in the way they are? Think I have a fix for that

@segiddins
Copy link
Member Author

@CocoaPods/core please review. I will update the integration specs and merge once i get a thumbs-up

@manuyavuz
Copy link
Member

@segiddins Is the change in integration specs for install_custom_module_name normal? User project has deployment target of 8.2, previous execution output was including - Installing target 'SocketRocket' iOS 8.2 as expected, but with your changes, it has returned to - Installing target 'SocketRocket' iOS 8.0

I'm not sure this is an expected change.

@segiddins
Copy link
Member Author

Yes, this is exactly the expected change, since the pod's deployment target (as set in the podspec) is 8.0

@manuyavuz
Copy link
Member

Could you direct me to the corresponding podspec? I see ios 6.0 here: https://github.com/CocoaPods/Specs/blob/master/Specs/libPusher/1.6.1/libPusher.podspec.json Does it use a different podspec?

@segiddins
Copy link
Member Author

Ah sorry, was thinking about the other spec. No, it uses that podspec, but its integrating as a framework, so the minimum deployment target shouldn't be lower than 8.0

@manuyavuz
Copy link
Member

But the thing is it was also having iOS 8.2 before, and downgraded to 8.0. I think I misunderstand the proposed fix here. Previous to your changes, Pods project was getting deployment target from user project, which was causing wrong deployment target setups, and with your change, deployment targets for Pods project and pod targets are solely determined via constraints in podfile or podspecs Therefore, it correctly downgraded to 8.0 because only requirement for the pod was use_frameworks! The 8.2 requirement set by user project should not effect the requirement of Pods project. Am I right?

@segiddins
Copy link
Member Author

it being 8.2 earlier was probably a bug. but yes, you're correct that user project deployment targets should no longer effect the deployment targets for podtargets

@manuyavuz
Copy link
Member

So we could directly observed that your change really ensures the deployment target requirement for pod targets :)

Thumbs up from me!

@segiddins
Copy link
Member Author

:shipit:

segiddins added a commit that referenced this pull request Dec 1, 2015
…works

[PodTarget] Ensure the deployment target is high enough for frameworks
@segiddins segiddins merged commit d0b0610 into master Dec 1, 2015
@segiddins segiddins deleted the seg-deployment-target-8-frameworks branch December 1, 2015 01:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deployment target is set incorrectly
3 participants