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

Fix crash when adding sub project. #762

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HDB-Li
Copy link

@HDB-Li HDB-Li commented Jun 8, 2020

I once mentioned a PR, but that branch was deleted by me, so I proposed a new one.

745 PR

In the current version(1.16.0), if you add a sub project to the project, the application will be crash when running, because the sub project will be added to the original Products PBXGroup, that's why it crashed. After manually adding the sub project through analysis, Apple will create a new Products PBXGroup, and add the sub project to the newly created Products PBXGroup, so we can do the same thing as Apple.
Another small problem, the remotoInfo of PBXContainerItemProxy should be the name of the project, but now is 'Subproject'.

@HDB-Li
Copy link
Author

HDB-Li commented Jun 9, 2020

@dnkoutso Hi, I resubmitted my PR, everything is ok now. I'm waiting for a review. 😄

@HDB-Li
Copy link
Author

HDB-Li commented Jun 10, 2020

Clear commit history.

@HDB-Li HDB-Li reopened this Jun 10, 2020
@HDB-Li HDB-Li requested a review from dnkoutso June 13, 2020 14:41
Copy link
Author

@HDB-Li HDB-Li left a comment

Choose a reason for hiding this comment

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

😄

@HDB-Li
Copy link
Author

HDB-Li commented Jun 20, 2020

@dnkoutso I'm waiting for a review.

@dnkoutso
Copy link
Contributor

I am still trying to understand the fix and the issue. It also seems like a duplicate PR to #728.

I want to ensure this is not a breaking change for anyone.

@HDB-Li
Copy link
Author

HDB-Li commented Jun 20, 2020

@dnkoutso Fix the same problem in that PR. but that PR didn't work as same as Apple. remote_info also need changed. You can try to add a sub project to a project. to reappear the crash and find the differents between Apple and Xcodeproj.

@dnkoutso
Copy link
Contributor

@HDB-Li does this fix also fix this old issue CocoaPods/CocoaPods#7728?

@HDB-Li
Copy link
Author

HDB-Li commented Jun 24, 2020

@dnkoutso No.

@HDB-Li
Copy link
Author

HDB-Li commented Jun 27, 2020

@dnkoutso Do I need to provide a demo to describe what issue I had modified?

@dnkoutso
Copy link
Contributor

@HDB-Li apologies for this taking too long. I am uncertain whether this ends up being a breaking change for folks or not. Might not be but I want to somehow verify that. Can you post screenshots of what Xcode does manually?

@HDB-Li
Copy link
Author

HDB-Li commented Jul 18, 2020

@dnkoutso Hi, I've been working a lot recently, and I've been responding late. I have provided you with a set of demos. You can compare and see how the system works, how Xcodeproj does it, and how my version does it.

Demo : https://github.com/HDB-Li/XCodeProjDemos

Look forward to your reply.

@HDB-Li
Copy link
Author

HDB-Li commented Jul 18, 2020

I also provided a screenshot in demo.

@HDB-Li
Copy link
Author

HDB-Li commented Jul 22, 2020

@dnkoutso

@HDB-Li
Copy link
Author

HDB-Li commented Aug 27, 2020

@dnkoutso Hi

@HDB-Li
Copy link
Author

HDB-Li commented Oct 8, 2020

@dnkoutso Hi, Is there no reply to this question?

@HDB-Li
Copy link
Author

HDB-Li commented Apr 13, 2021

Did this problem fixed?

@HDB-Li
Copy link
Author

HDB-Li commented Apr 13, 2021

@dnkoutso hi

@avetiso
Copy link

avetiso commented Jul 30, 2021

@dnkoutso Hello, this crashing is causing a lot of issues for our mobile development team at MX Technologies. Is there any intent to fix it? If not, we will consider forking and maintaining a separate version of xcodeproj internally so we can fix the problem for our team, but would prefer not to if this can be fixed somehow.

Would love to hear back. Thanks!

@dnkoutso
Copy link
Contributor

hey @avetiso this PR needs a rebase and potentially an additional test?

We can see if we can get it merged but its relatively old now and needs a rebase at least.

@avetiso
Copy link

avetiso commented Jul 30, 2021

hey @avetiso this PR needs a rebase and potentially an additional test?

We can see if we can get it merged but its relatively old now and needs a rebase at least.

Okay. Let me see if I can find one of our devs that are most familiar with Xcode and our build system to take a look at this, as it's not my area of expertise.

@dnkoutso
Copy link
Contributor

FWIW cocoapods does offer multiple project generation for its projects so it must be getting the sub-project references right.

I saw similar code in cocoapods here https://github.com/CocoaPods/CocoaPods/blob/master/lib/cocoapods/native_target_extension.rb

@@ -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)
Copy link
Contributor

@dnkoutso dnkoutso Jul 30, 2021

Choose a reason for hiding this comment

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

does Xcode keep adding a new "Products" group for each subproject?

@avetiso avetiso mentioned this pull request Aug 4, 2021
@deberle
Copy link

deberle commented Jan 6, 2022

I'd love to see this merged!

@avetiso
Copy link

avetiso commented Jan 6, 2022

@dnkoutso just FYI regarding this issue, we were able to find a way to fix the linker phase with this patch. We've pulled xcodeproj internal to apply this patch anyways, but it may be that the error with adding libraries to linker phase may have simply been use misusing other xcodeproj functionality.

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

4 participants