Skip to content

Enable ARC for key modules and add Swift Package Manager support#533

Closed
bc-lee wants to merge 2 commits intogoogle:mainfrom
bc-lee:feature/spm-arc
Closed

Enable ARC for key modules and add Swift Package Manager support#533
bc-lee wants to merge 2 commits intogoogle:mainfrom
bc-lee:feature/spm-arc

Conversation

@bc-lee
Copy link
Copy Markdown
Contributor

@bc-lee bc-lee commented Jul 25, 2025

This pull request enables ARC (Automatic Reference Counting) for the GTMLogger and GTMStringEncoding modules by removing manual memory management code. It also introduces Swift Package Manager (SPM) support, making it easier to manage dependencies and integrate the library into modern Swift projects.

Related: #258
Related: https://issuetracker.google.com/issues/174418229

@thomasvl
Copy link
Copy Markdown
Member

First up, thank you for the PR! 🎉

I'd suggest split this up to talk about things in a few chunks. Would you mind splitting off the ARC changes to be a PR for each module? That way just incase there is a hiccup with anything downstream we have specific commits we could revert if need be. Long ago we saw some code become hotspots when moving to ARC, so while I don't think that would happen this time it would be nice to have distinct commits for each target being moved so things can be easily reverted if something did come up.

As for support for SwiftPM, I wouldn't say this "Fixes #258", since it would only do a few targets. 😄 But supporting SwiftPM work is going to be a slightly larger discussion and worth a distinct PR. If you look at google/gtm-session-fetcher and google/google-api-objectivec-client-for-rest you'll see we had to go a very different layout than the includes directory and links. The problem we hit was downstream code that does something like #import <GTM/GTMLogger.h> didn't work as the dependency moved between CocoaPods and SwiftPM. The layout we used in those was what the Firebase folks found they had to use to make the transition possible. And this complexity is sorta why we've been slow to add SwiftPM support here, the restructuring can be non trivial. Anyway, lets move the SwiftPM part to a distinct PR, and see if there's a good way to test consumption to make sure someone could migrate something already working in CocoaPods to a new release with any changes to layout and then migration to SwiftPM without a need for lots more changes in their code.

@bc-lee
Copy link
Copy Markdown
Contributor Author

bc-lee commented Jul 29, 2025

Thanks for the thoughtful review!

I'll go ahead and split this PR into multiple smaller ones as you suggested, to make reviewing and potential reverts easier.

Regarding SwiftPM support — I think that we don't need to convert all google-toolbox-for-mac targets right away. I’ve elaborated a bit more on this in my other comment. My initial assumption was that we could avoid changing the layout by introducing proper symlinks for headers and sources, allowing SwiftPM to recognize them without requiring structural changes. As an external contributor, I don’t have visibility into or responsibility for any internal google3 changes that might be affected by a layout modification. I also assumed the project is consumed internally using Bazel/Blaze, so I was cautious about introducing layout changes that could potentially disrupt that setup.

But I’ll take a closer look at the approach used in the other repos you mentioned. It makes sense to align with what’s proven to work downstream.

I'll keep this PR open while I work on splitting out the necessary changes into separate PRs. Once those are ready, I’ll close this one.

@thomasvl
Copy link
Copy Markdown
Member

Regarding SwiftPM support — I think that we don't need to convert all google-toolbox-for-mac targets right away. I’ve elaborated a bit more on this in my other comment

Replied there also (saw that first), I agree we don't have to convert everything at once. If this is in reference to my comment:

I wouldn't say this "Fixes #258", since it would only do a few targets. 😄

Then my point wasn't that everything had to be done right now, just that the use of "Fixes #258" would close that issue as complete, and I don't think we should close it until everything is supported.

@bc-lee bc-lee marked this pull request as draft July 30, 2025 00:02
@bc-lee
Copy link
Copy Markdown
Contributor Author

bc-lee commented Aug 1, 2025

This PR will be replaced by #540.

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.

3 participants