Repository navigation
Conversation
… targets Extends `BuildService` and `package:flutter_tools_core` to support `flutter assemble` targets contributed by tool extensions (`ExtensionAssembleTarget`, `ExtensionTarget`, `ExtensionBuildContext`, `Source`, `Artifact`, `HostArtifact`, and `Target`), allowing extension targets to participate in the build graph alongside built-in targets.
github-actions
Bot
added
tool
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request integrates custom build targets defined by tool extensions into the Flutter build system, updating AssembleCommand and BuildCommand to discover and execute these targets, and expanding the prototype Linux extension with several concrete build targets. The review feedback highlights potential crashes from unhandled ArgumentErrors when parsing invalid platform strings with TargetPlatform.fromName in ExtensionAssembleTarget, ArtifactResolver, and ExtensionBuildSubCommand, recommending wrapping these in try-catch blocks to throw user-friendly ToolExits. Additionally, a more robust path extraction method is suggested in the Linux prototype's directory copying logic to avoid potential truncation bugs when trailing separators are present.
| final resolver = ArtifactResolver( | ||
| artifacts: artifacts, | ||
| buildMode: mode, | ||
| targetPlatform: buildTarget.targetPlatform.isNotEmpty | ||
| ? TargetPlatform.fromName(buildTarget.targetPlatform) | ||
| : null, | ||
| ); |
There was a problem hiding this comment.
TargetPlatform.fromName throws an ArgumentError if the platform name string is invalid or unrecognized. Since tool extensions are external and can define arbitrary platform strings (or have typos), calling TargetPlatform.fromName directly without a try-catch block will cause the Flutter tool to crash with an unhandled exception and a raw stack trace.
Wrapping this call in a try-catch block and throwing a user-friendly ToolExit instead ensures a much better user experience, in line with our style guide principle that error messages should be useful.
| final resolver = ArtifactResolver( | |
| artifacts: artifacts, | |
| buildMode: mode, | |
| targetPlatform: buildTarget.targetPlatform.isNotEmpty | |
| ? TargetPlatform.fromName(buildTarget.targetPlatform) | |
| : null, | |
| ); | |
| TargetPlatform? targetPlatform; | |
| if (buildTarget.targetPlatform.isNotEmpty) { | |
| try { | |
| targetPlatform = TargetPlatform.fromName(buildTarget.targetPlatform); | |
| } on ArgumentError { | |
| throwToolExit('Invalid target platform "${buildTarget.targetPlatform}" for extension build target "${buildTarget.name}".'); | |
| } | |
| } | |
| final resolver = ArtifactResolver( | |
| artifacts: artifacts, | |
| buildMode: mode, | |
| targetPlatform: targetPlatform, | |
| ); |
References
- Error messages should be useful: Every error message is an opportunity to make someone love our product. (link)
| final TargetPlatform? platform = platformName != null | ||
| ? TargetPlatform.fromName(platformName) | ||
| : null; |
There was a problem hiding this comment.
Similarly, if platformName is invalid here, TargetPlatform.fromName will throw an ArgumentError and crash the build process with a raw stack trace. We should handle this gracefully by catching the error and throwing a ToolExit.
| final TargetPlatform? platform = platformName != null | |
| ? TargetPlatform.fromName(platformName) | |
| : null; | |
| TargetPlatform? platform; | |
| if (platformName != null) { | |
| try { | |
| platform = TargetPlatform.fromName(platformName); | |
| } on ArgumentError { | |
| throwToolExit('Invalid platform name "$platformName" for artifact "${artifact.name}".'); | |
| } | |
| } |
References
- Error messages should be useful: Every error message is an opportunity to make someone love our product. (link)
| final resolver = ArtifactResolver( | ||
| artifacts: artifacts, | ||
| buildMode: buildInfo.mode, | ||
| targetPlatform: target.targetPlatform.isNotEmpty | ||
| ? TargetPlatform.fromName(target.targetPlatform) | ||
| : null, | ||
| ); |
There was a problem hiding this comment.
TargetPlatform.fromName throws an ArgumentError if the platform name string is invalid or unrecognized. We should wrap this call in a try-catch block and throw a user-friendly ToolExit to prevent raw crashes when processing invalid platform strings from tool extensions.
| final resolver = ArtifactResolver( | |
| artifacts: artifacts, | |
| buildMode: buildInfo.mode, | |
| targetPlatform: target.targetPlatform.isNotEmpty | |
| ? TargetPlatform.fromName(target.targetPlatform) | |
| : null, | |
| ); | |
| TargetPlatform? targetPlatform; | |
| if (target.targetPlatform.isNotEmpty) { | |
| try { | |
| targetPlatform = TargetPlatform.fromName(target.targetPlatform); | |
| } on ArgumentError { | |
| throwToolExit('Invalid target platform "${target.targetPlatform}" for extension build target "${target.name}".'); | |
| } | |
| } | |
| final resolver = ArtifactResolver( | |
| artifacts: artifacts, | |
| buildMode: buildInfo.mode, | |
| targetPlatform: targetPlatform, | |
| ); |
References
- Error messages should be useful: Every error message is an opportunity to make someone love our product. (link)
| for (final FileSystemEntity entity in source.listSync()) { | ||
| final String name = entity.path.substring(source.path.length + 1); | ||
| if (entity is Directory) { | ||
| _copyDirectory(entity, Directory('${destination.path}/$name')); | ||
| } else if (entity is File) { | ||
| entity.copySync('${destination.path}/$name'); | ||
| } | ||
| return ExtensionBuildResult(success: false, errorMessage: 'Unknown build target: $targetName'); | ||
| } |
There was a problem hiding this comment.
Using entity.path.substring(source.path.length + 1) to extract the relative name of the file or directory assumes that source.path does not end with a separator (like /). If source.path ends with a separator, source.path.length + 1 will slice too far and truncate the first character of the child's name, causing a bug/file copy failure.
A much more robust and standard way is to use entity.uri.pathSegments.lastWhere((e) => e.isNotEmpty) to get the base name of the entity, which works perfectly regardless of trailing slashes.
| for (final FileSystemEntity entity in source.listSync()) { | |
| final String name = entity.path.substring(source.path.length + 1); | |
| if (entity is Directory) { | |
| _copyDirectory(entity, Directory('${destination.path}/$name')); | |
| } else if (entity is File) { | |
| entity.copySync('${destination.path}/$name'); | |
| } | |
| return ExtensionBuildResult(success: false, errorMessage: 'Unknown build target: $targetName'); | |
| } | |
| for (final FileSystemEntity entity in source.listSync()) { | |
| final String name = entity.uri.pathSegments.lastWhere((e) => e.isNotEmpty); | |
| if (entity is Directory) { | |
| _copyDirectory(entity, Directory('${destination.path}/$name')); | |
| } else if (entity is File) { | |
| entity.copySync('${destination.path}/$name'); | |
| } | |
| } |
…and use pathSegments in prototype build
…d and align build APIs
|
from triage: is this ready for review? :) |
Keep host Source and Target untouched in flutter_tools and convert extension Sources inside ExtensionAssembleTarget, reverting changes to built-in targets and Source.fromProject.
github-actions
Bot
removed
team-android
Not yet! I'll mark it as Draft :) |
2 participants
Extends
BuildServiceandpackage:flutter_tools_coreso tool extensions can defineTargetnodes (ExtensionTarget) withSource,Artifact, andHostArtifactinputs/outputs and dependencies on built-in or custom targets. IntegratesExtensionAssembleTargetintoflutter assembleandflutter buildso extension-provided targets can execute within the Flutter build graph and receive resolved environment paths and engine artifacts over RPC viaExtensionBuildContext.Part of #180258
Pre-launch Checklist
///).