-
Notifications
You must be signed in to change notification settings - Fork 383
[cronet_http] ✨ Add Cronet embedded tool #853
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
Conversation
@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? |
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 |
@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 |
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 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:
|
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. |
@AlexV525 Your solution will work but it feels a bit hacky to me. Let's take a step make and think about options:
Is there some other approach? |
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
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.
That is correct, but the impact is not as much as we thought. Create a script that modifies this repository in-place (this approach)
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.
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
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. |
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. |
Hi @brianquinlan, did you come up with more thoughts or considerations in these weeks? |
Hey @AlexV525 , I'm sorry for the long delay. I'm patching in the code now to take a look. |
Hey @AlexV525, I think that you need to make at least three changes:
|
@@ -16,6 +16,7 @@ dependencies: | |||
dev_dependencies: | |||
lints: ^1.0.0 | |||
pigeon: ^3.2.3 | |||
xml: ^6.1.0 |
There was a problem hiding this comment.
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. 😌
@brianquinlan Updated accroding to your suggestions. |
Also please let me know if we can setup a GitHub action for this, maybe in separate PRs. |
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. |
Hey @AlexV525, Thanks for your contribution and your patience! I will try to do a release tomorrow. |
Is it working as expected? |
@AlexV525 The release is https://pub.dev/packages/cronet_http_embedded You could check to see if it works as you expected ;-) |
build failed with cronet_http_embedded on android, since
|
I just released version 0.2.1 - could you check to see if that fixes the problem? |
Resolves #807.
Creates a new tool for publishing the non-GMS-based Cronet package. The tool is implemented with these concerns:
.github/workflows
).