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.

Sign up for GitHub

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

Jump to bottom

Do not automatically add -ObjC flag when integrating Objective-C dependencies #6244

Merged
merged 2 commits into from May 14, 2024
Merged

Do not automatically add -ObjC flag when integrating Objective-C dependencies #6244

merged 2 commits into from May 14, 2024
+16 −38

Conversation

thedavidharris
Copy link
Collaborator

@thedavidharris thedavidharris commented Apr 30, 2024 •
edited

This reverts commit 9297022.

Resolves #6243

Short description 📝

This reverts a change that automatically places -ObjC compiler flags with no opt-out behavior. This can cause issues in projects and make them unbuildable or bloat their binary size unnecessarily.

How to test the changes locally 🧐

Test with app_with_spm_dependencies fixture and see that the -ObjC flag is not automatically added

Contributor checklist ✅

  • The code has been linted using run mise run lint:fix
  • The change is tested via unit testing or acceptance testing, or both
  • The title of the PR is formulated in a way that is usable as a changelog entry
  • In case the PR introduces changes that affect users, the documentation has been updated

Reviewer checklist ✅

  • The code architecture and patterns are consistent with the rest of the codebase
  • Reviewer has checked that, if needed, the PR includes the label changelog:added, changelog:fixed, or changelog:changed, and the title is usable as a changelog entry

fortmarek
fortmarek reviewed May 2, 2024
View reviewed changes
Copy link
Member

@fortmarek fortmarek left a comment

Choose a reason for hiding this comment

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

I am aligned with reverting the change but then we need to add good docs around this. I would add a section noting what to do when someone runs into the runtime error somewhere in this page. Would you mind adding that as a part of this PR?

Tagging @freak4pc, so he's aware of this change.

@freak4pc
Copy link
Sponsor Contributor

freak4pc commented May 2, 2024

I am aligned with reverting the change but then we need to add good docs around this. I would add a section noting what to do when someone runs into the runtime error somewhere in this page. Would you mind adding that as a part of this PR?

Tagging @freak4pc, so he's aware of this change.

Thanks 🙏 we'll try to add manually where needed. Still seems to me Apple has some other built in behavior in SPM that makes things work even without this flag, so pretty interesting what that could be

@thedavidharris
Copy link
Collaborator Author

@fortmarek absolutely, coming back to this today

@freak4pc can you confirm which of your dependencies are running into this and see if any of the other resolutions mentioned in #6243 work? Happy to chat over Slack as well to see if we can identify any other patterns/resoloutions.

I am definitely curious as to why this seems to work with SPM but Apple definitely does some funky linking things there. Yet, some docs (like Firebase) seemed to say it was needed for SPM.

I do think the best long term strategy is for Tuist to support something similar to Bazel's alwayslink that then uses force-load flags, although this is definitely trickier.

@thedavidharris thedavidharris force-pushed the dh/revert-objc-linker-flag-changes branch from 8bc3ef9 to e50e257 Compare May 2, 2024 17:18
@freak4pc
Copy link
Sponsor Contributor

freak4pc commented May 2, 2024

@freak4pc can you confirm which of your dependencies are running into this and see if any of the other resolutions mentioned in #6243 work? Happy to chat over Slack as well to see if we can identify any other patterns/resoloutions.

The issued was with GoogleSignIn, IMO. I'll see if we have some time to debug this anytime soon.

@thedavidharris thedavidharris force-pushed the dh/revert-objc-linker-flag-changes branch from 970bcc0 to a61ffce Compare May 3, 2024 19:44
@fortmarek fortmarek changed the title Revert "Fix runtime error when using objc dependencies (#5929)" Do not automatically add -ObjC flag when integrating Objective-C dependencies May 6, 2024
pepicrft
pepicrft approved these changes May 6, 2024
View reviewed changes
Copy link
Contributor

@pepicrft pepicrft left a comment

Choose a reason for hiding this comment

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

Very onboard with this change @thedavidharris. Until today, I had been using that flag without 100% understanding the role that it played when compiling Objective-C code. I think the documentation update should be sufficient for developers to understand any issue arising from their integration with Objective-C code, and since we allow overriding the build settings, I think we are all good.

@thedavidharris thedavidharris force-pushed the dh/revert-objc-linker-flag-changes branch from a61ffce to 8dbd8b3 Compare May 7, 2024 16:25
@thedavidharris thedavidharris force-pushed the dh/revert-objc-linker-flag-changes branch from 8dbd8b3 to bba8f66 Compare May 7, 2024 20:19
@pepicrft pepicrft added the changelog:fixed PR will be listed in the Fixed section of CHANGELOG label May 14, 2024
@pepicrft pepicrft merged commit 9f0b55b into main May 14, 2024
8 checks passed
@pepicrft pepicrft deleted the dh/revert-objc-linker-flag-changes branch May 14, 2024 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fortmarek fortmarek fortmarek left review comments

@pepicrft pepicrft pepicrft approved these changes

Assignees
No one assigned
Labels
changelog:fixed PR will be listed in the Fixed section of CHANGELOG
Projects
None yet
Milestone
No milestone
4 participants
@thedavidharris @freak4pc @pepicrft @fortmarek