diff --git a/testing/run_tests.py b/testing/run_tests.py index 6a807c65021a6..9c4a6c254dbc3 100755 --- a/testing/run_tests.py +++ b/testing/run_tests.py @@ -1045,6 +1045,25 @@ def gather_engine_repo_tools_tests(build_dir): ) +def gather_git_repo_tools_tests(build_dir): + test_dir = os.path.join( + BUILDROOT_DIR, 'flutter', 'tools', 'pkg', 'git_repo_tools' + ) + dart_tests = glob.glob('%s/*_test.dart' % test_dir) + for dart_test_file in dart_tests: + opts = [ + '--disable-dart-dev', + dart_test_file, + ] + yield EngineExecutableTask( + build_dir, + os.path.join('dart-sdk', 'bin', 'dart'), + None, + flags=opts, + cwd=test_dir + ) + + def gather_api_consistency_tests(build_dir): test_dir = os.path.join(BUILDROOT_DIR, 'flutter', 'tools', 'api_check') dart_tests = glob.glob('%s/test/*_test.dart' % test_dir) @@ -1355,6 +1374,7 @@ def main(): tasks += list(gather_build_bucket_golden_scraper_tests(build_dir)) tasks += list(gather_engine_build_configs_tests(build_dir)) tasks += list(gather_engine_repo_tools_tests(build_dir)) + tasks += list(gather_git_repo_tools_tests(build_dir)) tasks += list(gather_api_consistency_tests(build_dir)) tasks += list(gather_path_ops_tests(build_dir)) tasks += list(gather_const_finder_tests(build_dir)) diff --git a/tools/clang_tidy/lib/clang_tidy.dart b/tools/clang_tidy/lib/clang_tidy.dart index e8cb4db6e9f9e..8a2308eb49e8d 100644 --- a/tools/clang_tidy/lib/clang_tidy.dart +++ b/tools/clang_tidy/lib/clang_tidy.dart @@ -6,13 +6,13 @@ import 'dart:convert' show LineSplitter, jsonDecode; import 'dart:io' as io show File, stderr, stdout; import 'package:engine_repo_tools/engine_repo_tools.dart'; +import 'package:git_repo_tools/git_repo_tools.dart'; import 'package:meta/meta.dart'; import 'package:path/path.dart' as path; import 'package:process/process.dart'; import 'package:process_runner/process_runner.dart'; import 'src/command.dart'; -import 'src/git_repo.dart'; import 'src/options.dart'; const String _linterOutputHeader = ''' @@ -211,7 +211,7 @@ class ClangTidy { .toList(); } - final GitRepo repo = GitRepo( + final GitRepo repo = GitRepo.fromRoot( options.repoPath, processManager: _processManager, verbose: options.verbose, diff --git a/tools/clang_tidy/pubspec.yaml b/tools/clang_tidy/pubspec.yaml index b06cd30247821..3dbe9e04ac86a 100644 --- a/tools/clang_tidy/pubspec.yaml +++ b/tools/clang_tidy/pubspec.yaml @@ -18,6 +18,7 @@ environment: dependencies: args: any + git_repo_tools: any engine_repo_tools: any meta: any path: any @@ -28,6 +29,7 @@ dev_dependencies: async_helper: any expect: any litetest: any + process_fakes: any smith: any dependency_overrides: @@ -45,6 +47,8 @@ dependency_overrides: path: ../../../third_party/dart/pkg/expect file: path: ../../../third_party/pkg/file/packages/file + git_repo_tools: + path: ../pkg/git_repo_tools litetest: path: ../../testing/litetest meta: @@ -55,6 +59,8 @@ dependency_overrides: path: ../../../third_party/pkg/platform process: path: ../../../third_party/pkg/process + process_fakes: + path: ../pkg/process_fakes process_runner: path: ../../../third_party/pkg/process_runner smith: diff --git a/tools/clang_tidy/test/clang_tidy_test.dart b/tools/clang_tidy/test/clang_tidy_test.dart index 4dbb39e2b125e..819815bed8d97 100644 --- a/tools/clang_tidy/test/clang_tidy_test.dart +++ b/tools/clang_tidy/test/clang_tidy_test.dart @@ -11,9 +11,9 @@ import 'package:engine_repo_tools/engine_repo_tools.dart'; import 'package:litetest/litetest.dart'; import 'package:path/path.dart' as path; import 'package:process/process.dart'; +import 'package:process_fakes/process_fakes.dart'; import 'package:process_runner/process_runner.dart'; -import 'process_fakes.dart'; /// A test fixture for the `clang-tidy` tool. final class Fixture { diff --git a/tools/githooks/pubspec.yaml b/tools/githooks/pubspec.yaml index 07d1820110b37..66755904f2fd5 100644 --- a/tools/githooks/pubspec.yaml +++ b/tools/githooks/pubspec.yaml @@ -45,6 +45,8 @@ dependency_overrides: path: ../../../third_party/dart/pkg/expect file: path: ../../../third_party/pkg/file/packages/file + git_repo_tools: + path: ../pkg/git_repo_tools litetest: path: ../../testing/litetest meta: diff --git a/tools/pkg/git_repo_tools/README.md b/tools/pkg/git_repo_tools/README.md new file mode 100644 index 0000000000000..e308c8104b911 --- /dev/null +++ b/tools/pkg/git_repo_tools/README.md @@ -0,0 +1,23 @@ +# `git_repo_tools` + +This is a repo-internal library for `flutter/engine`, that contains shared code +for writing tools that want to interact with the `git` repository. For example, +finding all changed files in the current branch: + +```dart +import 'dart:io' as io show File, Platform; + +import 'package:engine_repo_tools/engine_repo_tools.dart'; +import 'package:git_repo_tools/git_repo_tools.dart'; +import 'package:path/path.dart' as path; + +void main() async { + // Finds the root of the engine repository from the current script. + final Engine engine = Engine.findWithin(path.dirname(path.fromUri(io.Platform.script))); + final GitRepo gitRepo = GitRepo(engine.flutterDir); + + for (final io.File file in gitRepo.changedFiles) { + print('Changed file: ${file.path}'); + } +} +``` diff --git a/tools/clang_tidy/lib/src/git_repo.dart b/tools/pkg/git_repo_tools/lib/git_repo_tools.dart similarity index 67% rename from tools/clang_tidy/lib/src/git_repo.dart rename to tools/pkg/git_repo_tools/lib/git_repo_tools.dart index 746359bb51198..40fd07747f06b 100644 --- a/tools/clang_tidy/lib/src/git_repo.dart +++ b/tools/pkg/git_repo_tools/lib/git_repo_tools.dart @@ -2,58 +2,66 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -import 'dart:io' as io show Directory, File; +import 'dart:io' as io show Directory, File, stdout; import 'package:path/path.dart' as path; import 'package:process/process.dart'; import 'package:process_runner/process_runner.dart'; -/// Utility methods for working with a git repo. -class GitRepo { +/// Utility methods for working with a git repository. +final class GitRepo { /// The git repository rooted at `root`. - GitRepo(this.root, { + GitRepo.fromRoot(this.root, { this.verbose = false, + StringSink? logSink, ProcessManager processManager = const LocalProcessManager(), - }) : _processManager = processManager; + }) : + _processManager = processManager, + logSink = logSink ?? io.stdout; /// Whether to produce verbose log output. + /// + /// If true, output of git commands will be printed to [logSink]. final bool verbose; + /// Where to send verbose log output. + /// + /// Defaults to [io.stdout]. + final StringSink logSink; + /// The root of the git repo. final io.Directory root; /// The delegate to use for running processes. final ProcessManager _processManager; - List? _changedFiles; - /// Returns a list of all non-deleted files which differ from the nearest /// merge-base with `main`. If it can't find a fork point, uses the default /// merge-base. /// /// This is only computed once and cached. Subsequent invocations of the /// getter will return the same result. - Future> get changedFiles async => - _changedFiles ??= await _getChangedFiles(); - - List? _changedFilesAtHead; - - /// Returns a list of non-deleted files which differ between the HEAD - /// commit and its parent. - Future> get changedFilesAtHead async => - _changedFilesAtHead ??= await _getChangedFilesAtHead(); + late final Future> changedFiles = _changedFiles(); - Future> _getChangedFiles() async { + Future> _changedFiles() async { final ProcessRunner processRunner = ProcessRunner( defaultWorkingDirectory: root, processManager: _processManager, ); await _fetch(processRunner); + // Find the merge base between the current branch and the branch that was + // checked out at the time of the last fetch. The merge base is the common + // ancestor of the two branches, and the output is the hash of the merge + // base. ProcessRunnerResult mergeBaseResult = await processRunner.runProcess( ['git', 'merge-base', '--fork-point', 'FETCH_HEAD', 'HEAD'], failOk: true, ); if (mergeBaseResult.exitCode != 0) { + if (verbose) { + logSink.writeln('git merge-base --fork-point failed, using default merge-base'); + logSink.writeln('Output:\n${mergeBaseResult.stdout}'); + } mergeBaseResult = await processRunner.runProcess([ 'git', 'merge-base', @@ -62,8 +70,7 @@ class GitRepo { ]); } final String mergeBase = mergeBaseResult.stdout.trim(); - final ProcessRunnerResult masterResult = await processRunner - .runProcess([ + final ProcessRunnerResult masterResult = await processRunner.runProcess([ 'git', 'diff', '--name-only', @@ -73,9 +80,17 @@ class GitRepo { return _gitOutputToList(masterResult); } - Future> _getChangedFilesAtHead() async { + /// Returns a list of non-deleted files which differ between the HEAD + /// commit and its parent. + /// + /// This is only computed once and cached. Subsequent invocations of the + /// getter will return the same result. + late final Future> changedFilesAtHead = _changedFilesAtHead(); + + Future> _changedFilesAtHead() async { final ProcessRunner processRunner = ProcessRunner( defaultWorkingDirectory: root, + processManager: _processManager, ); await _fetch(processRunner); final ProcessRunnerResult diffTreeResult = await processRunner.runProcess( @@ -98,6 +113,10 @@ class GitRepo { failOk: true, ); if (fetchResult.exitCode != 0) { + if (verbose) { + logSink.writeln('git fetch upstream main failed, using origin main'); + logSink.writeln('Output:\n${fetchResult.stdout}'); + } await processRunner.runProcess([ 'git', 'fetch', @@ -110,7 +129,7 @@ class GitRepo { List _gitOutputToList(ProcessRunnerResult result) { final String diffOutput = result.stdout.trim(); if (verbose) { - print('git diff output:\n$diffOutput'); + logSink.writeln('git diff output:\n$diffOutput'); } final Set resultMap = {}; resultMap.addAll(diffOutput.split('\n').where( diff --git a/tools/pkg/git_repo_tools/pubspec.yaml b/tools/pkg/git_repo_tools/pubspec.yaml new file mode 100644 index 0000000000000..1cf937a42d22d --- /dev/null +++ b/tools/pkg/git_repo_tools/pubspec.yaml @@ -0,0 +1,60 @@ +# Copyright 2013 The Flutter Authors. All rights reserved. +# Use of this source code is governed by a BSD-style license that can be +# found in the LICENSE file. + +name: git_repo_tools +publish_to: none +environment: + sdk: '>=3.2.0-0 <4.0.0' + +# Do not add any dependencies that require more than what is provided in +# //third_party/pkg, //third_party/dart/pkg, or +# //third_party/dart/third_party/pkg. In particular, package:test is not usable +# here. + +# If you do add packages here, make sure you can run `pub get --offline`, and +# check the .packages and .package_config to make sure all the paths are +# relative to this directory into //third_party/dart + +dependencies: + meta: any + path: any + process: any + process_runner: any + +dev_dependencies: + async_helper: any + expect: any + litetest: any + process_fakes: any + smith: any + +dependency_overrides: + args: + path: ../../../../third_party/dart/third_party/pkg/args + async: + path: ../../../../third_party/dart/third_party/pkg/async + async_helper: + path: ../../../../third_party/dart/pkg/async_helper + collection: + path: ../../../../third_party/dart/third_party/pkg/collection + expect: + path: ../../../../third_party/dart/pkg/expect + file: + path: ../../../../third_party/pkg/file/packages/file + litetest: + path: ../../../testing/litetest + meta: + path: ../../../../third_party/dart/pkg/meta + path: + path: ../../../../third_party/dart/third_party/pkg/path + platform: + path: ../../../../third_party/pkg/platform + process: + path: ../../../../third_party/pkg/process + process_fakes: + path: ../process_fakes + process_runner: + path: ../../../../third_party/pkg/process_runner + smith: + path: ../../../../third_party/dart/pkg/smith diff --git a/tools/pkg/git_repo_tools/test/git_repo_tools_test.dart b/tools/pkg/git_repo_tools/test/git_repo_tools_test.dart new file mode 100644 index 0000000000000..bca022f353a5d --- /dev/null +++ b/tools/pkg/git_repo_tools/test/git_repo_tools_test.dart @@ -0,0 +1,215 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:io' as io; + +import 'package:git_repo_tools/git_repo_tools.dart'; +import 'package:litetest/litetest.dart'; +import 'package:process_fakes/process_fakes.dart'; + +void main() { + const String fakeShaHash = 'fake-sha-hash'; + + + test('returns non-deleted files which differ from merge-base with main', () async { + final Fixture fixture = Fixture( + processManager: FakeProcessManager( + onStart: (List command) { + // Succeed calling "git merge-base --fork-point FETCH_HEAD HEAD". + if (command.join(' ').startsWith('git merge-base --fork-point')) { + return FakeProcess(stdout: fakeShaHash); + } + + // Succeed calling "git fetch upstream main". + if (command.join(' ') == 'git fetch upstream main') { + return FakeProcess(); + } + + // Succeed calling "git diff --name-only --diff-filter=ACMRT fake-sha-hash". + if (command.join(' ') == 'git diff --name-only --diff-filter=ACMRT $fakeShaHash') { + return FakeProcess(stdout: 'file1\nfile2'); + } + + // Otherwise, fail. + return FakeProcessManager.unhandledStart(command); + }, + ), + ); + + try { + final List changedFiles = await fixture.gitRepo.changedFiles; + expect(changedFiles, hasLength(2)); + expect(changedFiles[0].path, endsWith('file1')); + expect(changedFiles[1].path, endsWith('file2')); + } finally { + fixture.gitRepo.root.deleteSync(recursive: true); + } + }); + + test('returns non-deleted files which differ from default merge-base', () async { + final Fixture fixture = Fixture( + processManager: FakeProcessManager( + onStart: (List command) { + if (command.join(' ').startsWith('git merge-base --fork-point')) { + return FakeProcess(exitCode: 1); + } + + if (command.join(' ').startsWith('git merge-base')) { + return FakeProcess(stdout: fakeShaHash); + } + + if (command.join(' ') == 'git fetch upstream main') { + return FakeProcess(); + } + + if (command.join(' ') == 'git diff --name-only --diff-filter=ACMRT $fakeShaHash') { + return FakeProcess(stdout: 'file1\nfile2'); + } + + // Otherwise, fail. + return FakeProcessManager.unhandledStart(command); + }, + ), + ); + + try { + final List changedFiles = await fixture.gitRepo.changedFiles; + expect(changedFiles, hasLength(2)); + expect(changedFiles[0].path, endsWith('file1')); + expect(changedFiles[1].path, endsWith('file2')); + } finally { + fixture.gitRepo.root.deleteSync(recursive: true); + } + }); + + test('returns non-deleted files which differ from HEAD', () async { + final Fixture fixture = Fixture( + processManager: FakeProcessManager( + onStart: (List command) { + if (command.join(' ') == 'git fetch upstream main') { + return FakeProcess(); + } + + if (command.join(' ') == 'git diff-tree --no-commit-id --name-only --diff-filter=ACMRT -r HEAD') { + return FakeProcess(stdout: 'file1\nfile2'); + } + + // Otherwise, fail. + return FakeProcessManager.unhandledStart(command); + }, + ), + ); + + try { + final List changedFiles = await fixture.gitRepo.changedFilesAtHead; + expect(changedFiles, hasLength(2)); + expect(changedFiles[0].path, endsWith('file1')); + expect(changedFiles[1].path, endsWith('file2')); + } finally { + fixture.gitRepo.root.deleteSync(recursive: true); + } + }); + + test('returns non-deleted files which differ from HEAD when merge-base fails', () async { + final Fixture fixture = Fixture( + processManager: FakeProcessManager( + onStart: (List command) { + if (command.join(' ') == 'git fetch upstream main') { + return FakeProcess(); + } + + if (command.join(' ') == 'git diff-tree --no-commit-id --name-only --diff-filter=ACMRT -r HEAD') { + return FakeProcess(stdout: 'file1\nfile2'); + } + + if (command.join(' ').startsWith('git merge-base --fork-point')) { + return FakeProcess(exitCode: 1); + } + + if (command.join(' ').startsWith('git merge-base')) { + return FakeProcess(stdout: fakeShaHash); + } + + // Otherwise, fail. + return FakeProcessManager.unhandledStart(command); + }, + ), + ); + + try { + final List changedFiles = await fixture.gitRepo.changedFilesAtHead; + expect(changedFiles, hasLength(2)); + expect(changedFiles[0].path, endsWith('file1')); + expect(changedFiles[1].path, endsWith('file2')); + } finally { + fixture.gitRepo.root.deleteSync(recursive: true); + } + }); + + test('verbose output is captured', () async { + final Fixture fixture = Fixture( + processManager: FakeProcessManager( + onStart: (List command) { + if (command.join(' ').startsWith('git merge-base --fork-point')) { + return FakeProcess(exitCode: 1); + } + + if (command.join(' ').startsWith('git merge-base')) { + return FakeProcess(stdout: fakeShaHash); + } + + if (command.join(' ') == 'git fetch upstream main') { + return FakeProcess(); + } + + if (command.join(' ') == 'git diff --name-only --diff-filter=ACMRT $fakeShaHash') { + return FakeProcess(stdout: 'file1\nfile2'); + } + + // Otherwise, fail. + return FakeProcessManager.unhandledStart(command); + }, + ), + verbose: true, + ); + + try { + await fixture.gitRepo.changedFiles; + expect(fixture.logSink.toString(), contains('git merge-base --fork-point failed, using default merge-base')); + expect(fixture.logSink.toString(), contains('git diff output:\nfile1\nfile2')); + } finally { + fixture.gitRepo.root.deleteSync(recursive: true); + } + }); +} + +final class Fixture { + factory Fixture({ + FakeProcessManager? processManager, + bool verbose = false, + }) { + final io.Directory root = io.Directory.systemTemp.createTempSync('git_repo_tools.test'); + final StringBuffer logSink = StringBuffer(); + processManager ??= FakeProcessManager(); + return Fixture._( + gitRepo: GitRepo.fromRoot(root, + logSink: logSink, + processManager: processManager, + verbose: verbose, + ), + logSink: logSink, + processManager: processManager, + ); + } + + const Fixture._({ + required this.gitRepo, + required this.logSink, + required this.processManager, + }); + + final GitRepo gitRepo; + final StringBuffer logSink; + final FakeProcessManager processManager; +} diff --git a/tools/pkg/process_fakes/README.md b/tools/pkg/process_fakes/README.md new file mode 100644 index 0000000000000..33da210aea5b0 --- /dev/null +++ b/tools/pkg/process_fakes/README.md @@ -0,0 +1,7 @@ +# `process_fakes` + +Fake implementations of `Process` and `ProcessManager` for testing. + +This is not a great package, and is the bare minimum needed for fairly basic +tooling that uses `ProcessManager`. If we ever need a more complete solution +we should look at upstreaming [`flutter_tools/.../fake_proecss_manager.dart`](https://github.com/flutter/flutter/blob/a9183f696c8e12617d05a26b0b5e80035e515f2a/packages/flutter_tools/test/src/fake_process_manager.dart#L223) diff --git a/tools/clang_tidy/test/process_fakes.dart b/tools/pkg/process_fakes/lib/process_fakes.dart similarity index 91% rename from tools/clang_tidy/test/process_fakes.dart rename to tools/pkg/process_fakes/lib/process_fakes.dart index 7bc5fe5778e1f..356f1d391f33d 100644 --- a/tools/clang_tidy/test/process_fakes.dart +++ b/tools/pkg/process_fakes/lib/process_fakes.dart @@ -9,15 +9,20 @@ import 'package:process/process.dart'; /// A fake implementation of [ProcessManager] that allows control for testing. final class FakeProcessManager implements ProcessManager { + /// Creates a fake process manager delegates to [onRun] and [onStart]. + /// + /// If either is not provided, it throws an [UnsupportedError] when called. FakeProcessManager({ io.ProcessResult Function(List command) onRun = unhandledRun, io.Process Function(List command) onStart = unhandledStart, }) : _onRun = onRun, _onStart = onStart; + /// A default implementation of [onRun] that throws an [UnsupportedError]. static io.ProcessResult unhandledRun(List command) { throw UnsupportedError('Unhandled run: ${command.join(' ')}'); } + /// A default implementation of [onStart] that throws an [UnsupportedError]. static io.Process unhandledStart(List command) { throw UnsupportedError('Unhandled start: ${command.join(' ')}'); } diff --git a/tools/pkg/process_fakes/pubspec.yaml b/tools/pkg/process_fakes/pubspec.yaml new file mode 100644 index 0000000000000..8ac6e85aa88fe --- /dev/null +++ b/tools/pkg/process_fakes/pubspec.yaml @@ -0,0 +1,36 @@ +# Copyright 2013 The Flutter Authors. All rights reserved. +# Use of this source code is governed by a BSD-style license that can be +# found in the LICENSE file. + +name: process_fakes +publish_to: none +environment: + sdk: '>=3.2.0-0 <4.0.0' + +# Do not add any dependencies that require more than what is provided in +# //third_party/pkg, //third_party/dart/pkg, or +# //third_party/dart/third_party/pkg. In particular, package:test is not usable +# here. + +# If you do add packages here, make sure you can run `pub get --offline`, and +# check the .packages and .package_config to make sure all the paths are +# relative to this directory into //third_party/dart + +dependencies: + file: any + meta: any + path: any + platform: any + process: any + +dependency_overrides: + file: + path: ../../../../third_party/pkg/file/packages/file + meta: + path: ../../../../third_party/dart/pkg/meta + path: + path: ../../../../third_party/dart/third_party/pkg/path + platform: + path: ../../../../third_party/pkg/platform + process: + path: ../../../../third_party/pkg/process diff --git a/tools/pub_get_offline.py b/tools/pub_get_offline.py index 17f529c61c805..931db7319b8f9 100644 --- a/tools/pub_get_offline.py +++ b/tools/pub_get_offline.py @@ -44,6 +44,8 @@ os.path.join(ENGINE_DIR, 'tools', 'path_ops', 'dart'), os.path.join(ENGINE_DIR, 'tools', 'pkg', 'engine_build_configs'), os.path.join(ENGINE_DIR, 'tools', 'pkg', 'engine_repo_tools'), + os.path.join(ENGINE_DIR, 'tools', 'pkg', 'git_repo_tools'), + os.path.join(ENGINE_DIR, 'tools', 'pkg', 'process_fakes'), ]