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

bugfix for pods containing swift frameworks #3675

Closed
wants to merge 3 commits into from
Closed

bugfix for pods containing swift frameworks #3675

wants to merge 3 commits into from

Conversation

timrosenblatt
Copy link

🌈

This a bugfix for an issue that seems to come up when running builds from the command line (although not from Xcode UI?). I work at Ship.io and we see this issue come up in a bunch of customer builds. I've also seen it reported by many other users.

The issue manifests as an rsync error 23 during the Embed Pods Frameworks phase. For some reason, the target_definition.label seems to be blank, and it results in the framework being placed up a directory.

I'm not 100% sure of the root cause. I arrived at this fix thanks to some other posters who have posted about the issue. I've patched our local copies of the gem and have validated that this does solve the problem.

I've looked at #3550 and when it is merged in, it will overwrite this fix, and will also solve the root cause.

Old copies of the generated shell script may need to be fixed manually. This can be done by basically copy and pasting this fix into place, and manually replacing the variable. This fix does solve the issue as new Pods are installed.

I'd like to thank my company, Ship.io for sponsoring this fix. They let me spend a bunch of time researching this at the company's expense, and gave me permission to share it back with the community. Also, they make a pretty darn good hosted CI system for iOS. :)

@timrosenblatt
Copy link
Author

BTW I know the specs are failing. Doesn't seem to be related to anything I changed. Any thoughts?

@segiddins
Copy link
Member

@mrackwitz does this seem right to you?

@segiddins
Copy link
Member

@timrosenblatt those are the integration specs failing. Don't worry about it, I'll update them if/when we merge.

@@ -55,6 +55,12 @@ To install release candidates run `[sudo] gem install cocoapods --pre`
[Vincent Isambart](https://github.com/vincentisambart)
[#3161](https://github.com/CocoaPods/CocoaPods/issues/3161)

* Added a fix for a bug where some Pods containing Swift frameworks were not
being written to the expected directory. Alamofire seems to be a victim in many
Copy link
Member

Choose a reason for hiding this comment

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

@timrosenblatt: Further investigation revealed that even #3550 doesn't fully resolve the underlying issue. Can you update the changelog entry accordingly and add a reference to this PR? Then we are good to go.

Copy link
Author

Choose a reason for hiding this comment

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

@mrackwitz Thanks for the info. I would like to understand why 3550 doesn't resolve it. Can you tell me where I can look to get more information?

@kylef
Copy link
Contributor

kylef commented Jun 16, 2015

👌 This fixes my issue when using CocoaPods with Swift integrated Pods with Snapshot.

@mrackwitz mrackwitz mentioned this pull request Jun 16, 2015
7 tasks
@timrosenblatt
Copy link
Author

@mrackwitz @segiddins anything else?

@segiddins
Copy link
Member

Just a rebase, then I can merge and I'll handle updating the integration specs. Thanks for the PR, @timrosenblatt :)

@basememara
Copy link

Any idea when we can get this this merged into master? Issue #3550 has been successfully merged into master 🎉 and hoping this is the last piece of the archiving/App Store blocker.

@segiddins
Copy link
Member

@basememara this will get merged once it's rebased

@segiddins segiddins added this to the 0.38 milestone Jun 25, 2015
@timrosenblatt
Copy link
Author

Need me to do anything or will you handle it?

On Thursday, June 25, 2015, Samuel E. Giddins notifications@github.com
wrote:

@basememara https://github.com/basememara this will get merged once
it's rebased


Reply to this email directly or view it on GitHub
#3675 (comment).

Tim Rosenblatt
http://www.timrosenblatt.com

If it's urgent, just call or text me at 407-719-2777

@neonichu
Copy link
Member

@timrosenblatt would be great if you could rebase this, so we can directly merge, thanks!

@basememara
Copy link

@timrosenblatt if it's easier and everyone agrees, maybe it would be better just to create a fresh pull request instead of getting tangled up with rebasing.

@kylef
Copy link
Contributor

kylef commented Jun 25, 2015

Don't worry about rebasing/creating a new PR. I'll take care of this shortly.

@timrosenblatt
Copy link
Author

OK, cool. Thank you, Kyle.

On Thu, Jun 25, 2015 at 2:52 PM, Kyle Fuller notifications@github.com
wrote:

Don't worry about rebasing/creating a new PR. I'll take care of this
shortly.


Reply to this email directly or view it on GitHub
#3675 (comment).

Tim Rosenblatt
http://www.timrosenblatt.com

If it's urgent, just call or text me at 407-719-2777

@segiddins segiddins self-assigned this Jun 25, 2015
@segiddins
Copy link
Member

Will merge via #3737.

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.

6 participants