Skip to content

Conversation

@matanlurey
Copy link
Contributor

Partial work towards #132245.

I also couldn't help myself to do a very minor refactor and add some comments to LocalEngineInfo because I was getting confused myself implementing it.

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@github-actions github-actions bot added platform-ios iOS applications specifically tool Affects the "flutter" command-line tool. See also t: labels. a: desktop Running on desktop labels Aug 15, 2023
@matanlurey matanlurey force-pushed the tool-local-engine-host-linwin branch from 676aa2b to b195ac0 Compare August 15, 2023 19:31
Copy link
Contributor

@jonahwilliams jonahwilliams 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 self-assigned this Aug 15, 2023
@matanlurey matanlurey requested a review from cbracken August 16, 2023 19:07
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

if (localEngineInfo != null) {
final String engineOutPath = localEngineInfo.engineOutPath;
final String engineOutPath = localEngineInfo.targetOutPath;
// $ENGINE/src/out/foo_bar_baz -> $ENGINE/src
Copy link
Member

@cbracken cbracken Aug 16, 2023

Choose a reason for hiding this comment

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

Maybe for context:

Suggested change
// $ENGINE/src/out/foo_bar_baz -> $ENGINE/src
// Get the engine source root: $ENGINE/src/out/foo_bar_baz -> $ENGINE/src

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

@cbracken cbracken left a comment

Choose a reason for hiding this comment

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

LGTM stamp from a Japanese personal seal

@auto-submit auto-submit bot merged commit 223ae5d into flutter:master Aug 16, 2023
@matanlurey matanlurey deleted the tool-local-engine-host-linwin branch August 16, 2023 22:34
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: desktop Running on desktop autosubmit Merge PR when tree becomes green via auto submit App platform-ios iOS applications specifically tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants