Skip to content

Treat missing --local-engine-host as fatal on CI-like systems. #132707

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
auto-submit merged 2 commits into from
Aug 17, 2023
Merged

Treat missing --local-engine-host as fatal on CI-like systems. #132707

auto-submit merged 2 commits into from
Aug 17, 2023

Conversation

@matanlurey
Copy link
Contributor

@matanlurey matanlurey commented Aug 16, 2023

Partial work towards #132245.

The goal here is to "sniff" out any missing pieces that would block engine builds, rolls, benchmarks and so on before requiring humans to provide the parameter. The implementation is based on a short discussion with @christopherfujino:

@matanlurey:

Not sure whether to post here or ⁠hackers-infra-🌡 , but is there a way to (and is it advisable to) detect whether the tool is running in a CI environment? I'd like to "soft enforce" --local-engine-host being provided strictly on CI, make sure that lands well, and then "upgrade" it to being non-CI invocations as well (re: #132245).

Also happy to get talked out of this idea 🙂

@christopherfujino:

we have a check, lemme find it
whether or not it is advisable, idk
https://github.com/flutter/flutter/blob/flutter-3.14-candidate.0/packages/flutter_tools/lib/src/base/bot_detector.dart#L30

(...)

is your desire to get early signal before enforcing t his for humans to prevent functionality churn of landing and reverting and re-landing?

(yes)

uhh, sure, that's advisable 🙂

@github-actions github-actions bot added a: text input Entering text in a text field or keyboard related problems tool Affects the "flutter" command-line tool. See also t: labels. labels Aug 16, 2023
christopherfujino
christopherfujino approved these changes Aug 16, 2023
View reviewed changes
Copy link
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

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

LGTM

@matanlurey matanlurey added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 16, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 16, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Aug 16, 2023

auto label is removed for flutter/flutter/132707, due to - The status or check suite Mac tool_tests_commands has failed. Please fix the issues identified (or deflake) before re-applying this label.

@matanlurey matanlurey added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 16, 2023
@auto-submit auto-submit bot merged commit b66878a into flutter:master Aug 17, 2023
@matanlurey matanlurey deleted the tool-local-engine-host-fatal-on-ci branch August 17, 2023 00:50
@matanlurey
Copy link
Contributor Author

matanlurey commented Aug 17, 2023

/cc @whesse ^

@whesse
Copy link
Contributor

whesse commented Aug 17, 2023

No errors have shown up in the Dart CI after this change landed.

@whesse
Copy link
Contributor

whesse commented Aug 17, 2023

Errors have shown up on Flutter engine CI: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8772529585020554513/+/u/Framework_analyze/Update_packages/stdout

@whesse
Copy link
Contributor

whesse commented Aug 17, 2023

A fix for the flutter engine CI errors is at https://flutter-review.googlesource.com/c/recipes/+/49620

Please log in to this review host, which creates an account, so you can be added as CC on reviews on this site.

A further use of --local-engine with a matching --local-engine-host is in the devicelab drone runners, which get their local engine value from an environment variable LOCAL_ENGINE, with no matching environment variable LOCAL_ENGINE_HOST. They pass the --local-engine flag to the devicelab test runner, which has had the corresponding flag local-engine-host added in https://github.com/flutter/flutter/blob/master/dev/devicelab/lib/command/test.dart#L40.

I don't know why these have not started failing since this CL landed. It may be that no CI builders pass the LOCAL_ENGINE flag to these drones any more, since engine testing has migrated to engine_v2 which does not use local-engine. If this is the case, then the support for LOCAL_ENGINE should be removed from the recipe, instead of adding support for LOCAL_ENGINE_HOST and providing it.

@matanlurey matanlurey mentioned this pull request Aug 17, 2023
Closed
13 tasks
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 17, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
@engine-flutter-autoroll
b66878a Treat missing --local-engine-host as fatal on CI-like systems. (
cbd2a16 
flutter/flutter#132707)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 18, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 18, 2023
@engine-flutter-autoroll
b66878a Treat missing --local-engine-host as fatal on CI-like systems. (
74d0f30 
flutter/flutter#132707)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 18, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 18, 2023
@engine-flutter-autoroll
b66878a Treat missing --local-engine-host as fatal on CI-like systems. (
42fc362 
flutter/flutter#132707)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 18, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 18, 2023
@engine-flutter-autoroll
b66878a Treat missing --local-engine-host as fatal on CI-like systems. (
61685b8 
flutter/flutter#132707)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 18, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 18, 2023
@engine-flutter-autoroll
b66878a Treat missing --local-engine-host as fatal on CI-like systems. (
c6ae9d5 
flutter/flutter#132707)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 15, 2023
@engine-flutter-autoroll
b66878a Treat missing --local-engine-host as fatal on CI-like systems. (
cba38a5 
flutter/flutter#132707)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@christopherfujino christopherfujino christopherfujino approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

a: text input Entering text in a text field or keyboard related problems autosubmit Merge PR when tree becomes green via auto submit App tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@matanlurey @whesse @christopherfujino