Skip to content

Conversation

@christopherfujino
Copy link
Contributor

@christopherfujino christopherfujino commented Aug 3, 2023

Fixes: #124970
Part of #47161

Before this change, there were two places we overrode the Artifacts in a Zone:

  1. if/when we parse local-engine CLI options:
    Artifacts: Artifacts.getLocalEngine(engineBuildPaths),
  2. an additional override for fuchsia platform dill (no longer used, deleted in this PR):
    Artifacts: () => overrideArtifacts,

Note 1 above creates a new instance of Artifacts.getLocalEngine(). In this flow, there exist two instances of Artifacts:

  1. The default fallback instance of CachedArtifacts (which gets all artifacts from flutter/bin/cache), instantiated in context_runner.dart:
    Artifacts: () => CachedArtifacts(
  2. An instance of CachedLocalEngineArtifacts created in the command runner once the CLI options have been parsed:
    Artifacts: Artifacts.getLocalEngine(engineBuildPaths),

The regression happened when we direct injected the Artifacts 1 from above BEFORE we parsed the local-engine flag, and then used this in the second zone override, and then when creating the FlutterDevice there are multiple calls to globals.artifacts returned it when it should have returned Artifacts 2:

static Future<FlutterDevice> create(

Device.artifactOverrides was originally introduced in #32071, but is no longer used, so I deleted it.

I also removed direct injection of Artifacts to the attach sub-command, because that class now no longer references artifacts.

I believe the ideal true fix for this would be to:

  1. Migrate all leaf calls to globals.artifacts to use direct injection (in this case, the offending invocations were in FlutterDevice.create(), but I'm not sure that something else would not have broken later)
  2. Ensure we are always direct injecting the desired instance of Artifacts--that is, if the user desires local engine artifacts, that we are passing an instance of CachedLocalEngineArtifacts.
    a. Alternatively, and probably simpler, teach CachedArtifacts to know about the local engine. This would mean parsing the global CLI options BEFORE we ever construct any instance of Artifacts.

As an overall recommendation for implementing #47161, in the overall tree of tool function calls, we should probably migrate the leaves first (that is, migrate the sub-commands last). We should also audit and reconsider any usage of runZoned() or context.run() for the purpose overriding zoneValues.

@github-actions github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Aug 3, 2023
@christopherfujino christopherfujino force-pushed the fix-flutter-attach-local-engine branch from f09aee4 to f406070 Compare August 3, 2023 16:34
Comment on lines +265 to +267
expect(artifacts, isA<CachedLocalEngineArtifacts>());
// expecting this to be true ensures this test ran
passedArtifactTest = true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI: this is what we are actually testing

@christopherfujino christopherfujino marked this pull request as ready for review August 4, 2023 18:14
Copy link
Contributor

@eliasyishak eliasyishak left a comment

Choose a reason for hiding this comment

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

LGTM

@christopherfujino christopherfujino added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 9, 2023
@christopherfujino
Copy link
Contributor Author

FYI @moffatman

@auto-submit auto-submit bot merged commit a611861 into flutter:master Aug 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 10, 2023
@christopherfujino christopherfujino deleted the fix-flutter-attach-local-engine branch August 10, 2023 19:53
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

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

Development

Successfully merging this pull request may close these issues.

flutter attach doesn't respect --local-engine

2 participants