Fix crashing subprojects #851
Conversation
CHANGELOG.md
Outdated
| @@ -138,6 +138,10 @@ | |||
| [nickgravelyn](https://github.com/nickgravelyn) | |||
| [#757](https://github.com/CocoaPods/Xcodeproj/pull/757) | |||
|
|
|||
| * Fix crash when adding sub project. | |||
There was a problem hiding this comment.
nit: please add two empty spaces after the period for markdown formatting.
also add your name for credit!
CHANGELOG.md
Outdated
| @@ -138,6 +138,10 @@ | |||
| [nickgravelyn](https://github.com/nickgravelyn) | |||
| [#757](https://github.com/CocoaPods/Xcodeproj/pull/757) | |||
|
|
|||
| * Fix crash when adding sub project. | |||
| [HDB-Li](https://github.com/HDB-Li) | |||
| [#762](https://github.com/CocoaPods/Xcodeproj/pull/762) | |||
There was a problem hiding this comment.
Sorry, modified before I saw these comments.
| @@ -180,15 +180,16 @@ def new_subproject(group, path, source_tree) | |||
| ref = new_file_reference(group, path, source_tree) | |||
| ref.include_in_index = nil | |||
|
|
|||
| product_group_ref = find_products_group_ref(group, true) | |||
| product_group_ref = group.project.new(PBXGroup) | |||
There was a problem hiding this comment.
should we create a new group each time?
There was a problem hiding this comment.
Waiting to hear back from a team member regarding our Xcode set up. Will get back to you as soon as I know.
There was a problem hiding this comment.
It works the same for us. The concern we had with having this be a new group was that our subprojects would generate as a new subproject folder instead of a new subproject xcodeproj file in the navigator. I just ran through this with our main engineer that set up our Xcode project and verified that it still sets up a new .xcodeproj file as we want.
As far as side effects however, I don't believe him or I can really speak beyond just our project. If you have any other people that are very familiar with Xcode that want to take a look at this I'd be happy to see the input and correct anything that's wrong, but that's out of my expertise at that point.
I just know that this fixes our crashing when trying to change our project settings.
Happy to hear any concerns and implement any changes.
Also, I found this article that describes a much more verbose fix for the same issue. Take a look and compare and let me know what you think, perhaps their solution is better? https://www.programmersought.com/article/83996170465/
There was a problem hiding this comment.
Any feedback? Sorry we can't be of more help.
There was a problem hiding this comment.
I will need to read the article a bit and think about it.
The reason why I've been so apprehensive on this change is because I want to ensure we will not ship a version of Xcodeproj that will break existing projects unexpectedly.
I will be checking out your branch though and also see what Xcode vanilla does when adding a new sub project reference so I can get a full understanding of the change.
There was a problem hiding this comment.
Sorry for the long delay, I recommend for now to fork and proceed. I have not had time to review this and evaluate it. I apologize but normal day-to-day work stuff................
There was a problem hiding this comment.
No worries, totally get it. We will patch and host it locally for now, and when you get to this and check it out, it would be great if you could let us know so we can revert back to the public one. :)
There was a problem hiding this comment.
Just FYI, after doing more of our own internal testing it looks like this ends up breaking one of the entries in our linking phase.
There was a problem hiding this comment.
thats "good" to hear I guess! Without some deeper investigation I would be afraid to land this. Again really sorry for lack of traction here, the repercussions can be large.
There was a problem hiding this comment.
Yeah, totally. Wouldn't want to break anything for anyone.
Looking around though, it does seem like this has been an issue for multiple people. Are there any plans to investigate this further? Or, perhaps, to write up a guide on how to properly create subprojects so that we can see if perhaps we are mis-using the library?
| # | ||
| # @return [String] The remote info name. | ||
| # | ||
| def find_remote_info_name_for_product_reference(ref, project) |
There was a problem hiding this comment.
I think you can re-write this as:
def find_remote_info_name_for_product_reference(ref, project)
project.native_targets.find { |t| t.product_reference == ref }&.name || 'Subproject'
end There was a problem hiding this comment.
RuboCop complains over this for using the 2.1 parser. Want me to update to 2.3?
There was a problem hiding this comment.
It complains about the ampersand. Changing to 2.3 complains about frozen string literals and freezing immutable objects and other stuff.
Picking up where #762 left off. Updated the specs to show what the new remote_info actually is. Fixed some whitespacing.