From eb60ab38deeda6975c9b0fef883978f2a9f69120 Mon Sep 17 00:00:00 2001 From: Scott Lee Date: Thu, 14 Aug 2025 14:18:36 -0700 Subject: [PATCH] Revert "presubmit: emit errors instead of warnings for bad copyright headers" This reverts commit fa62515ecbf96cab7fe0cc93a5cbf2990bbc73f5. Reason for revert: it seems that the existing files without valid copyright headers are causing linux-presubmit builds to fail. b/438791294 Bug: 435696543,40237859 Original change's description: > presubmit: emit errors instead of warnings for bad copyright headers > > This is an effective revert of https://crrev.com/c/4895337 with > additional patches to support a footer. > > https://crrev.com/c/3887721 updated CheckLicense() to emit errors > for bad copyright headers. However, https://crrev.com/c/4895337 > was changed the finding type from error to warning, claiming that > the check is N/A for moved third files, but it's not so easy > to programatiically distinguish moved third-party files. > > After discussions, it was decided to change the finding type back > to error to prevent accidental submissions for new files without > correct CopyRight headers. > > To mitigate moved, third-party files, this CL adds support for > "Bypass-Check-License: " footer. > > If the check should be ignored in a given CL, CL authors should > use the footer instead. > > Bug: 435696543,40237859 > Change-Id: I177915c65932a3d76ea60ee6a0e396f726bc400d > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6842238 > Reviewed-by: Gavin Mak > Commit-Queue: Scott Lee Bug: 435696543,40237859 No-Presubmit: true No-Tree-Checks: true No-Try: true Change-Id: Ibedf8d13e3742249947e29e625a14cceaf89879c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6852377 Commit-Queue: Scott Lee Bot-Commit: Rubber Stamper Reviewed-by: Gavin Mak --- presubmit_canned_checks.py | 44 ++++++++++------------------ tests/presubmit_unittest.py | 58 +++++-------------------------------- 2 files changed, 23 insertions(+), 79 deletions(-) diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index a53fab56be..0fd69e6679 100644 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -740,8 +740,6 @@ def CheckLongLines(input_api, output_api, maxlen, source_file_filter=None): return [] -_BYPASS_CHECK_LICENSE_FOOTER = 'Bypass-Check-License' - def CheckLicense(input_api, output_api, license_re_param=None, @@ -749,20 +747,6 @@ def CheckLicense(input_api, source_file_filter=None, accept_empty_files=True): """Verifies the license header.""" - # The CL is ignoring the license check - reasons = input_api.change.GitFootersFromDescription().get( - _BYPASS_CHECK_LICENSE_FOOTER, []) - - if len(reasons): - if ''.join(reasons).strip() == '': - return [ - output_api.PresubmitError( - '{key} is specified without the reason. Please provide the reason ' - 'in "{key}: "'.format( - key=_BYPASS_CHECK_LICENSE_FOOTER)) - ] - input_api.logging.info('License check is being ignored') - return [] # Early-out if the license_re is guaranteed to match everything. if license_re_param and license_re_param == '.*': @@ -843,9 +827,11 @@ def CheckLicense(input_api, bad_files.append(f.LocalPath()) results = [] if bad_new_files: + # We can't distinguish between Google and thirty-party files, so this has to be a + # warning rather than an error. if license_re_param: - error_message = ('License on new files must match:\n\n%s\n' % - license_re_param) + warning_message = ('License on new files must match:\n\n%s\n' % + license_re_param) else: # Verbatim text that can be copy-pasted into new files (possibly # adjusting the leading comment delimiter). @@ -857,28 +843,28 @@ def CheckLicense(input_api, 'project': project_name, 'key_line': key_line, } - error_message = ( + warning_message = ( 'License on new files must be:\n\n%s\n' % new_license_text + '(adjusting the comment delimiter accordingly).\n\n' + 'If this is a moved file, then update the license but do not ' + 'update the year.\n\n' + - 'If this is a third-party file, then add a footer ' + - 'with "{key}: " to skip this check.'.format( - key=_BYPASS_CHECK_LICENSE_FOOTER)) - error_message += 'Found a bad license header in these new or moved files:' + 'If this is a third-party file then ignore this warning.\n\n') + warning_message += 'Found a bad license header in these new or moved files:' results.append( - output_api.PresubmitError(error_message, items=bad_new_files)) + output_api.PresubmitPromptWarning(warning_message, + items=bad_new_files)) if wrong_year_new_files: + # We can't distinguish between new and moved files, so this has to be a + # warning rather than an error. results.append( - output_api.PresubmitError( + output_api.PresubmitPromptWarning( 'License doesn\'t list the current year. If this is a new file, ' - 'use the current year. If this is a moved file, then add ' - 'a footer with "{key}: " to skip this check.'.format( - key=_BYPASS_CHECK_LICENSE_FOOTER), + 'use the current year. If this is a moved file then ignore this ' + 'warning.', items=wrong_year_new_files)) if bad_files: results.append( - output_api.PresubmitError( + output_api.PresubmitPromptWarning( 'License must match:\n%s\n' % license_re.pattern + 'Found a bad license header in these files:', items=bad_files)) diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 05e0b6bcd5..6359031721 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -2689,17 +2689,9 @@ the current line as well! committing, expected_result, new_file=False, - description='this is description', **kwargs): - change = presubmit.Change( - 'author', - description, - '/path/root', - None, - 0, - 0, - None, - ) + change = mock.MagicMock(presubmit.GitChange) + change.scm = 'svn' input_api = self.MockInputApi(change, committing) affected_file = mock.MagicMock(presubmit.GitAffectedFile) if new_file: @@ -2747,41 +2739,7 @@ the current line as well! license_text = (r".*? Copyright \(c\) 0007 Nobody.\n" r".*? All Rights Reserved\.\n") self._LicenseCheck(text, license_text, True, - presubmit.OutputApi.PresubmitError) - - def testBypassCheckLicense(self): - text = '\n'.join([ - '#!/bin/python', - '# this is not a CopyRight.', - '# No Rights Reserved.', - ]) - description = '\n'.join([ - 'this is a change', - '', - '', - 'Bypass-Check-License: a third-party', - ]) - self._LicenseCheck(text, None, True, None, description=description) - - def testBypassCheckLicenseWithoutReason(self): - text = '\n'.join([ - '#!/bin/python', - '# this is not a CopyRight.', - '# No Rights Reserved.', - ]) - description = '\n'.join([ - 'this is a change', - '', - '', - 'Bypass-Check-License: ', - ]) - self._LicenseCheck( - text, - None, - True, - # must error out that should be given. - presubmit.OutputApi.PresubmitError, - description=description) + presubmit.OutputApi.PresubmitPromptWarning) def testCheckLicenseFailUpload(self): text = ("#!/bin/python\n" @@ -2791,7 +2749,7 @@ the current line as well! license_text = (r".*? Copyright \(c\) 0007 Nobody.\n" r".*? All Rights Reserved\.\n") self._LicenseCheck(text, license_text, False, - presubmit.OutputApi.PresubmitError) + presubmit.OutputApi.PresubmitPromptWarning) def testCheckLicenseEmptySuccess(self): text = '' @@ -2822,7 +2780,7 @@ the current line as well! self._LicenseCheck(text, license_text, False, - presubmit.OutputApi.PresubmitError, + presubmit.OutputApi.PresubmitPromptWarning, new_file=True) def _GetLicenseText(self, current_year): @@ -2834,15 +2792,15 @@ the current line as well! "# found in the LICENSE file.\n" "print('foo')\n" % current_year) - def testCheckLicenseNewFileError(self): - # Check that we error on new files with wrong year. Test with first + def testCheckLicenseNewFileWarn(self): + # Check that we warn on new files with wrong year. Test with first # allowed year. text = self._GetLicenseText(2006) license_text = None self._LicenseCheck(text, license_text, False, - presubmit.OutputApi.PresubmitError, + presubmit.OutputApi.PresubmitPromptWarning, new_file=True) def testCheckLicenseNewCSSFilePass(self):