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

Remove FXIOS-5820 [v112] Swift protobuf dependency #13331

Merged
merged 1 commit into from
Feb 23, 2023
Merged

Remove FXIOS-5820 [v112] Swift protobuf dependency #13331

merged 1 commit into from
Feb 23, 2023

Conversation

lmarceau
Copy link
Contributor

FXIOS-5820 #13304

Remove swift protobuf dependency, not needed anymore it seems

@lmarceau lmarceau changed the title FXIOS-5820 [v112] Remove swift protobuf dependency Remove FXIOS-5820 [v112] Swift protobuf dependency Feb 22, 2023
@nbhasin2
Copy link
Contributor

@lougeniaC64 I don't see any instances of Protobuf being used directly in our app

@travis79 Do you happen to know if Glean needs Protobuf for anything?

We are removing the library it in this PR but want to give you a chance to let us know if you would like us to keep it?

Thanks!! :)

If I don't hear from either one of you then we will merge it tomorrow EOD cc @lmarceau

@mte-test
Copy link

Messages
📖 Edited 2 files
📖 Created 0 files

Generated by 🚫 Danger Swift against dadf14a

@travis79
Copy link
Member

@lougeniaC64 I don't see any instances of Protobuf being used directly in our app

@travis79 Do you happen to know if Glean needs Protobuf for anything?

We are removing the library it in this PR but want to give you a chance to let us know if you would like us to keep it?

Thanks!! :)

If I don't hear from either one of you then we will merge it tomorrow EOD cc @lmarceau

I don't think Glean relies on this unless it would be coming from the UniFFI dependency or something... I think this should be fine to remove but it wouldn't hurt to smoke test that telemetry was working afterwards.

@lougeniaC64
Copy link
Contributor

@lougeniaC64 I don't see any instances of Protobuf being used directly in our app

@travis79 Do you happen to know if Glean needs Protobuf for anything?

We are removing the library it in this PR but want to give you a chance to let us know if you would like us to keep it?

Thanks!! :)

If I don't hear from either one of you then we will merge it tomorrow EOD cc @lmarceau

I think this should be okay. I would run these changes locally and ensure that the sign-in flow and send tab aren't affected as it looks like protobuf is used in viaduct which is used in fxa (though I think the iOS protobuf dependency was for our components before uniffi). cc/ @tarikeshaq as he has more fxa/push insight

tarikeshaq
tarikeshaq approved these changes Feb 23, 2023
View reviewed changes
Copy link
Contributor

@tarikeshaq tarikeshaq left a comment

Choose a reason for hiding this comment

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

Goodbye, protobufs!

Yes, we don't use protobufs for iOS, it's used by viaduct in Android and Desktop, but not in iOS - let's kill it if the app itself doesn't use it!

@lmarceau
Copy link
Contributor Author

Build is green here

@lmarceau lmarceau merged commit 4d48767 into mozilla-mobile:main Feb 23, 2023
@lmarceau lmarceau deleted the lm/FXIOS-5820-remove-swiftprotobuf branch February 23, 2023 18:03
fbertsch pushed a commit to fbertsch/firefox-ios that referenced this pull request Feb 24, 2023
@lmarceau @fbertsch
Remove FXIOS-5820 [v112] Swift protobuf dependency (mozilla-mobile#13331
8992874 
)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nbhasin2 nbhasin2 nbhasin2 approved these changes

@tarikeshaq tarikeshaq tarikeshaq approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lmarceau @nbhasin2 @mte-test @travis79 @lougeniaC64 @tarikeshaq