Unverified Commit a6118612 authored by Christopher Fujino's avatar Christopher Fujino Committed by GitHub

Fix flutter attach local engine (#131825)

Fixes: https://github.com/flutter/flutter/issues/124970
Part of https://github.com/flutter/flutter/issues/47161

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

1. if/when we parse local-engine CLI options: https://github.com/flutter/flutter/blob/1cf3907407cbc91be7cec2c38b348a2d66041dd5/packages/flutter_tools/lib/src/runner/flutter_command_runner.dart#L281
2. an additional override for fuchsia platform dill (no longer used, deleted in this PR): https://github.com/flutter/flutter/blob/1cf3907407cbc91be7cec2c38b348a2d66041dd5/packages/flutter_tools/lib/src/commands/attach.dart#L274

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: https://github.com/flutter/flutter/blob/1cf3907407cbc91be7cec2c38b348a2d66041dd5/packages/flutter_tools/lib/src/context_runner.dart#L137
2. An instance of `CachedLocalEngineArtifacts` created in the command runner once the CLI options have been parsed: https://github.com/flutter/flutter/blob/1cf3907407cbc91be7cec2c38b348a2d66041dd5/packages/flutter_tools/lib/src/runner/flutter_command_runner.dart#L281

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: https://github.com/flutter/flutter/blob/1cf3907407cbc91be7cec2c38b348a2d66041dd5/packages/flutter_tools/lib/src/resident_runner.dart#L80

Device.artifactOverrides was originally introduced in https://github.com/flutter/flutter/pull/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()`](https://github.com/flutter/flutter/blob/1cf3907407cbc91be7cec2c38b348a2d66041dd5/packages/flutter_tools/lib/src/resident_runner.dart#L80-L218), 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 https://github.com/flutter/flutter/issues/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.
parent 64a0683b
......@@ -156,7 +156,6 @@ List<FlutterCommand> generateCommands({
AssembleCommand(verboseHelp: verboseHelp, buildSystem: globals.buildSystem),
AttachCommand(
verboseHelp: verboseHelp,
artifacts: globals.artifacts,
stdio: globals.stdio,
logger: globals.logger,
terminal: globals.terminal,
......
......@@ -7,9 +7,7 @@ import 'dart:async';
import 'package:vm_service/vm_service.dart';
import '../android/android_device.dart';
import '../artifacts.dart';
import '../base/common.dart';
import '../base/context.dart';
import '../base/file_system.dart';
import '../base/io.dart';
import '../base/logger.dart';
......@@ -65,7 +63,6 @@ class AttachCommand extends FlutterCommand {
AttachCommand({
bool verboseHelp = false,
HotRunnerFactory? hotRunnerFactory,
required Artifacts? artifacts,
required Stdio stdio,
required Logger logger,
required Terminal terminal,
......@@ -73,8 +70,7 @@ class AttachCommand extends FlutterCommand {
required Platform platform,
required ProcessInfo processInfo,
required FileSystem fileSystem,
}): _artifacts = artifacts,
_hotRunnerFactory = hotRunnerFactory ?? HotRunnerFactory(),
}) : _hotRunnerFactory = hotRunnerFactory ?? HotRunnerFactory(),
_stdio = stdio,
_logger = logger,
_terminal = terminal,
......@@ -145,7 +141,6 @@ class AttachCommand extends FlutterCommand {
}
final HotRunnerFactory _hotRunnerFactory;
final Artifacts? _artifacts;
final Stdio _stdio;
final Logger _logger;
final Terminal _terminal;
......@@ -267,13 +262,7 @@ known, it can be explicitly provided to attach via the command-line, e.g.
throwToolExit('Did not find any valid target devices.');
}
final Artifacts? overrideArtifacts = device.artifactOverrides ?? _artifacts;
await context.run<void>(
body: () => _attachToDevice(device),
overrides: <Type, Generator>{
Artifacts: () => overrideArtifacts,
},
);
await _attachToDevice(device);
return FlutterCommandResult.success();
}
......
......@@ -585,7 +585,7 @@ class DefaultResidentCompiler implements ResidentCompiler {
required this.buildMode,
required Logger logger,
required ProcessManager processManager,
required Artifacts artifacts,
required this.artifacts,
required Platform platform,
required FileSystem fileSystem,
this.testCompilation = false,
......@@ -604,7 +604,6 @@ class DefaultResidentCompiler implements ResidentCompiler {
@visibleForTesting StdoutHandler? stdoutHandler,
}) : _logger = logger,
_processManager = processManager,
_artifacts = artifacts,
_stdoutHandler = stdoutHandler ?? StdoutHandler(logger: logger, fileSystem: fileSystem),
_platform = platform,
dartDefines = dartDefines ?? const <String>[],
......@@ -615,7 +614,7 @@ class DefaultResidentCompiler implements ResidentCompiler {
final Logger _logger;
final ProcessManager _processManager;
final Artifacts _artifacts;
final Artifacts artifacts;
final Platform _platform;
final bool testCompilation;
......@@ -751,12 +750,12 @@ class DefaultResidentCompiler implements ResidentCompiler {
{String? additionalSourceUri}
) async {
final TargetPlatform? platform = (targetModel == TargetModel.dartdevc) ? TargetPlatform.web_javascript : null;
final String frontendServer = _artifacts.getArtifactPath(
final String frontendServer = artifacts.getArtifactPath(
Artifact.frontendServerSnapshotForEngineDartSdk,
platform: platform,
);
final List<String> command = <String>[
_artifacts.getArtifactPath(Artifact.engineDartBinary, platform: platform),
artifacts.getArtifactPath(Artifact.engineDartBinary, platform: platform),
'--disable-dart-dev',
frontendServer,
'--sdk-root',
......
......@@ -8,7 +8,6 @@ import 'dart:math' as math;
import 'package:meta/meta.dart';
import 'application_package.dart';
import 'artifacts.dart';
import 'base/context.dart';
import 'base/dds.dart';
import 'base/file_system.dart';
......@@ -741,9 +740,6 @@ abstract class Device {
/// Clear the device's logs.
void clearLogs();
/// Optional device-specific artifact overrides.
OverrideArtifacts? get artifactOverrides => null;
/// Start an app package on the current device.
///
/// [platformArgs] allows callers to pass platform-specific arguments to the
......
......@@ -5,7 +5,6 @@
import 'package:args/args.dart';
import 'package:args/command_runner.dart';
import 'package:file/memory.dart';
import 'package:flutter_tools/src/artifacts.dart';
import 'package:flutter_tools/src/base/file_system.dart';
import 'package:flutter_tools/src/base/io.dart';
import 'package:flutter_tools/src/base/logger.dart';
......@@ -75,7 +74,6 @@ void main() {
),
),
AttachCommand(
artifacts: Artifacts.test(),
stdio: FakeStdio(),
logger: logger,
terminal: FakeTerminal(),
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment