Skip to content

🚀 Use org.chromium.net:cronet-embedded instead of com.google.android.gms:play-services-cronet #842

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

Closed
wants to merge 3 commits into from

Conversation

AlexV525
Copy link
Contributor

Resolves #807.

We can discuss the intention to use or not to use GMS-based dependencies further.

Cronet represent org.chromium.net instead of as part of Google Play Services. So it should be using the org.chromium.net, the implementation code in the package remains valid after the change.

@AlexV525
Copy link
Contributor Author

AlexV525 commented Dec 27, 2022

@brianquinlan @natebosch

@AlexV525 AlexV525 force-pushed the cronet-android-no-gms branch from 48d90b0 to ed2b4d3 Compare December 28, 2022 13:35
@brianquinlan
Copy link
Collaborator

Wouldn't this change increase the packaged size of applications significantly? A note on the exoplayer site suggests that the size increase would be ~8MB.

@brianquinlan brianquinlan self-requested a review January 4, 2023 21:35
@AlexV525
Copy link
Contributor Author

AlexV525 commented Jan 4, 2023

Wouldn't this change increase the packaged size of applications significantly? A note on the exoplayer site suggests that the size increase would be ~8MB.

Yes, but with phones that's unable to use GMS, the GMS version won't get any available instances of Cronet.

@brianquinlan
Copy link
Collaborator

As we discussed in the bug (#807), I think that we should have two separate packages here and the developer can decide whether they want to have a GPS dependency or not.

@AlexV525
Copy link
Contributor Author

AlexV525 commented Jan 5, 2023

As we discussed in the bug (#807), I think that we should have two separate packages here and the developer can decide whether they want to have a GPS dependency or not.

So as I said in #807 (comment), a CI that writes different dependency and publish another package would be okay too. WDYT?

@brianquinlan
Copy link
Collaborator

In principle I like the idea! I have two concerns:

  1. Whatever CI we use should be compatible with the rest of dart-lang/http (I think that there was some talk of doing automatic publishing - @devoncarew might know more)
  2. That the implementation not be too hacky - were you planning on rewriting build.gradle and pubspec.yaml in the CI script?

@AlexV525
Copy link
Contributor Author

AlexV525 commented Jan 6, 2023

  1. Whatever CI we use should be compatible with the rest of dart-lang/http

It's easy to publish packages within a monorepo or nested repo, just need to use matrix when running jobs. See https://github.com/cfug/diox/blob/main/.github/workflows/publish.yml

  1. That the implementation not be too hacky - were you planning on rewriting build.gradle and pubspec.yaml in the CI script?

Yes, that's the minimum requirement, we can run a Dart (or Shell) script during the job which won't require any manual tweak.

@natebosch
Copy link
Member

I think we could symlink the same lib directory between two packages, and otherwise treat them like they are both independent packages. I don't have strong opinions between that and synthesizing the package during ah automated publish - but given that we don't currently have an automated publish at all, a package with symlinked content might be the best fit.

@AlexV525
Copy link
Contributor Author

I think we could symlink the same lib directory between two packages, and otherwise treat them like they are both independent packages.

You'll have to deal with publishing scripts eventually because pub doesn't support symlink publishing.

@natebosch
Copy link
Member

because pub doesn't support symlink publishing

It doesn't? I thought the new implementation supported them.

dart-lang/pub#2827 (comment)

If it doesn't, we can file an issue and see how hard it would be to make them work. I can't imagine it should be too hard.

@AlexV525
Copy link
Contributor Author

If it doesn't, we can file an issue and see how hard it would be to make them work. I can't imagine it should be too hard.

dart-lang/pub#3143

@AlexV525
Copy link
Contributor Author

I think we could symlink the same lib directory between two packages, and otherwise treat them like they are both independent packages.

And also, this will increase the complexities when maintaining two packages. Considering the current cronet adds other implementations without the symlinked, they will have to maintain with more commits.

@AlexV525
Copy link
Contributor Author

Close in favor of #853.

@AlexV525 AlexV525 closed this Jan 18, 2023
@AlexV525 AlexV525 deleted the cronet-android-no-gms branch November 24, 2023 15:59
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.

cronet_http: depend on org.chromium.net:cronet-api instead
3 participants