From 4f7d9503691d7f76ff457f3b1088ae2f22de686c Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Tue, 13 Jul 2021 13:29:07 -0400 Subject: [PATCH] [flutter_plugin_tools] Improve license-check output Currently each type of check handles its output in isolation, which creates confusing output when the last check succeeds but an earlier check fails, since the end of the output will just be a success message. This makes the output follow the same basic approach as the package looper commands, where all failures are collected, and then a final summary is presented at the end, so the last message will always reflect the important details. It also adopts the colorized output now used by most other commands. --- script/tool/CHANGELOG.md | 4 + .../tool/lib/src/license_check_command.dart | 122 ++++++++------- .../tool/test/license_check_command_test.dart | 147 +++++++++++++----- 3 files changed, 175 insertions(+), 98 deletions(-) diff --git a/script/tool/CHANGELOG.md b/script/tool/CHANGELOG.md index 9db94dda37da..17b28927538d 100644 --- a/script/tool/CHANGELOG.md +++ b/script/tool/CHANGELOG.md @@ -1,3 +1,7 @@ +## NEXT + +- Improved `license-check` output. + ## 0.4.0 - Modified the output format of many commands diff --git a/script/tool/lib/src/license_check_command.dart b/script/tool/lib/src/license_check_command.dart index 093f8143df4f..e68585c44bdf 100644 --- a/script/tool/lib/src/license_check_command.dart +++ b/script/tool/lib/src/license_check_command.dart @@ -107,21 +107,65 @@ class LicenseCheckCommand extends PluginCommand { @override Future run() async { - final Iterable codeFiles = (await _getAllFiles()).where((File file) => + final Iterable allFiles = await _getAllFiles(); + + final Iterable codeFiles = allFiles.where((File file) => _codeFileExtensions.contains(p.extension(file.path)) && !_shouldIgnoreFile(file)); - final Iterable firstPartyLicenseFiles = (await _getAllFiles()).where( - (File file) => - path.basename(file.basename) == 'LICENSE' && !_isThirdParty(file)); + final Iterable firstPartyLicenseFiles = allFiles.where((File file) => + path.basename(file.basename) == 'LICENSE' && !_isThirdParty(file)); - final bool copyrightCheckSucceeded = await _checkCodeLicenses(codeFiles); - print('\n=======================================\n'); - final bool licenseCheckSucceeded = + final List licenseFileFailures = await _checkLicenseFiles(firstPartyLicenseFiles); + final Map<_LicenseFailureType, List> codeFileFailures = + await _checkCodeLicenses(codeFiles); + + bool passed = true; + + print('\n=======================================\n'); + + if (licenseFileFailures.isNotEmpty) { + passed = false; + printError( + 'The following LICENSE files do not follow the expected format:'); + for (final File file in licenseFileFailures) { + printError(' ${file.path}'); + } + printError('Please ensure that they use the exact format used in this ' + 'repository".\n'); + } + + if (codeFileFailures[_LicenseFailureType.incorrectFirstParty]!.isNotEmpty) { + passed = false; + printError('The license block for these files is missing or incorrect:'); + for (final File file + in codeFileFailures[_LicenseFailureType.incorrectFirstParty]!) { + printError(' ${file.path}'); + } + printError( + 'If this third-party code, move it to a "third_party/" directory, ' + 'otherwise ensure that you are using the exact copyright and license ' + 'text used by all first-party files in this repository.\n'); + } + + if (codeFileFailures[_LicenseFailureType.unknownThirdParty]!.isNotEmpty) { + passed = false; + printError( + 'No recognized license was found for the following third-party files:'); + for (final File file + in codeFileFailures[_LicenseFailureType.unknownThirdParty]!) { + printError(' ${file.path}'); + } + print('Please check that they have a license at the top of the file. ' + 'If they do, the license check needs to be updated to recognize ' + 'the new third-party license block.\n'); + } - if (!copyrightCheckSucceeded || !licenseCheckSucceeded) { + if (!passed) { throw ToolExit(1); } + + printSuccess('All files passed validation!'); } // Creates the expected copyright+license block for first-party code. @@ -135,9 +179,10 @@ class LicenseCheckCommand extends PluginCommand { '${comment}found in the LICENSE file.$suffix\n'; } - // Checks all license blocks for [codeFiles], returning false if any of them - // fail validation. - Future _checkCodeLicenses(Iterable codeFiles) async { + /// Checks all license blocks for [codeFiles], returning any that fail + /// validation. + Future>> _checkCodeLicenses( + Iterable codeFiles) async { final List incorrectFirstPartyFiles = []; final List unrecognizedThirdPartyFiles = []; @@ -171,7 +216,6 @@ class LicenseCheckCommand extends PluginCommand { } } } - print('\n'); // Sort by path for more usable output. final int Function(File, File) pathCompare = @@ -179,38 +223,14 @@ class LicenseCheckCommand extends PluginCommand { incorrectFirstPartyFiles.sort(pathCompare); unrecognizedThirdPartyFiles.sort(pathCompare); - if (incorrectFirstPartyFiles.isNotEmpty) { - print('The license block for these files is missing or incorrect:'); - for (final File file in incorrectFirstPartyFiles) { - print(' ${file.path}'); - } - print('If this third-party code, move it to a "third_party/" directory, ' - 'otherwise ensure that you are using the exact copyright and license ' - 'text used by all first-party files in this repository.\n'); - } - - if (unrecognizedThirdPartyFiles.isNotEmpty) { - print( - 'No recognized license was found for the following third-party files:'); - for (final File file in unrecognizedThirdPartyFiles) { - print(' ${file.path}'); - } - print('Please check that they have a license at the top of the file. ' - 'If they do, the license check needs to be updated to recognize ' - 'the new third-party license block.\n'); - } - - final bool succeeded = - incorrectFirstPartyFiles.isEmpty && unrecognizedThirdPartyFiles.isEmpty; - if (succeeded) { - print('All source files passed validation!'); - } - return succeeded; + return <_LicenseFailureType, List>{ + _LicenseFailureType.incorrectFirstParty: incorrectFirstPartyFiles, + _LicenseFailureType.unknownThirdParty: unrecognizedThirdPartyFiles, + }; } - // Checks all provide LICENSE files, returning false if any of them - // fail validation. - Future _checkLicenseFiles(Iterable files) async { + /// Checks all provided LICENSE [files], returning any that fail validation. + Future> _checkLicenseFiles(Iterable files) async { final List incorrectLicenseFiles = []; for (final File file in files) { @@ -219,22 +239,8 @@ class LicenseCheckCommand extends PluginCommand { incorrectLicenseFiles.add(file); } } - print('\n'); - if (incorrectLicenseFiles.isNotEmpty) { - print('The following LICENSE files do not follow the expected format:'); - for (final File file in incorrectLicenseFiles) { - print(' ${file.path}'); - } - print( - 'Please ensure that they use the exact format used in this repository".\n'); - } - - final bool succeeded = incorrectLicenseFiles.isEmpty; - if (succeeded) { - print('All LICENSE files passed validation!'); - } - return succeeded; + return incorrectLicenseFiles; } bool _shouldIgnoreFile(File file) { @@ -255,3 +261,5 @@ class LicenseCheckCommand extends PluginCommand { .map((FileSystemEntity file) => file as File) .toList(); } + +enum _LicenseFailureType { incorrectFirstParty, unknownThirdParty } diff --git a/script/tool/test/license_check_command_test.dart b/script/tool/test/license_check_command_test.dart index 64adc9214d80..288cf4696a59 100644 --- a/script/tool/test/license_check_command_test.dart +++ b/script/tool/test/license_check_command_test.dart @@ -131,8 +131,12 @@ void main() { await runCapturingPrint(runner, ['license-check']); // Sanity check that the test did actually check a file. - expect(output, contains('Checking checked.cc')); - expect(output, contains('All source files passed validation!')); + expect( + output, + containsAllInOrder([ + contains('Checking checked.cc'), + contains('All files passed validation!'), + ])); }); test('handles the comment styles for all supported languages', () async { @@ -150,10 +154,14 @@ void main() { await runCapturingPrint(runner, ['license-check']); // Sanity check that the test did actually check the files. - expect(output, contains('Checking file_a.cc')); - expect(output, contains('Checking file_b.sh')); - expect(output, contains('Checking file_c.html')); - expect(output, contains('All source files passed validation!')); + expect( + output, + containsAllInOrder([ + contains('Checking file_a.cc'), + contains('Checking file_b.sh'), + contains('Checking file_c.html'), + contains('All files passed validation!'), + ])); }); test('fails if any checked files are missing license blocks', () async { @@ -176,12 +184,14 @@ void main() { // Failure should give information about the problematic files. expect( output, - contains( - 'The license block for these files is missing or incorrect:')); - expect(output, contains(' bad.cc')); - expect(output, contains(' bad.h')); + containsAllInOrder([ + contains( + 'The license block for these files is missing or incorrect:'), + contains(' bad.cc'), + contains(' bad.h'), + ])); // Failure shouldn't print the success message. - expect(output, isNot(contains('All source files passed validation!'))); + expect(output, isNot(contains(contains('All files passed validation!')))); }); test('fails if any checked files are missing just the copyright', () async { @@ -202,11 +212,13 @@ void main() { // Failure should give information about the problematic files. expect( output, - contains( - 'The license block for these files is missing or incorrect:')); - expect(output, contains(' bad.cc')); + containsAllInOrder([ + contains( + 'The license block for these files is missing or incorrect:'), + contains(' bad.cc'), + ])); // Failure shouldn't print the success message. - expect(output, isNot(contains('All source files passed validation!'))); + expect(output, isNot(contains(contains('All files passed validation!')))); }); test('fails if any checked files are missing just the license', () async { @@ -227,11 +239,13 @@ void main() { // Failure should give information about the problematic files. expect( output, - contains( - 'The license block for these files is missing or incorrect:')); - expect(output, contains(' bad.cc')); + containsAllInOrder([ + contains( + 'The license block for these files is missing or incorrect:'), + contains(' bad.cc'), + ])); // Failure shouldn't print the success message. - expect(output, isNot(contains('All source files passed validation!'))); + expect(output, isNot(contains(contains('All files passed validation!')))); }); test('fails if any third-party code is not in a third_party directory', @@ -250,11 +264,13 @@ void main() { // Failure should give information about the problematic files. expect( output, - contains( - 'The license block for these files is missing or incorrect:')); - expect(output, contains(' third_party.cc')); + containsAllInOrder([ + contains( + 'The license block for these files is missing or incorrect:'), + contains(' third_party.cc'), + ])); // Failure shouldn't print the success message. - expect(output, isNot(contains('All source files passed validation!'))); + expect(output, isNot(contains(contains('All files passed validation!')))); }); test('succeeds for third-party code in a third_party directory', () async { @@ -276,8 +292,12 @@ void main() { await runCapturingPrint(runner, ['license-check']); // Sanity check that the test did actually check the file. - expect(output, contains('Checking a_plugin/lib/src/third_party/file.cc')); - expect(output, contains('All source files passed validation!')); + expect( + output, + containsAllInOrder([ + contains('Checking a_plugin/lib/src/third_party/file.cc'), + contains('All files passed validation!'), + ])); }); test('allows first-party code in a third_party directory', () async { @@ -294,9 +314,12 @@ void main() { await runCapturingPrint(runner, ['license-check']); // Sanity check that the test did actually check the file. - expect(output, - contains('Checking a_plugin/lib/src/third_party/first_party.cc')); - expect(output, contains('All source files passed validation!')); + expect( + output, + containsAllInOrder([ + contains('Checking a_plugin/lib/src/third_party/first_party.cc'), + contains('All files passed validation!'), + ])); }); test('fails for licenses that the tool does not expect', () async { @@ -320,11 +343,13 @@ void main() { // Failure should give information about the problematic files. expect( output, - contains( - 'No recognized license was found for the following third-party files:')); - expect(output, contains(' third_party/bad.cc')); + containsAllInOrder([ + contains( + 'No recognized license was found for the following third-party files:'), + contains(' third_party/bad.cc'), + ])); // Failure shouldn't print the success message. - expect(output, isNot(contains('All source files passed validation!'))); + expect(output, isNot(contains(contains('All files passed validation!')))); }); test('Apache is not recognized for new authors without validation changes', @@ -353,11 +378,13 @@ void main() { // Failure should give information about the problematic files. expect( output, - contains( - 'No recognized license was found for the following third-party files:')); - expect(output, contains(' third_party/bad.cc')); + containsAllInOrder([ + contains( + 'No recognized license was found for the following third-party files:'), + contains(' third_party/bad.cc'), + ])); // Failure shouldn't print the success message. - expect(output, isNot(contains('All source files passed validation!'))); + expect(output, isNot(contains(contains('All files passed validation!')))); }); test('passes if all first-party LICENSE files are correctly formatted', @@ -370,8 +397,12 @@ void main() { await runCapturingPrint(runner, ['license-check']); // Sanity check that the test did actually check the file. - expect(output, contains('Checking LICENSE')); - expect(output, contains('All LICENSE files passed validation!')); + expect( + output, + containsAllInOrder([ + contains('Checking LICENSE'), + contains('All files passed validation!'), + ])); }); test('fails if any first-party LICENSE files are incorrectly formatted', @@ -387,7 +418,7 @@ void main() { }); expect(commandError, isA()); - expect(output, isNot(contains('All LICENSE files passed validation!'))); + expect(output, isNot(contains(contains('All files passed validation!')))); }); test('ignores third-party LICENSE format', () async { @@ -400,8 +431,42 @@ void main() { await runCapturingPrint(runner, ['license-check']); // The file shouldn't be checked. - expect(output, isNot(contains('Checking third_party/LICENSE'))); - expect(output, contains('All LICENSE files passed validation!')); + expect(output, isNot(contains(contains('Checking third_party/LICENSE')))); + }); + + test('outputs all errors at the end', () async { + root.childFile('bad.cc').createSync(); + root + .childDirectory('third_party') + .childFile('bad.cc') + .createSync(recursive: true); + final File license = root.childFile('LICENSE'); + license.createSync(); + license.writeAsStringSync(_incorrectLicenseFileText); + + Error? commandError; + final List output = await runCapturingPrint( + runner, ['license-check'], errorHandler: (Error e) { + commandError = e; + }); + + expect(commandError, isA()); + expect( + output, + containsAllInOrder([ + contains('Checking LICENSE'), + contains('Checking bad.cc'), + contains('Checking third_party/bad.cc'), + contains( + 'The following LICENSE files do not follow the expected format:'), + contains(' LICENSE'), + contains( + 'The license block for these files is missing or incorrect:'), + contains(' bad.cc'), + contains( + 'No recognized license was found for the following third-party files:'), + contains(' third_party/bad.cc'), + ])); }); }); }