Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[ci] use background script #4349

Merged
merged 3 commits into from
Sep 14, 2021
Merged

[ci] use background script #4349

merged 3 commits into from
Sep 14, 2021

Conversation

fkorotkov
Copy link
Contributor

There is a special instruction in Cirrus for that https://cirrus-ci.org/guide/writing-tasks/#background-script-instruction

Might be related to flutter/flutter#90078

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. (Note that unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin 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.
  • I updated CHANGELOG.md to add a description of the change.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the 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.

@google-cla google-cla bot added the cla: yes label Sep 14, 2021
Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

LGTM! Let's try this and see if it helps the repo recover!

- git clone https://github.com/flutter/web_installers.git
- cd web_installers/packages/web_drivers/
- dart pub get
- dart lib/web_driver_installer.dart chromedriver --install-only
- ./chromedriver/chromedriver --port=4444 &
- ./chromedriver/chromedriver --port=4444
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably want this to be a separate script step that is background, and the rest to stay as-is.

Copy link
Member

Choose a reason for hiding this comment

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

      install_script:
        - git clone https://github.com/flutter/web_installers.git
        - cd web_installers/packages/web_drivers/
        - dart pub get
        - dart lib/web_driver_installer.dart chromedriver --install-only
      install_background_script:
        - ./chromedriver/chromedriver --port=4444

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See 02dac56

Copy link
Member

@ditman ditman 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 probably going to fail:

dart lib/web_driver_installer.dart chromedriver --install-only
Error when reading 'lib/web_driver_installer.dart': No such file or directory.

Does this need to cd at the beginning of the install_background_script (similarly to how we cd web_installers/packages/web_drivers/ at the beginning of install_script?)

@fkorotkov
Copy link
Contributor Author

Does seems like it's working now: https://cirrus-ci.com/task/6037670892666880?logs=chromedriver#L10

Waiting for all the checks to pass.

@ditman
Copy link
Member

ditman commented Sep 14, 2021

I'll merge on red when the rest of the tests pass. Thanks @fkorotkov for the super fast fix!

@ditman
Copy link
Member

ditman commented Sep 14, 2021

@fkorotkov
Copy link
Contributor Author

@ditman it might failed to stream in real-time and will try to recover once the whole tasks finishes... Let's wait.

@stuartmorgan-g
Copy link
Contributor

Do we know why the old version suddenly broke?

@ditman
Copy link
Member

ditman commented Sep 14, 2021

@fkorotkov
Copy link
Contributor Author

@stuartmorgan yeah, seems this is the reason flutter/flutter#90078 (comment)

@stuartmorgan-g
Copy link
Contributor

I'll merge on red when the rest of the tests pass. Thanks @fkorotkov for the super fast fix!

Since this can't possible affect any tasks other than the one in changed, and those are green, I'm going to land now to green the tree.

@stuartmorgan-g stuartmorgan-g merged commit b85edeb into flutter:master Sep 14, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 14, 2021
amantoux pushed a commit to amantoux/plugins that referenced this pull request Sep 27, 2021
Uses the background script annotation, since & no longer works.
KyleFin pushed a commit to KyleFin/plugins that referenced this pull request Dec 21, 2021
Uses the background script annotation, since & no longer works.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants