Skip to content

[ci] Removes bespoke web scripts. #4129

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
Merged
merged 1 commit into from
Jun 5, 2023
Merged

[ci] Removes bespoke web scripts. #4129

merged 1 commit into from
Jun 5, 2023

Conversation

@ditman
Copy link
Member

@ditman ditman commented Jun 1, 2023

Removes the bespoke scripts (regen_mocks.sh, run_test.sh) that I had added to web packages.

I'll keep those in my own ~/bin moving forward, since I'm not sure anybody else actually used/needed them other than myself :)

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@ditman ditman requested a review from stuartmorgan-g as a code owner June 1, 2023 21:46
@ditman ditman mentioned this pull request Jun 1, 2023
Merged
11 tasks
@ditman
Copy link
Member Author

ditman commented Jun 2, 2023 •
edited
Loading

I think this PR can ignore the changelog and versioning checks, since all it's doing is removing non-standard scripts that trip those checks :)

@ditman ditman added override: no changelog needed Override the check requiring CHANGELOG updates for most changes override: no versioning needed Override the check requiring version bumps for most changes labels Jun 2, 2023
stuartmorgan-g
stuartmorgan-g approved these changes Jun 2, 2023
View reviewed changes
Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM

Out of curiosity, is there a reason you aren't just using the repo tooling's drive-examples --web? If it's just the ability to specify a specific test file, we could definitely add a flag for that.

@ditman
Copy link
Member Author

ditman commented Jun 2, 2023 •
edited
Loading

Out of curiosity, is there a reason you aren't just using the repo tooling's drive-examples --web? If it's just the ability to specify a specific test file, we could definitely add a flag for that.

@stuartmorgan I gave this some thought before removing custom scripts, and there's a couple of things that bothered me:

  • Having to run the script from the root of the checkout and specify a --packages (rather than from the directory where I'm actually working on and assuming the package is whatever directory I'm in).
  • The tool_runner.sh hardcodes --packages-for-branch and IIRC that prevents me from passing --packages=my_package because the script says it must be one or the other (IIRC).
  • Running a single file is useful some times, especially when introducing new test files to the package.
  • Detecting that chromedriver is not running early (or the driver for the browser that is going to be used) is faster than letting flutter drive fail.

Minor annoyances, really, and I'm not sure if they warrant a fix or they're a quirk of the way I'm used to working with the repo!

@ditman ditman force-pushed the move-bespoke-scripts branch from 92b6c80 to 68dfde1 Compare June 2, 2023 22:55
@stuartmorgan-g stuartmorgan-g mentioned this pull request Jun 5, 2023
Open
@stuartmorgan-g
Copy link
Collaborator

stuartmorgan-g commented Jun 5, 2023

  • Having to run the script from the root of the checkout

That's definitely annoying; I'd gotten so used to it that it hadn't occurred to me that we can easily fix it now: flutter/flutter#128231

and specify a --packages (rather than from the directory where I'm actually working on and assuming the package is whatever directory I'm in).

flutter/flutter#128232

  • The tool_runner.sh hardcodes --packages-for-branch and IIRC that prevents me from passing --packages=my_package because the script says it must be one or the other (IIRC).

I pared tool_runner.sh down to just CI stuff a while ago; you shouldn't use it locally. Instead, dart run the script directly.

  • Running a single file is useful some times, especially when introducing new test files to the package.

flutter/flutter#128234

  • Detecting that chromedriver is not running early (or the driver for the browser that is going to be used) is faster than letting flutter drive fail.

This one is probably best left out of the tool since adding it in a cross-platform way would be kind of a pain.

But if we fix the others then your wrapper script can just be that one check.

@ditman
Copy link
Member Author

ditman commented Jun 5, 2023

Thanks for creating the tracking issues!

But if we fix the others then your wrapper script can just be that one check.

I haven't tried the check in a while, it might be something that can be made part of dart drive too to make it fail fast!

@ditman ditman added autosubmit Merge PR when tree becomes green via auto submit App and removed autosubmit Merge PR when tree becomes green via auto submit App labels Jun 5, 2023
@ditman ditman force-pushed the move-bespoke-scripts branch from 68dfde1 to 7c48a7b Compare June 5, 2023 21:11
@ditman ditman added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 5, 2023
@auto-submit auto-submit bot merged commit 81e3428 into flutter:main Jun 5, 2023
@ditman ditman deleted the move-bespoke-scripts branch June 5, 2023 22:07
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 6, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 6, 2023
@engine-flutter-autoroll
81e3428 [ci] Removes bespoke web scripts. (flutter/packages#4129)
4d51611
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 6, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 6, 2023
@engine-flutter-autoroll
81e3428 [ci] Removes bespoke web scripts. (flutter/packages#4129)
3d3b852
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 6, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 6, 2023
@engine-flutter-autoroll
81e3428 [ci] Removes bespoke web scripts. (flutter/packages#4129)
1b0107d
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 6, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 6, 2023
@engine-flutter-autoroll
81e3428 [ci] Removes bespoke web scripts. (flutter/packages#4129)
faf493d
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jun 6, 2023
@engine-flutter-autoroll
Roll Packages from db4e5c2 to da72219 (10 revisions) (#128348)
7e87650 
flutter/packages@db4e5c2...da72219

2023-06-06 42692766+dimabran@users.noreply.github.com remove unnecessary typed_data imports (flutter/packages#4136)
2023-06-06 43054281+camsim99@users.noreply.github.com [local_auth_android] Bump androidx.fragment to 1.5.7 and androidx.core to 1.10.1 (flutter/packages#4142)
2023-06-05 ditman@gmail.com [google_maps] Endorses package:google_maps_flutter_web. (flutter/packages#4124)
2023-06-05 ditman@gmail.com [ci] Removes bespoke web scripts. (flutter/packages#4129)
2023-06-05 arteevraina@gmail.com [webview_flutter]: fix typo (flutter/packages#4070)
2023-06-05 49699333+dependabot[bot]@users.noreply.github.com [camera]: Bump com.google.guava:guava from 31.1-android to 32.0.0-android in /packages/camera/camera_android_camerax/android (flutter/packages#4116)
2023-06-05 stuartmorgan@google.com [tools] Fix OOB test error (flutter/packages#4144)
2023-06-05 46547604+dsambuk@users.noreply.github.com [path_provider] Allow win32 up to version 5.x (flutter/packages#4125)
2023-06-05 49699333+dependabot[bot]@users.noreply.github.com [sign_in]: Bump com.google.guava:guava from 31.1-android to 32.0.0-android in /packages/google_sign_in/google_sign_in_android/android (flutter/packages#4112)
2023-06-05 49699333+dependabot[bot]@users.noreply.github.com [video_player]: Bump exoplayer_version from 2.18.6 to 2.18.7 in /packages/video_player/video_player_android/android (flutter/packages#4053)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC flutter-ecosystem@google.com,rmistry@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@stuartmorgan-g stuartmorgan-g mentioned this pull request Jun 12, 2023
Merged
11 tasks
auto-submit bot pushed a commit that referenced this pull request Jun 13, 2023
@stuartmorgan-g
[tool] Allow running from anywhere (#4199)
eed9b68 
Now that the repo tooling is always run from source, not via `pub global`, we no longer need to infer the repo location from the current directory. Instead, hard-code knowledge of where the repository root is. This makes it much easier to run the tooling, since it's common to be in a package directory rather than the repo root.

To make it even easier to run from within a package, this also adds a `--current-package` as an alternative to `--packages`. This makes it possible to, e.g., write local wrapper scripts that run a specific set of commands on whatever the current package happens to be (such as a generic version of the script discussed in #4129).

As related cleanup, makes the tool non-publishable (we haven't been publishing it since the repo merge, but I never made it unpublishable; this is important now that it would not work if published) and remove the LICENSE and CHANGELOG since it's no longer a stand-alone package.

Fixes flutter/flutter#128231
Fixes flutter/flutter#128232
This was referenced Jul 7, 2023
Merged
Closed
Merged
@reidbaker reidbaker mentioned this pull request Jul 26, 2023
Merged
11 tasks
@reidbaker reidbaker mentioned this pull request Aug 4, 2023
Merged
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stuartmorgan-g stuartmorgan-g stuartmorgan-g approved these changes

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App override: no changelog needed Override the check requiring CHANGELOG updates for most changes override: no versioning needed Override the check requiring version bumps for most changes p: camera p: file_selector p: google_maps_flutter p: google_sign_in p: image_picker p: shared_preferences p: url_launcher p: video_player p: webview_flutter platform-web

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ditman @stuartmorgan-g