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

Revert to real CoreFoundation based plist handling. #200

Merged
merged 21 commits into from
Oct 7, 2014

Conversation

alloy
Copy link
Member

@alloy alloy commented Oct 5, 2014

This is an implementation of #198.

Instead of using a C extension, this uses the Fiddle API that comes with Ruby >= 1.9.3.

The obvious question is what to do with Ruby 1.8.7. A simple solution would be to keep including the C ext build for a few more versions or alternatively kill 1.8.7 support now.

/cc @CocoaPods/core

@segiddins
Copy link
Member

Personally, I don't think we should kill 1.8.7 support quite yet, and that making those still on it build fiddle's ext is fine.

@neonichu
Copy link
Member

neonichu commented Oct 6, 2014

1.8.7 🔥

Xcode 6 is out for some weeks now and there's always older versions of CP for those still relying on 1.8.7

@segiddins
Copy link
Member

But have app store submissions with Xcode 5 been disabled?

@alloy
Copy link
Member Author

alloy commented Oct 6, 2014

Especially since CocoaPods 0.34 is going to get a bug fix version bump, it seems like a bad moment to drop support now. I think that including the prebuild C ext that we included previously is good enough, it doesn’t even need the C source etc. If somebody is going to build Ruby 1.8.7 from source they are just cray cray and should be installing a current Ruby instead.

With CocoaPods 0.35 we can drop Ruby 1.8.7 support.

@AliSoftware
Copy link
Contributor

We had an issue with one of our current project no later than today: one dev of the team did a pod install to add a new pod, and commit+pushed it, thus pushing the XML version of the project.pbxproj. But some other dev did add a file on his branch and when he went to merge, it was a mess, as of course GIT was trying to merge his ASCII-plist pbxproj against the XML-plist pbxproj from the repo*.

In the meantime I suggested them to install @0xced 's xcproj tool on their respective machine, but we definitely need to make CocoaPods/Xcodeproj generate ASCII-plist pbxproj again instead of XML!


* By the way, I was surprised that a pod install did modify the application's xcodeproj and not just the Pods.xcodeproj. It probably has something to do with the fact that this project's Podfile contains a post_install hook?

But the post_install hook only modify a Pod's project and shouldn't touch the App's xcodeproj, right?!
Seems to me that when there is a post_install hook, CP do open and resave the App's project without any other modification, than generating new UUIDs (I'm eager to see #175 implemented!) and turing the pbxproj file to XML?

@segiddins
Copy link
Member

@AliSoftware CocoaPods has to touch the Application's project file to make sure that target dependencies and configuration files are properly set.

@alloy
Copy link
Member Author

alloy commented Oct 6, 2014

In the meantime I suggested them to install @0xced 's xcproj tool on their respective machine, but we definitely need to make CocoaPods/Xcodeproj generate ASCII-plist pbxproj again instead of XML!

@AliSoftware We never did that, though. The CoreFoundation APIs will read the ASCII format, but they will not create them. The only way this was ever supported was by installing xcproj.

By the way, I was surprised that a pod install did modify the application's xcodeproj and not just the Pods.xcodeproj.

@fabiopelosin recently mentioned that there’s a bug in 0.34 which makes it always touch the user’s project. I thought he had filed a ticket for it, but I can’t even find the comment right now.

@fabiopelosin Can you file a ticket about this issue and why it’s occurring at all?

@AliSoftware
Copy link
Contributor

@segiddins I get that sometimes CP has to change the Application's project to change the target dependencies and configuration files, but it would be really great if it wouldn't open the Application's project and resave it if there were no change to be done ;)

For example open the Application project to check if the target deps and config files were already set as expected, but then only call save on the Application's Xcodeproj object if it were modified, and don't save it over if nothing was changed.

That way the Application's project would keep its original UUIDs and its ASCII format — instead of generating XML format and new UUIDs that messes with GIT merges 😉

But it probably is what was implemented in earlier versions but has been broken in 0.34 like @alloy did just mention 😜

@AliSoftware
Copy link
Contributor

@AliSoftware We never did that, though. The CoreFoundation APIs will read the ASCII format, but they will not create them. The only way this was ever supported was by installing xcproj.

@alloy any plan on making Xcodeproj generate a ASCII-plist pbxproj project natively (without xcproj or any need to install an additional tool) in a near or far future?

@0xced
Copy link
Contributor

0xced commented Oct 6, 2014

This is being discussed in #199.

@AliSoftware
Copy link
Contributor

@fabiopelosin recently mentioned that there’s a bug in 0.34 which makes it always touch the user’s project. I thought he had filed a ticket for it, but I can’t even find the comment right now.

@alloy I found that in CocoaPods/CocoaPods#2561, that's probably the comment you were talking about? (We still need to open an issue about it, though)

@alloy
Copy link
Member Author

alloy commented Oct 6, 2014

@AliSoftware Yes, that’s the one!

@AliSoftware
Copy link
Contributor

This is being discussed in #199.

Thx @0xced !

@alloy
Copy link
Member Author

alloy commented Oct 6, 2014

Somebody verified if the current state of this branch works on 10.8 with Ruby 1.8.7 and it works fine! 🤘

https://gist.github.com/alloy/0d0b5aca8a1b80004f3e

(That one failure is fine, it’s not supposed to work on 1.8.7 but I didn’t guard it.)

@orta
Copy link
Member

orta commented Oct 7, 2014

0.35 sounds like a good time to drop 1.8.7 to me too. As it may come with swift support it'd make sense to base our tooling on the now present tooling.

alloy added a commit that referenced this pull request Oct 7, 2014
Revert to real CoreFoundation based plist handling.

Fixes #198.
@alloy alloy merged commit 52e62ce into master Oct 7, 2014
@alloy alloy deleted the CoreFoundation-Fiddle branch October 7, 2014 18:36
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.

None yet

6 participants