From a48a1f07b8f8fe6b74371272e9984552b43a3f9f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Sharma?= <737941+loic-sharma@users.noreply.github.com> Date: Mon, 31 Mar 2025 22:15:25 -0700 Subject: [PATCH] [tool] Improve using project files in build targets (#166211) The Flutter tool's build system executes targets that take inputs and produces outputs. Previously, targets would hardcode paths if they needed to use Flutter project files as inputs/outputs. For example: ```dart class FooTarget extends Target { @override List get inputs => [ Source.pattern('{PROJECT_DIR}/foo/bar'), ]; } ``` This is problematic as the [`FlutterProject`](https://github.com/flutter/flutter/blob/05b5e79105441acb55e98a778ff7854cd191de8c/packages/flutter_tools/lib/src/project.dart#L89) is the source of truth for where a Flutter project's files are located: 1. If we change a project file's location, we need to update `FlutterProject` as well as any hardcoded target inputs/outputs. 2. Project files' location can be dynamic. For example, a Flutter app puts iOS files in the `ios/` directory, but a Flutter module puts iOS files in the `.ios/` directory. Targets need to duplicate `FlutterProject`'s logic to determine the project file's location. As a result, hardcoding project file paths in targets can be error-prone. This introduces a new `Source` factory that lets you use the `FlutterProject` to create the source: ```dart class FooTarget extends Target { @override List get inputs => [ Source.fromProject((FlutterProject project) => project.fooFile), ]; } ``` Part of https://github.com/flutter/flutter/issues/163874 Next pull request: https://github.com/flutter/flutter/pull/165916 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --- .../lib/src/build_system/source.dart | 51 +++++++++++++++- .../targets/dart_plugin_registrant.dart | 5 +- .../lib/src/build_system/targets/ios.dart | 11 +--- .../build_system/source_test.dart | 60 +++++++++++++++++++ 4 files changed, 113 insertions(+), 14 deletions(-) diff --git a/packages/flutter_tools/lib/src/build_system/source.dart b/packages/flutter_tools/lib/src/build_system/source.dart index 6837c5d173..305dc06dc6 100644 --- a/packages/flutter_tools/lib/src/build_system/source.dart +++ b/packages/flutter_tools/lib/src/build_system/source.dart @@ -5,6 +5,7 @@ import '../artifacts.dart'; import '../base/file_system.dart'; import '../build_info.dart'; +import '../project.dart'; import 'build_system.dart'; import 'exceptions.dart'; @@ -12,7 +13,7 @@ import 'exceptions.dart'; // // // ✨ THINKING OF MOVING/REFACTORING THIS FILE? READ ME FIRST! ✨ // // // -// There is a link to this file in //docs/tool/Engine-artfiacts.md // +// There is a link to this file in //docs/tool/Engine-artifacts.md // // and it would be very kind of you to update the link, if needed. // // // ////////////////////////////////////////////////////////////////////// @@ -42,6 +43,9 @@ class SourceVisitor implements ResolvedFiles { /// Defaults to `true`. final bool inputs; + /// The current project. + late final FlutterProject _project = FlutterProject.fromDirectory(environment.projectDir); + @override final List sources = []; @@ -222,12 +226,26 @@ class SourceVisitor implements ResolvedFiles { } sources.add(entity as File); } + + void visitProjectSource(ProjectSourceBuilder builder, bool optional) { + final File source = builder(_project); + final String path = source.absolute.path; + + if (optional && !environment.fileSystem.isFileSync(path)) { + return; + } + + sources.add(environment.fileSystem.file(path)); + } } /// A description of an input or output of a [Target]. abstract class Source { /// This source is a file URL which contains some references to magic - /// environment variables. + /// environment variables defined in [Environment]. + /// + /// If [optional] is true, the file is not required to exist. In this case, it + /// is never resolved as an input. const factory Source.pattern(String pattern, {bool optional}) = _PatternSource; /// The source is provided by an [Artifact]. @@ -241,6 +259,20 @@ abstract class Source { /// If [artifact] points to a directory then all child files are included. const factory Source.hostArtifact(HostArtifact artifact) = _HostArtifactSource; + /// The source is provided by a [FlutterProject]. + /// + /// If [optional] is true, the file is not required to exist. In this case, it + /// is never resolved as an input. + /// + /// Example: + /// + /// ```dart + /// // A project's `pubspec.yaml` file: + /// Source.fromProject((FlutterProject project) => project.pubspecFile); + /// ``` + const factory Source.fromProject(ProjectSourceBuilder sourceBuilder, {bool optional}) = + _ProjectSource; + /// Visit the particular source type. void accept(SourceVisitor visitor); @@ -292,3 +324,18 @@ class _HostArtifactSource implements Source { @override bool get implicit => false; } + +typedef ProjectSourceBuilder = File Function(FlutterProject); + +class _ProjectSource implements Source { + const _ProjectSource(this.builder, {this.optional = false}); + + final ProjectSourceBuilder builder; + final bool optional; + + @override + void accept(SourceVisitor visitor) => visitor.visitProjectSource(builder, optional); + + @override + bool get implicit => false; +} diff --git a/packages/flutter_tools/lib/src/build_system/targets/dart_plugin_registrant.dart b/packages/flutter_tools/lib/src/build_system/targets/dart_plugin_registrant.dart index d0da61c1de..c77bda8f2f 100644 --- a/packages/flutter_tools/lib/src/build_system/targets/dart_plugin_registrant.dart +++ b/packages/flutter_tools/lib/src/build_system/targets/dart_plugin_registrant.dart @@ -76,9 +76,6 @@ class DartPluginRegistrantTarget extends Target { @override List get outputs => [ - const Source.pattern( - '{PROJECT_DIR}/.dart_tool/flutter_build/dart_plugin_registrant.dart', - optional: true, - ), + Source.fromProject((FlutterProject project) => project.dartPluginRegistrant, optional: true), ]; } diff --git a/packages/flutter_tools/lib/src/build_system/targets/ios.dart b/packages/flutter_tools/lib/src/build_system/targets/ios.dart index e1e5fcaada..04052c2f6c 100644 --- a/packages/flutter_tools/lib/src/build_system/targets/ios.dart +++ b/packages/flutter_tools/lib/src/build_system/targets/ios.dart @@ -444,14 +444,9 @@ abstract class IosLLDBInit extends Target { ]; @override - List get outputs { - final FlutterProject flutterProject = FlutterProject.current(); - final String lldbInitFilePath = flutterProject.ios.lldbInitFile.path.replaceFirst( - flutterProject.directory.path, - '{PROJECT_DIR}/', - ); - return [Source.pattern(lldbInitFilePath)]; - } + List get outputs => [ + Source.fromProject((FlutterProject project) => project.ios.lldbInitFile), + ]; @override List get dependencies => []; diff --git a/packages/flutter_tools/test/general.shard/build_system/source_test.dart b/packages/flutter_tools/test/general.shard/build_system/source_test.dart index 0bfaf226ce..1bfbb61d2a 100644 --- a/packages/flutter_tools/test/general.shard/build_system/source_test.dart +++ b/packages/flutter_tools/test/general.shard/build_system/source_test.dart @@ -2,6 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +import 'package:file_testing/file_testing.dart'; import 'package:flutter_tools/src/artifacts.dart'; import 'package:flutter_tools/src/base/file_system.dart'; import 'package:flutter_tools/src/base/platform.dart'; @@ -9,6 +10,7 @@ import 'package:flutter_tools/src/build_info.dart'; import 'package:flutter_tools/src/build_system/build_system.dart'; import 'package:flutter_tools/src/build_system/exceptions.dart'; import 'package:flutter_tools/src/globals.dart' as globals; +import 'package:flutter_tools/src/project.dart'; import '../../src/common.dart'; import '../../src/fake_process_manager.dart'; @@ -282,4 +284,62 @@ void main() { expect(visitor.sources.single.path, contains('engine.stamp')); }), ); + + test( + 'can substitute project file', + () => testbed.run(() { + final String path = globals.fs.file('.flutter-plugins-dependencies').absolute.path; + globals.fs.file(path).createSync(recursive: true); + final Source pluginsSource = Source.fromProject( + (FlutterProject project) => project.flutterPluginsDependenciesFile, + ); + pluginsSource.accept(visitor); + + expect(visitor.sources.single.absolute.path, path); + expect(visitor.sources.single, exists); + }), + ); + + test( + 'can substitute nonexistent project file', + () => testbed.run(() { + final String path = globals.fs.file('.flutter-plugins-dependencies').absolute.path; + final Source pluginsSource = Source.fromProject( + (FlutterProject project) => project.flutterPluginsDependenciesFile, + ); + pluginsSource.accept(visitor); + + expect(visitor.sources.single.absolute.path, path); + expect(visitor.sources.single, isNot(exists)); + }), + ); + + test( + 'can substitute optional project file', + () => testbed.run(() { + final String path = globals.fs.file('.flutter-plugins-dependencies').absolute.path; + globals.fs.file(path).createSync(recursive: true); + final Source pluginsSource = Source.fromProject( + (FlutterProject project) => project.flutterPluginsDependenciesFile, + optional: true, + ); + pluginsSource.accept(visitor); + + expect(visitor.sources.single.absolute.path, path); + expect(visitor.sources.single, exists); + }), + ); + + test( + 'skips nonexistent optional project file', + () => testbed.run(() { + final Source pluginsSource = Source.fromProject( + (FlutterProject project) => project.flutterPluginsDependenciesFile, + optional: true, + ); + pluginsSource.accept(visitor); + + expect(visitor.sources.isEmpty, isTrue); + }), + ); }