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

[cronet_http] ✨ Add Cronet embedded tool #853

Merged
merged 9 commits into from
Feb 17, 2023

Conversation

AlexV525
Copy link
Contributor

Resolves #807.

Creates a new tool for publishing the non-GMS-based Cronet package. The tool is implemented with these concerns:

  • Not creating symlinks because the structure can be changed easily.
  • Not to restrict running on a single platform, so it's been written in Dart.
  • Only modify files, but do not publish in this tool, leave the process to the operator, since the latest stable version can have breaking changes and require further CI tests (more jobs in .github/workflows).
  • As easy as possible to maintain the tool.

@AlexV525
Copy link
Contributor Author

@brianquinlan @natebosch

pkgs/cronet_http/tool/prepare_for_embedded.dart Outdated Show resolved Hide resolved
pkgs/cronet_http/tool/prepare_for_embedded.dart Outdated Show resolved Hide resolved
pkgs/cronet_http/tool/prepare_for_embedded.dart Outdated Show resolved Hide resolved
@brianquinlan
Copy link
Collaborator

@natebosch Do you know how the upcoming auto-publish feature works? Will it be possible to trigger a script like this as part of the publishing process?

@AlexV525
Copy link
Contributor Author

FYI, previously when we do automatically publish on GitHub, it's easy to run any scripts in jobs. e.g. https://github.com/fluttercandies/flutter_photo_manager/blob/361773c98c4f9665ee61ef25f1a6aa003d7692ec/.github/workflows/publish.yml#L14

@natebosch
Copy link
Member

@devoncarew is setting up autopublish on some repos. I don't think we've set up any mono repos yet - so we may need some work in the mono_repo tool.

@devoncarew
Copy link
Member

From my understanding, you want to run a custom dart script before publishing? It'll perform a native build or provision native artifacts?

The new CI publishing feature has a few different ways it can operate. The way I've used it is configure the package on pub.dev to be published from a github action. Then, when a github action is triggered by a tag on a commit, running dart pub publish --force will recognize that it's running in a github action, has an 'OIDC' token, and pass that as authentication to pub.dev when publishing.

The tooling I've written for publishing automation - that does validation for PRs and publishing for tagged commits on main - is likely too purpose built to re-use here (it does however support mono-repos :) ).

If you think you want to use some publishing automation here, you could:

@AlexV525
Copy link
Contributor Author

Okay according to the above discussions and advises from @devoncarew, we can make an auto-publish workflow, and also allow it to be done manually using the simple Dart tool. I'll go for this goal then, please vote 👍 if you guys think it's valid.

@brianquinlan
Copy link
Collaborator

@AlexV525 Your solution will work but it feels a bit hacky to me.

Let's take a step make and think about options:

  1. Fork this repo, make the needed changes, and periodically sync the fork:
    • The sync can be automated
    • There would be no coordination in releases i.e. a release of package:cronet_http would not automatically release package:cronet_http_embedded
    • The fork would include unnecessary packages like cupertino_http
  2. Create a script that modifies this repository in-place (this approach):
    • Error prone if used done manually (but reasonable if done through automation)
    • Will double CI time if also runs with the script-modified changes
  3. Create a branch, make the needed changes, and periodically sync from main
    • Similar to (1) except the Dart team would have to manage it

Is there some other approach?

@AlexV525
Copy link
Contributor Author

The main guideline I based on when I was developing ideas is Avoid manipulation by humans as possible, let the automation rolls. The guideline will avoid exceptions made by human mistakes, such as forgetting to apply patches, forgetting to sync the code base, merging leads to code loss, etc.

I'll base on this guideline to discuss the above options.

Fork this repo, make the needed changes, and periodically sync the fork

  • The sync can be automated
  • There would be no coordination in releases i.e. a release of package:cronet_http would not automatically release package:cronet_http_embedded

Not only the sync can be done automatically, as we discussed, but the publishing can also do it as well. The coordination can be bridged using GitHub Actions too, but it'll involve crossing repo calls, which seems too much for the proposal.

  • The fork would include unnecessary packages like cupertino_http

That is correct, but the impact is not as much as we thought.

Create a script that modifies this repository in-place (this approach)

  • Error prone if used done manually (but reasonable if done through automation)

Given the complexity and the corresponding change made by the tool, I'll say it'll barely meet exceptions during regular development. The idea is to get rid of manual publishing, so if we think too much about doing this manually, we'll come to another path eventually.

  • Will double CI time if also runs with the script-modified changes

The time cost can be replaced with space cost (running two jobs parallel). And if new exceptions were thrown during the publishing of the embedded package, it can be easily solved during PRs. It actually saves time for debugging incompatible changes.

Create a branch, make the needed changes, and periodically sync from main

  • Similar to (1) except the Dart team would have to manage it

Any manual manipulation should be avoided when doing patches or publishing, which will make the package solid.

Other approaches?

I don't come up with any at present.

@AlexV525
Copy link
Contributor Author

AlexV525 commented Feb 1, 2023

Frankly, the difference between the GMS-based and embedded is tiny in the code base, and the community as well as the Dart team has already explored automatic publishing. We should not have many issues with maintaining a publishing tool.

@AlexV525
Copy link
Contributor Author

Hi @brianquinlan, did you come up with more thoughts or considerations in these weeks?

@brianquinlan
Copy link
Collaborator

brianquinlan commented Feb 14, 2023

Hey @AlexV525 ,

I'm sorry for the long delay. I'm patching in the code now to take a look.

@brianquinlan brianquinlan self-requested a review February 14, 2023 01:50
@brianquinlan
Copy link
Collaborator

brianquinlan commented Feb 14, 2023

Hey @AlexV525,

I think that you need to make at least three changes:

  1. you need to modify the name and description in pubspec.yaml. The name should probably be something like cronet_http_embedded and the description could be `An Android Flutter plugin that provides access to the Cronet HTTP client. Identical to package:cronet_http except that it embeds Cronet rather than relying on Google Play Services.
  2. we probably need a new readme (maybe README_EMBEDDED.md) that explains that the difference between it and package:cronet_http is and then links to package:cronet_http (so we don't need to keep the documentation in sync).
  3. Your prepare_for_embedded.dart script should rename README_EMBEDDED.md to README.md

@@ -16,6 +16,7 @@ dependencies:
dev_dependencies:
lints: ^1.0.0
pigeon: ^3.2.3
xml: ^6.1.0
Copy link
Member

Choose a reason for hiding this comment

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

I got freaked out. It's only a dev_dependency. 😌

@AlexV525
Copy link
Contributor Author

@brianquinlan Updated accroding to your suggestions.

@AlexV525
Copy link
Contributor Author

Also please let me know if we can setup a GitHub action for this, maybe in separate PRs.

@brianquinlan
Copy link
Collaborator

I have a few code nits but overall it looks good!

I wouldn't worry about having a github action for now. I think that we are going to roll out automatic publishing soon and we can use this script when that rolls out. For now I'll just use it manually when I publish package:cronet_http.

pkgs/cronet_http/README_EMBEDDED.md Outdated Show resolved Hide resolved
pkgs/cronet_http/README_EMBEDDED.md Outdated Show resolved Hide resolved
pkgs/cronet_http/tool/prepare_for_embedded.dart Outdated Show resolved Hide resolved
pkgs/cronet_http/tool/prepare_for_embedded.dart Outdated Show resolved Hide resolved
pkgs/cronet_http/tool/prepare_for_embedded.dart Outdated Show resolved Hide resolved
pkgs/cronet_http/tool/run_pigeon.sh Show resolved Hide resolved
pkgs/cronet_http/README_EMBEDDED.md Outdated Show resolved Hide resolved
pkgs/cronet_http/tool/prepare_for_embedded.dart Outdated Show resolved Hide resolved
pkgs/cronet_http/tool/prepare_for_embedded.dart Outdated Show resolved Hide resolved
pkgs/cronet_http/tool/prepare_for_embedded.dart Outdated Show resolved Hide resolved
pkgs/cronet_http/tool/prepare_for_embedded.dart Outdated Show resolved Hide resolved
pkgs/cronet_http/tool/run_pigeon.sh Show resolved Hide resolved
@brianquinlan
Copy link
Collaborator

Hey @AlexV525,

Thanks for your contribution and your patience!

I will try to do a release tomorrow.

@brianquinlan brianquinlan merged commit c13a3f8 into dart-lang:master Feb 17, 2023
@AlexV525 AlexV525 deleted the add-cronet-embedded-tool branch February 17, 2023 02:11
@AlexV525
Copy link
Contributor Author

I will try to do a release tomorrow.

Is it working as expected?

@brianquinlan
Copy link
Collaborator

@AlexV525 The release is https://pub.dev/packages/cronet_http_embedded

You could check to see if it works as you expected ;-)

@DemoJameson
Copy link

build failed with cronet_http_embedded on android, since implementation com.google.android.gms:play-services-cronet:18.0.1 lack of double quotes

FAILURE: Build failed with an exception.

* Where:
Build file 'C:\Users\DemoJameson\AppData\Local\Pub\Cache\hosted\pub.flutter-io.cn\cronet_http_embedded-0.2.0\android\build.gradle' line: 49

* What went wrong:
Could not compile build file 'C:\Users\DemoJameson\AppData\Local\Pub\Cache\hosted\pub.flutter-io.cn\cronet_http_embedded-0.2.0\android\build.gradle'.
> startup failed:
  build file 'C:\Users\DemoJameson\AppData\Local\Pub\Cache\hosted\pub.flutter-io.cn\cronet_http_embedded-0.2.0\android\build.gradle': 49: Unexpected input: '{' @ line 49, column 14.
     dependencies {
                  ^

  1 error

@brianquinlan
Copy link
Collaborator

I just released version 0.2.1 - could you check to see if that fixes the problem?

@AlexV525 AlexV525 changed the title ✨ Add Cronet embedded tool [cronet_http] ✨ Add Cronet embedded tool Dec 10, 2023
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
6 participants