From cb9307db7ecd43bbf6b707fa7123889fddcc993c Mon Sep 17 00:00:00 2001 From: gaaclarke <30870216+gaaclarke@users.noreply.github.com> Date: Fri, 13 Dec 2024 15:28:36 -0800 Subject: [PATCH] Made compilation error colors come through et. (flutter/engine#57174) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fixes https://github.com/flutter/flutter/issues/147931 environment variable documented in github issue: https://github.com/ninja-build/ninja/issues/2196 ## screenshot of results Screenshot 2024-12-13 at 10 18 15 AM ## 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] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [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/master/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/engine/blob/main/docs/testing/Testing-the-engine.md [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/master/docs/contributing/Chat.md --- .../lib/src/build_config_runner.dart | 22 +++++++++++--- .../test/build_config_runner_test.dart | 29 +++++++++++++++++++ 2 files changed, 47 insertions(+), 4 deletions(-) diff --git a/engine/src/flutter/tools/pkg/engine_build_configs/lib/src/build_config_runner.dart b/engine/src/flutter/tools/pkg/engine_build_configs/lib/src/build_config_runner.dart index f96de994260..a4fed2b4359 100644 --- a/engine/src/flutter/tools/pkg/engine_build_configs/lib/src/build_config_runner.dart +++ b/engine/src/flutter/tools/pkg/engine_build_configs/lib/src/build_config_runner.dart @@ -5,7 +5,7 @@ import 'dart:async'; import 'dart:convert'; import 'dart:ffi' as ffi; -import 'dart:io' as io show Directory, File, Process, ProcessResult; +import 'dart:io' as io show Directory, File, Platform, Process, ProcessResult, stdout; import 'dart:math'; import 'package:meta/meta.dart' show visibleForTesting; @@ -565,6 +565,7 @@ final class BuildRunner extends Runner { static final RegExp _gccRegex = RegExp(r'^(.+)(:\d+:\d+:\s+(?:error|note|warning):\s+.*)$'); + static final RegExp _ansiRegex = RegExp(r'\x1b\[[\d;]*m'); /// Converts relative [path], who is relative to [dirPath] to a relative path /// of the `CWD`. @@ -573,15 +574,22 @@ final class BuildRunner extends Runner { return './${p.relative(abs)}'; } + static String _stripAnsi(String input) { + return input.replaceAll(_ansiRegex, ''); + } + /// Takes a [line] from compilation and makes the path relative to `CWD` where /// the paths are relative to [outDir]. @visibleForTesting static String fixGccPaths(String line, String outDir) { - final Match? match = _gccRegex.firstMatch(line); + final sansAnsi = _stripAnsi(line); + final Match? match = _gccRegex.firstMatch(sansAnsi); if (match == null) { return line; } else { - return '${_makeRelative(match.group(1)!, outDir)}${match.group(2)}'; + final String path = match.group(1)!; + final String fixedPath = _makeRelative(match.group(1)!, outDir); + return line.replaceFirst(path, fixedPath); } } @@ -625,10 +633,16 @@ final class BuildRunner extends Runner { if (dryRun) { processResult = _dryRunResult; } else { + final bool shouldEmitAnsi = + (io.stdout.supportsAnsiEscapes && io.Platform.environment['CLICOLOR_FORCE'] != '0') || + io.Platform.environment['CLICOLOR_FORCE'] == '1'; final io.Process process = await processRunner.processManager.start( command, workingDirectory: engineSrcDir.path, - environment: rbeConfig.environment, + environment: { + ...rbeConfig.environment, + if (shouldEmitAnsi) 'CLICOLOR_FORCE': '1', + } ); final List stderrOutput = []; final List stdoutOutput = []; diff --git a/engine/src/flutter/tools/pkg/engine_build_configs/test/build_config_runner_test.dart b/engine/src/flutter/tools/pkg/engine_build_configs/test/build_config_runner_test.dart index a090a85fd25..ef43da8f7dc 100644 --- a/engine/src/flutter/tools/pkg/engine_build_configs/test/build_config_runner_test.dart +++ b/engine/src/flutter/tools/pkg/engine_build_configs/test/build_config_runner_test.dart @@ -539,6 +539,35 @@ void main() { expect(fixed, './$error'); }); + test('fixes gcc paths with ansi colors', () { + final String outDir = path.join(io.Directory.current.path, 'foo', 'bar'); + // An error string with ANSI escape codes for colors. + final List bytes = [ + 27, 91, 49, 109, 46, 46, 47, 46, 46, 47, 102, // + 108, 117, 116, 116, 101, 114, 47, 105, 109, 112, 101, 108, 108, 101, // + 114, 47, 100, 105, 115, 112, 108, 97, 121, 95, 108, 105, 115, 116, 47, // + 100, 108, 95, 100, 105, 115, 112, 97, 116, 99, 104, 101, 114, 46, 99, // + 99, 58, 55, 51, 52, 58, 55, 58, 32, 27, 91, 48, 109, 27, 91, 48, 59, // + 49, 59, 51, 49, 109, 101, 114, 114, 111, 114, 58, 32, 27, 91, 48, 109, // + 27, 91, 49, 109, 117, 115, 101, 32, 111, 102, 32, 117, 110, 100, 101, // + 99, 108, 97, 114, 101, 100, 32, 105, 100, 101, 110, 116, 105, 102, 105, // + 101, 114, 32, 39, 114, 111, 99, 107, 101, 116, 39, 27, 91, 48, 109, + ]; + final String error = convert.utf8.decode(bytes); + final String fixed = BuildRunner.fixGccPaths(error, outDir); + expect( + fixed.contains('../../flutter/impeller/display_list/dl_dispatcher.cc'), + isFalse, + reason: 'Fixed string: $fixed', + ); + expect( + fixed.contains('./flutter/impeller/display_list/dl_dispatcher.cc'), + isTrue, + reason: 'Fixed string: $fixed', + ); + expect(fixed[0], '\x1B', reason: 'Fixed string: $fixed'); + }); + test('GlobalBuildRunner skips generators when runGenerators is false', () async { final Build targetBuild = buildConfig.builds[0];