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] Enables CI for cronet_http_embedded #1070

Merged
merged 21 commits into from
Dec 21, 2023

Conversation

AlexV525
Copy link
Contributor

@AlexV525 AlexV525 commented Dec 5, 2023

Particially address #1047.


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

@github-actions github-actions bot added the type-infra A repository infrastructure change or enhancement label Dec 5, 2023
@AlexV525 AlexV525 changed the title Enables cronet_http's CI for cronet_http_embedded [cronet_http] Enables CI for cronet_http_embedded Dec 5, 2023
@AlexV525 AlexV525 force-pushed the feat/cronet-embed-pkg-tests branch from b32d661 to 91e5ba6 Compare December 7, 2023 05:51
@AlexV525 AlexV525 force-pushed the feat/cronet-embed-pkg-tests branch from bcf7b89 to f868580 Compare December 7, 2023 06:20
@AlexV525 AlexV525 marked this pull request as ready for review December 7, 2023 08:51
@AlexV525
Copy link
Contributor Author

AlexV525 commented Dec 7, 2023

@brianquinlan

Copy link
Collaborator

@brianquinlan brianquinlan left a comment

Choose a reason for hiding this comment

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

This is technically excellent work but I'm stuff not sure that cronet_http_embedded belongs in the same repo as cronet_http.

Let me talk to my team about it.

.github/workflows/cronet.yml Outdated Show resolved Hide resolved
@@ -2,14 +2,14 @@ group 'io.flutter.plugins.cronet_http'
version '1.0-SNAPSHOT'

buildscript {
ext.kotlin_version = '1.6.10'
ext.kotlin_version = '1.7.21'
Copy link
Collaborator

Choose a reason for hiding this comment

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

All of these types of upgrades should be done in a separate PR, no?

Copy link
Contributor Author

@AlexV525 AlexV525 Dec 12, 2023

Choose a reason for hiding this comment

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

I'm planning to reorg workflows and examples for the repo in further separate PRs. These are necessary for the current setup. 1.6.10 is already outdated for the latest Flutter.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, feel free to set these to any reasonable values.

pkgs/cronet_http/example/android/app/build.gradle Outdated Show resolved Hide resolved
@@ -19,48 +21,45 @@ env:
PUB_ENVIRONMENT: bot.github

jobs:
analyze:
name: Lint and static analysis
runs-on: ubuntu-latest
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you change this to macos? I think that there is more linux machine availability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are unknown timeouts when running on the Ubuntu runner. https://github.com/dart-lang/http/actions/runs/7270023551/job/19808487300?pr=1070

.github/workflows/cronet.yml Outdated Show resolved Hide resolved
@@ -2,14 +2,14 @@ group 'io.flutter.plugins.cronet_http'
version '1.0-SNAPSHOT'

buildscript {
ext.kotlin_version = '1.6.10'
ext.kotlin_version = '1.7.21'
Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, feel free to set these to any reasonable values.

pkgs/cronet_http/example/android/app/build.gradle Outdated Show resolved Hide resolved
@brianquinlan
Copy link
Collaborator

If I add these transforms to pkgs/cronet_http/tool/prepare_for_embedded.dart then the tests pass.

void updateImports() async {
  for (var file
      in _packageDirectory.listSync(recursive: true, followLinks: false)) {
    if (file.path.endsWith('.dart')) {
      final sourceFile = file as File;
      final updatedSource = sourceFile.readAsStringSync().replaceAll(
          'package:cronet_http/cronet_http.dart',
          'package:cronet_http_embedded/cronet_http_embedded.dart');
      sourceFile.writeAsStringSync(updatedSource);
    }
  }
}

void updateEntryPoint() async {
  File('${_packageDirectory.path}/lib/cronet_http.dart')
      .renameSync('${_packageDirectory.path}/lib/cronet_http_embedded.dart');
}

But the example application fails when making HTTP requests with:

E/flutter ( 7187): [ERROR:flutter/runtime/dart_vm_initializer.cc(41)] Unhandled Exception: ClientException: Cronet exception: org.chromium.net.impl.NetworkExceptionImpl: Exception in CronetUrlRequest: net::ERR_CERT_DATE_INVALID, ErrorCode=11, InternalErrorCode=-201, Retryable=false, uri=https://www.googleapis.com/books/v1/volumes?q=tes&maxResults=40&printType=books

@AlexV525
Copy link
Contributor Author

But the example application fails when making HTTP requests with:

E/flutter ( 7187): [ERROR:flutter/runtime/dart_vm_initializer.cc(41)] Unhandled Exception: ClientException: Cronet exception: org.chromium.net.impl.NetworkExceptionImpl: Exception in CronetUrlRequest: net::ERR_CERT_DATE_INVALID, ErrorCode=11, InternalErrorCode=-201, Retryable=false, uri=https://www.googleapis.com/books/v1/volumes?q=tes&maxResults=40&printType=books

That doesn't seem to be a library issue. I tried flutter.cn and it works fine. Sorry I cannot access googleapis from my location and the library doesn't seem to support a proxy.

@AlexV525
Copy link
Contributor Author

AlexV525 commented Dec 21, 2023

If I add these transforms to pkgs/cronet_http/tool/prepare_for_embedded.dart then the tests pass.

But the changes are great. I'm going to add it to the current script.

@@ -6,10 +6,12 @@ on:
- main
- master
paths:
- '.github/workflows/**'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe only trigger on changes to this file? So updating the monorepo spec doesn't trigger a 20 minute presubmit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sure. Will update these in another PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I already updated this.

arch: x86_64
profile: pixel
script: cd ./pkgs/cronet_http/example && flutter test --timeout=1200s integration_test/
script: cd 'pkgs/${{ matrix.package }}/example' && flutter test --timeout=1500s integration_test/
Copy link
Collaborator

Choose a reason for hiding this comment

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

I must misunderstand how this works - how can the tests work if the package imports inside the tests are not updated?

I mean, isn't the package test code still doing:

import 'package:cronet_http/cronet_http.dart';

But the tests pass so I'm confused ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't change in the latest commit. I guess there are some other problems with the Ubuntu setup.

@brianquinlan
Copy link
Collaborator

Hey @AlexV525,

Looks great! My intent is to merge this in the next couple of hours and then do a package:cronet_http_embedded release.

After that I'll increment the version number to 1.0 and do a package:cronet_http release but it will be functionally identical to the last package:cronet_http_embedded release.

We can work on switching publishers for package:cronet_http_embedded in the New Year. A lot of my colleagues are on vacation so I can't do it now.

Thanks so much for all your work on this, and on Dart HTTP in general!

@brianquinlan brianquinlan merged commit b10f448 into dart-lang:master Dec 21, 2023
23 checks passed
@AlexV525 AlexV525 deleted the feat/cronet-embed-pkg-tests branch December 22, 2023 02:02
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Feb 15, 2024
Revisions updated by `dart tools/rev_sdk_deps.dart`.

http (https://github.com/dart-lang/http/compare/f0a02f9..d8b237d):
  d8b237d  2024-02-15  Kevin Moore  [http] Migrate to the latest pkg:web, require Dart 3.3 (dart-lang/http#1132)
  5179d1c  2024-02-01  dependabot[bot]  Bump actions/cache from 3.3.2 to 4.0.0 (dart-lang/http#1125)
  0b803e8  2024-02-01  dependabot[bot]  Bump dart-lang/setup-dart from 1.6.0 to 1.6.2 (dart-lang/http#1124)
  82e0424  2024-01-29  Brian Quinlan  Use preferred flutter version constraints (dart-lang/http#1119)
  ccefa7c  2024-01-17  Brian Quinlan  Support `BaseResponseWithUrl` in `package:cupertino_http` and `package:cronet_http` (dart-lang/http#1110)
  e7a8e25  2024-01-16  Brian Quinlan  Add `BaseResponseWithUrl.url` (dart-lang/http#1109)
  c8f17a6  2024-01-12  Brian Quinlan  Add a contributing guide (dart-lang/http#1115)
  ebd86b9  2024-01-12  Brian Quinlan  Add the ability to get response headers as a `Map<String, List<String>>` (dart-lang/http#1114)
  5c75da6  2024-01-12  Brian Quinlan  Add tests for sending "cookie" and receiving "set-cookie" headers (dart-lang/http#1113)
  c2a6d64  2024-01-12  Alex Li  [cronet_http] ⬇️ Downgrade `minSdkVersion` to 21 (dart-lang/http#1104)
  661f5d6  2024-01-08  Brian Quinlan  Use `package:http_image_provider` in all `Client` implementation examples (dart-lang/http#1089)
  473a892  2024-01-04  Brian Quinlan  Remove the "play-services-cronet" dependency in the example app when building `package:cronet_http_embedded` (dart-lang/http#1103)
  e79ebe1  2024-01-03  Moritz  Fix `labeler.yml` (dart-lang/http#1099)
  73b0b1c  2024-01-01  dependabot[bot]  Bump actions/labeler from 4.3.0 to 5.0.0 (dart-lang/http#1096)
  15ec3ba  2023-12-21  Brian Quinlan  Prepare to publish `package:cronet_http` as 1.0.0 (dart-lang/http#1087)
  26e55c3  2023-12-21  Brian Quinlan  cronet_http: require android API level 28 (dart-lang/http#1088)
  b10f448  2023-12-22  Alex Li  [cronet_http] Enables CI for `cronet_http_embedded` (dart-lang/http#1070)
  a5b8eec  2023-12-18  Brian Quinlan  Prepare to publish cupertino 1.2.0 (dart-lang/http#1080)
  c114aa0  2023-12-15  Brian Quinlan  Add a fake response for PNG images (dart-lang/http#1081)
  db2cb76  2023-12-14  Kevin Moore  Run web tests with wasm with dev Dart sdk (dart-lang/http#1078)
  36f98e9  2023-12-12  Brian Quinlan  Fix a bug where BrowserClient was listed as requiring Flutter (dart-lang/http#1077)
  db7f165  2023-12-07  Brian Quinlan  Provide an example of configuring IOClient with an HttpClient. (dart-lang/http#1074)
  cd748b6  2023-12-04  Brian Quinlan  Document that runWithClient must be called for every isolate (dart-lang/http#1069)
  f585947  2023-12-04  Brian Quinlan  Test persistentConnection with large request bodies (dart-lang/http#984)
  7c05dde  2023-12-04  Brian Quinlan  Add documentation for "no_default_http_client" (dart-lang/http#1068)
  d8983fa  2023-12-04  Brian Quinlan  Add support for setting headers for all requests (dart-lang/http#1060)
  c90496e  2023-12-04  Brian Quinlan  Document how to use replacement `Client` implementations (dart-lang/http#1063)
  c8536e4  2023-12-01  Kevin Moore  [http_client_conformance_tests] Updates to support wasm compilation (dart-lang/http#1064)
  5dd5140  2023-12-01  dependabot[bot]  Bump actions/setup-java from 3 to 4 (dart-lang/http#1065)
  064f510  2023-11-30  Devon Carew  misc cleanup of yaml files (dart-lang/http#1061)
  22f52e2  2023-11-30  Brian Quinlan  Update pubspec.yaml to 0.4.2 (dart-lang/http#1059)
  40a46d8  2023-11-29  Brian Quinlan  Fix a bug where cronet_http sends incorrect HTTP request methods (dart-lang/http#1058)
  c125ed5  2023-11-27  Kevin Moore  [http] Allow pkg:web v0.3.0 (dart-lang/http#1055)
  9fb4cfa  2023-11-22  Kevin Moore  Update lints to latest, etc (dart-lang/http#1048)
  5e84d9f  2023-11-21  Kevin Moore  Update platform-specific imports for wasm (dart-lang/http#1051)
  8c9feb5  2023-11-21  Kevin Moore  [http] Fix type cast for dart2wasm (dart-lang/http#1050)
  a2f0b25  2023-11-21  Kevin Moore  [http] use pkg:web, require Dart 3.2 (dart-lang/http#1049)

markdown (https://github.com/dart-lang/markdown/compare/c2b8429..6efe141):
  6efe141  2024-02-14  Kevin Moore  Migrate example to pkg:web, update minimum required Dart version (dart-lang/markdown#582)

web_socket_channel (https://github.com/dart-lang/web_socket_channel/compare/5241175..3db86bc):
  3db86bc  2024-02-15  Kevin Moore  Require Dart 3.3 and the latest pkg:web (dart-lang/web_socket_channel#326)
  1c4a923  2024-02-01  dependabot[bot]  Bump dart-lang/setup-dart from 1.6.0 to 1.6.2 (dart-lang/web_socket_channel#323)
  041aa3c  2024-01-08  Devon Carew  adjust the HtmlWebSocketChannel ctor parameter type; rev to 2.4.3 (dart-lang/web_socket_channel#320)
  0e8bedc  2024-01-04  Tyler Dunn  Allow pkg:web v0.3.0 (dart-lang/web_socket_channel#306)
  62370cc  2024-01-01  dependabot[bot]  Bump actions/stale from 8.0.0 to 9.0.0 (dart-lang/web_socket_channel#312)
  2a0563f  2023-12-18  Kevin Moore  Prepare release v2.4.1 (dart-lang/web_socket_channel#301)
  906c944  2023-12-14  Kevin Moore  CI: test dev SDK with dart2wasm (dart-lang/web_socket_channel#304)
  a316c53  2023-12-13  Kevin Moore  Rename helper extensions to not collide with pkg:web unreleased (dart-lang/web_socket_channel#303)
  547184a  2023-12-11  Kevin Moore  blast_repo fixes (dart-lang/web_socket_channel#302)
  969bc6c  2023-12-09  Ömer Sinan Ağacan  Fix JS value to Dart conversion when receiving from a web socket (dart-lang/web_socket_channel#298)
  df096a9  2023-11-30  Ömer Sinan Ağacan  Remove removed lints (dart-lang/web_socket_channel#299)
  67bf9a3  2023-11-23  Nate Bosch  Drop some use of ! (dart-lang/web_socket_channel#296)
  d4a8d70  2023-11-22  Kevin Moore  Small tweak (dart-lang/web_socket_channel#295)
  9e80b8d  2023-11-22  Kevin Moore  migrate to pkg web (dart-lang/web_socket_channel#294)

Change-Id: I87c4baa07efc5fc2bb283c7b32e69ad69c5a02aa
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/352960
Reviewed-by: Kevin Moore <[email protected]>
Commit-Queue: Devon Carew <[email protected]>
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Feb 16, 2024
This reverts commit 8d703d1.

Reason for revert: HHH bot is broken

Original change's description:
> [deps] rev http, markdown, web_socket_channel
>
> Revisions updated by `dart tools/rev_sdk_deps.dart`.
>
> http (https://github.com/dart-lang/http/compare/f0a02f9..d8b237d):
>   d8b237d  2024-02-15  Kevin Moore  [http] Migrate to the latest pkg:web, require Dart 3.3 (dart-lang/http#1132)
>   5179d1c  2024-02-01  dependabot[bot]  Bump actions/cache from 3.3.2 to 4.0.0 (dart-lang/http#1125)
>   0b803e8  2024-02-01  dependabot[bot]  Bump dart-lang/setup-dart from 1.6.0 to 1.6.2 (dart-lang/http#1124)
>   82e0424  2024-01-29  Brian Quinlan  Use preferred flutter version constraints (dart-lang/http#1119)
>   ccefa7c  2024-01-17  Brian Quinlan  Support `BaseResponseWithUrl` in `package:cupertino_http` and `package:cronet_http` (dart-lang/http#1110)
>   e7a8e25  2024-01-16  Brian Quinlan  Add `BaseResponseWithUrl.url` (dart-lang/http#1109)
>   c8f17a6  2024-01-12  Brian Quinlan  Add a contributing guide (dart-lang/http#1115)
>   ebd86b9  2024-01-12  Brian Quinlan  Add the ability to get response headers as a `Map<String, List<String>>` (dart-lang/http#1114)
>   5c75da6  2024-01-12  Brian Quinlan  Add tests for sending "cookie" and receiving "set-cookie" headers (dart-lang/http#1113)
>   c2a6d64  2024-01-12  Alex Li  [cronet_http] ⬇️ Downgrade `minSdkVersion` to 21 (dart-lang/http#1104)
>   661f5d6  2024-01-08  Brian Quinlan  Use `package:http_image_provider` in all `Client` implementation examples (dart-lang/http#1089)
>   473a892  2024-01-04  Brian Quinlan  Remove the "play-services-cronet" dependency in the example app when building `package:cronet_http_embedded` (dart-lang/http#1103)
>   e79ebe1  2024-01-03  Moritz  Fix `labeler.yml` (dart-lang/http#1099)
>   73b0b1c  2024-01-01  dependabot[bot]  Bump actions/labeler from 4.3.0 to 5.0.0 (dart-lang/http#1096)
>   15ec3ba  2023-12-21  Brian Quinlan  Prepare to publish `package:cronet_http` as 1.0.0 (dart-lang/http#1087)
>   26e55c3  2023-12-21  Brian Quinlan  cronet_http: require android API level 28 (dart-lang/http#1088)
>   b10f448  2023-12-22  Alex Li  [cronet_http] Enables CI for `cronet_http_embedded` (dart-lang/http#1070)
>   a5b8eec  2023-12-18  Brian Quinlan  Prepare to publish cupertino 1.2.0 (dart-lang/http#1080)
>   c114aa0  2023-12-15  Brian Quinlan  Add a fake response for PNG images (dart-lang/http#1081)
>   db2cb76  2023-12-14  Kevin Moore  Run web tests with wasm with dev Dart sdk (dart-lang/http#1078)
>   36f98e9  2023-12-12  Brian Quinlan  Fix a bug where BrowserClient was listed as requiring Flutter (dart-lang/http#1077)
>   db7f165  2023-12-07  Brian Quinlan  Provide an example of configuring IOClient with an HttpClient. (dart-lang/http#1074)
>   cd748b6  2023-12-04  Brian Quinlan  Document that runWithClient must be called for every isolate (dart-lang/http#1069)
>   f585947  2023-12-04  Brian Quinlan  Test persistentConnection with large request bodies (dart-lang/http#984)
>   7c05dde  2023-12-04  Brian Quinlan  Add documentation for "no_default_http_client" (dart-lang/http#1068)
>   d8983fa  2023-12-04  Brian Quinlan  Add support for setting headers for all requests (dart-lang/http#1060)
>   c90496e  2023-12-04  Brian Quinlan  Document how to use replacement `Client` implementations (dart-lang/http#1063)
>   c8536e4  2023-12-01  Kevin Moore  [http_client_conformance_tests] Updates to support wasm compilation (dart-lang/http#1064)
>   5dd5140  2023-12-01  dependabot[bot]  Bump actions/setup-java from 3 to 4 (dart-lang/http#1065)
>   064f510  2023-11-30  Devon Carew  misc cleanup of yaml files (dart-lang/http#1061)
>   22f52e2  2023-11-30  Brian Quinlan  Update pubspec.yaml to 0.4.2 (dart-lang/http#1059)
>   40a46d8  2023-11-29  Brian Quinlan  Fix a bug where cronet_http sends incorrect HTTP request methods (dart-lang/http#1058)
>   c125ed5  2023-11-27  Kevin Moore  [http] Allow pkg:web v0.3.0 (dart-lang/http#1055)
>   9fb4cfa  2023-11-22  Kevin Moore  Update lints to latest, etc (dart-lang/http#1048)
>   5e84d9f  2023-11-21  Kevin Moore  Update platform-specific imports for wasm (dart-lang/http#1051)
>   8c9feb5  2023-11-21  Kevin Moore  [http] Fix type cast for dart2wasm (dart-lang/http#1050)
>   a2f0b25  2023-11-21  Kevin Moore  [http] use pkg:web, require Dart 3.2 (dart-lang/http#1049)
>
> markdown (https://github.com/dart-lang/markdown/compare/c2b8429..6efe141):
>   6efe141  2024-02-14  Kevin Moore  Migrate example to pkg:web, update minimum required Dart version (dart-lang/markdown#582)
>
> web_socket_channel (https://github.com/dart-lang/web_socket_channel/compare/5241175..3db86bc):
>   3db86bc  2024-02-15  Kevin Moore  Require Dart 3.3 and the latest pkg:web (dart-lang/web_socket_channel#326)
>   1c4a923  2024-02-01  dependabot[bot]  Bump dart-lang/setup-dart from 1.6.0 to 1.6.2 (dart-lang/web_socket_channel#323)
>   041aa3c  2024-01-08  Devon Carew  adjust the HtmlWebSocketChannel ctor parameter type; rev to 2.4.3 (dart-lang/web_socket_channel#320)
>   0e8bedc  2024-01-04  Tyler Dunn  Allow pkg:web v0.3.0 (dart-lang/web_socket_channel#306)
>   62370cc  2024-01-01  dependabot[bot]  Bump actions/stale from 8.0.0 to 9.0.0 (dart-lang/web_socket_channel#312)
>   2a0563f  2023-12-18  Kevin Moore  Prepare release v2.4.1 (dart-lang/web_socket_channel#301)
>   906c944  2023-12-14  Kevin Moore  CI: test dev SDK with dart2wasm (dart-lang/web_socket_channel#304)
>   a316c53  2023-12-13  Kevin Moore  Rename helper extensions to not collide with pkg:web unreleased (dart-lang/web_socket_channel#303)
>   547184a  2023-12-11  Kevin Moore  blast_repo fixes (dart-lang/web_socket_channel#302)
>   969bc6c  2023-12-09  Ömer Sinan Ağacan  Fix JS value to Dart conversion when receiving from a web socket (dart-lang/web_socket_channel#298)
>   df096a9  2023-11-30  Ömer Sinan Ağacan  Remove removed lints (dart-lang/web_socket_channel#299)
>   67bf9a3  2023-11-23  Nate Bosch  Drop some use of ! (dart-lang/web_socket_channel#296)
>   d4a8d70  2023-11-22  Kevin Moore  Small tweak (dart-lang/web_socket_channel#295)
>   9e80b8d  2023-11-22  Kevin Moore  migrate to pkg web (dart-lang/web_socket_channel#294)
>
> Change-Id: I87c4baa07efc5fc2bb283c7b32e69ad69c5a02aa
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/352960
> Reviewed-by: Kevin Moore <[email protected]>
> Commit-Queue: Devon Carew <[email protected]>

Change-Id: I8723a343908227e39604cba7827c0f99912a10f4
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/352967
Reviewed-by: Siva Annamalai <[email protected]>
Bot-Commit: Rubber Stamper <[email protected]>
Commit-Queue: Siva Annamalai <[email protected]>
Auto-Submit: Devon Carew <[email protected]>
Reviewed-by: Devon Carew <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:cronet_http type-infra A repository infrastructure change or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants