Add presubmit check for correct sha1 format in .png.sha1 files.
Benmason@ explained that a recent submission of a png file as png.sha1 file resulted in all images on that day not to be uploaded to TC. 2nd attempt after https://crrev.com/c/4374003 was reverted due to "Evaluation of CheckStrings failed: Access outside the repository root is denied." This is now using input_api.AffectedFiles().NewContents instead of opening the file directly. Bug: 1428712 Change-Id: Ia1bcb5b132f5d92ce7378a986be74b9958175680 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4432832 Reviewed-by: Ben Mason <benmason@chromium.org> Reviewed-by: Dominic Battre <battre@chromium.org> Commit-Queue: Ben Mason <benmason@chromium.org> Cr-Commit-Position: refs/heads/main@{#1142068}
This commit is contained in:
Jens Mueller
committed by
Chromium LUCI CQ
Chromium LUCI CQ
parent
5508f6d0df
commit
054652c1cb
33
PRESUBMIT.py
33
PRESUBMIT.py
@ -6050,6 +6050,7 @@ def CheckStrings(input_api, output_api):
|
||||
# - If the CL contains removed messages in grd files but the corresponding
|
||||
# .sha1 files aren't removed, warn the developer to remove them.
|
||||
unnecessary_screenshots = []
|
||||
invalid_sha1 = []
|
||||
missing_sha1 = []
|
||||
missing_sha1_modified = []
|
||||
unnecessary_sha1_files = []
|
||||
@ -6061,18 +6062,33 @@ def CheckStrings(input_api, output_api):
|
||||
# break message extraction for translation, hence would block Chromium
|
||||
# translations until they are fixed.
|
||||
icu_syntax_errors = []
|
||||
sha1_pattern = input_api.re.compile(r'^[a-fA-F0-9]{40}$',
|
||||
input_api.re.MULTILINE)
|
||||
|
||||
def _CheckScreenshotAdded(screenshots_dir, message_id):
|
||||
sha1_path = input_api.os_path.join(screenshots_dir,
|
||||
message_id + '.png.sha1')
|
||||
if sha1_path not in new_or_added_paths:
|
||||
missing_sha1.append(sha1_path)
|
||||
elif not _CheckValidSha1(sha1_path):
|
||||
invalid_sha1.append(sha1_path)
|
||||
|
||||
def _CheckScreenshotModified(screenshots_dir, message_id):
|
||||
sha1_path = input_api.os_path.join(screenshots_dir,
|
||||
message_id + '.png.sha1')
|
||||
if sha1_path not in new_or_added_paths:
|
||||
missing_sha1_modified.append(sha1_path)
|
||||
elif not _CheckValidSha1(sha1_path):
|
||||
invalid_sha1.append(sha1_path)
|
||||
|
||||
def _CheckValidSha1(sha1_path):
|
||||
return sha1_pattern.search(
|
||||
next(
|
||||
"\n".join(f.NewContents())
|
||||
for f in input_api.AffectedFiles()
|
||||
if f.LocalPath() == sha1_path
|
||||
)
|
||||
)
|
||||
|
||||
def _CheckScreenshotRemoved(screenshots_dir, message_id):
|
||||
sha1_path = input_api.os_path.join(screenshots_dir,
|
||||
@ -6270,14 +6286,8 @@ def CheckStrings(input_api, output_api):
|
||||
modified_ids.add(key)
|
||||
elif old_id_to_msg_map[key].attrs['meaning'] != \
|
||||
new_id_to_msg_map[key].attrs['meaning']:
|
||||
# The message meaning changed. Ensure there is a screenshot for it.
|
||||
sha1_path = input_api.os_path.join(screenshots_dir,
|
||||
key + '.png.sha1')
|
||||
if sha1_path not in new_or_added_paths and not \
|
||||
input_api.os_path.exists(sha1_path):
|
||||
# There is neither a previous screenshot nor is a new one added now.
|
||||
# Require a screenshot.
|
||||
modified_ids.add(key)
|
||||
# The message meaning changed. We later check for a screenshot.
|
||||
modified_ids.add(key)
|
||||
|
||||
if run_screenshot_check:
|
||||
# Check the screenshot directory for .png files. Warn if there is any.
|
||||
@ -6318,6 +6328,13 @@ def CheckStrings(input_api, output_api):
|
||||
'(https://g.co/chrome/translation) and add these files to your '
|
||||
'changelist:', sorted(missing_sha1)))
|
||||
|
||||
if invalid_sha1:
|
||||
results.append(
|
||||
output_api.PresubmitError(
|
||||
'The following files do not seem to contain valid sha1 hashes. '
|
||||
'Make sure they contain hashes created by '
|
||||
'tools/translate/upload_screenshots.py:', sorted(invalid_sha1)))
|
||||
|
||||
if missing_sha1_modified:
|
||||
results.append(
|
||||
output_api.PresubmitError(
|
||||
|
||||
@ -3467,6 +3467,7 @@ class StringTest(unittest.TestCase):
|
||||
'</message>',
|
||||
'</grit-part>')
|
||||
|
||||
VALID_SHA1 = ('0000000000000000000000000000000000000000',)
|
||||
DO_NOT_UPLOAD_PNG_MESSAGE = ('Do not include actual screenshots in the '
|
||||
'changelist. Run '
|
||||
'tools/translate/upload_screenshots.py to '
|
||||
@ -3481,6 +3482,9 @@ class StringTest(unittest.TestCase):
|
||||
ICU_SYNTAX_ERROR_MESSAGE = ('ICU syntax errors were found in the following '
|
||||
'strings (problems or feedback? Contact '
|
||||
'rainhard@chromium.org):')
|
||||
SHA1_FORMAT_MESSAGE = ('The following files do not seem to contain valid sha1 '
|
||||
'hashes. Make sure they contain hashes created by '
|
||||
'tools/translate/upload_screenshots.py:')
|
||||
|
||||
def makeInputApi(self, files):
|
||||
input_api = MockInputApi()
|
||||
@ -3549,7 +3553,7 @@ class StringTest(unittest.TestCase):
|
||||
MockAffectedFile('part.grdp', self.NEW_GRDP_CONTENTS3,
|
||||
self.NEW_GRDP_CONTENTS4, action='M'),
|
||||
MockFile(os.path.join('part_grdp', 'IDS_PART_TEST1.png.sha1'),
|
||||
'binary', action='A')])
|
||||
self.VALID_SHA1, action='A')])
|
||||
warnings = PRESUBMIT.CheckStrings(input_api, MockOutputApi())
|
||||
self.assertEqual(0, len(warnings))
|
||||
|
||||
@ -3568,10 +3572,20 @@ class StringTest(unittest.TestCase):
|
||||
MockAffectedFile('part.grdp', self.NEW_GRDP_CONTENTS5,
|
||||
self.NEW_GRDP_CONTENTS6, action='M'),
|
||||
MockFile(os.path.join('part_grdp', 'IDS_PART_TEST1.png.sha1'),
|
||||
'binary', action='A')])
|
||||
self.VALID_SHA1, action='A')])
|
||||
warnings = PRESUBMIT.CheckStrings(input_api, MockOutputApi())
|
||||
self.assertEqual(0, len(warnings))
|
||||
|
||||
def testModifiedIntroducedInvalidSha1(self):
|
||||
# CL modified a message and the sha1 file changed to invalid
|
||||
input_api = self.makeInputApi([
|
||||
MockAffectedFile('part.grdp', self.NEW_GRDP_CONTENTS5,
|
||||
self.NEW_GRDP_CONTENTS6, action='M'),
|
||||
MockAffectedFile(os.path.join('part_grdp', 'IDS_PART_TEST1.png.sha1'),
|
||||
('some invalid sha1',), self.VALID_SHA1, action='M')])
|
||||
warnings = PRESUBMIT.CheckStrings(input_api, MockOutputApi())
|
||||
self.assertEqual(1, len(warnings))
|
||||
|
||||
def testPngAddedSha1NotAdded(self):
|
||||
# CL added one new message in a grd file and added the png file associated
|
||||
# with it, but did not add the corresponding sha1 file. This should warn
|
||||
@ -3637,6 +3651,7 @@ class StringTest(unittest.TestCase):
|
||||
os.path.join('test_grd', 'IDS_TEST1.png.sha1')],
|
||||
warnings[1].items)
|
||||
|
||||
|
||||
def testScreenshotsWithSha1(self):
|
||||
# CL added four messages (two each in a grd and grdp) and their
|
||||
# corresponding .sha1 files. No warnings.
|
||||
@ -3655,24 +3670,64 @@ class StringTest(unittest.TestCase):
|
||||
# Added files:
|
||||
MockFile(
|
||||
os.path.join('test_grd', 'IDS_TEST1.png.sha1'),
|
||||
'binary',
|
||||
self.VALID_SHA1,
|
||||
action='A'),
|
||||
MockFile(
|
||||
os.path.join('test_grd', 'IDS_TEST2.png.sha1'),
|
||||
'binary',
|
||||
('0000000000000000000000000000000000000000', ''),
|
||||
action='A'),
|
||||
MockFile(
|
||||
os.path.join('part_grdp', 'IDS_PART_TEST1.png.sha1'),
|
||||
'binary',
|
||||
self.VALID_SHA1,
|
||||
action='A'),
|
||||
MockFile(
|
||||
os.path.join('part_grdp', 'IDS_PART_TEST2.png.sha1'),
|
||||
'binary',
|
||||
self.VALID_SHA1,
|
||||
action='A'),
|
||||
])
|
||||
warnings = PRESUBMIT.CheckStrings(input_api, MockOutputApi())
|
||||
self.assertEqual([], warnings)
|
||||
|
||||
|
||||
def testScreenshotsWithInvalidSha1(self):
|
||||
input_api = self.makeInputApi([
|
||||
# Modified files:
|
||||
MockAffectedFile(
|
||||
'test.grd',
|
||||
self.NEW_GRD_CONTENTS2,
|
||||
self.OLD_GRD_CONTENTS,
|
||||
action='M'),
|
||||
MockAffectedFile(
|
||||
'part.grdp',
|
||||
self.NEW_GRDP_CONTENTS2,
|
||||
self.OLD_GRDP_CONTENTS,
|
||||
action='M'),
|
||||
# Added files:
|
||||
MockFile(
|
||||
os.path.join('test_grd', 'IDS_TEST1.png.sha1'),
|
||||
self.VALID_SHA1,
|
||||
action='A'),
|
||||
MockFile(
|
||||
os.path.join('test_grd', 'IDS_TEST2.png.sha1'),
|
||||
('‰PNG', 'test'),
|
||||
action='A'),
|
||||
MockFile(
|
||||
os.path.join('part_grdp', 'IDS_PART_TEST1.png.sha1'),
|
||||
self.VALID_SHA1,
|
||||
action='A'),
|
||||
MockFile(
|
||||
os.path.join('part_grdp', 'IDS_PART_TEST2.png.sha1'),
|
||||
self.VALID_SHA1,
|
||||
action='A'),
|
||||
])
|
||||
warnings = PRESUBMIT.CheckStrings(input_api, MockOutputApi())
|
||||
self.assertEqual(1, len(warnings))
|
||||
self.assertEqual('error', warnings[0].type)
|
||||
self.assertEqual(self.SHA1_FORMAT_MESSAGE, warnings[0].message)
|
||||
self.assertEqual([os.path.join('test_grd', 'IDS_TEST2.png.sha1')],
|
||||
warnings[0].items)
|
||||
|
||||
|
||||
def testScreenshotsRemovedWithSha1(self):
|
||||
# Replace new contents with old contents in grd and grp files, removing
|
||||
# IDS_TEST1, IDS_TEST2, IDS_PART_TEST1 and IDS_PART_TEST2.
|
||||
@ -3690,12 +3745,14 @@ class StringTest(unittest.TestCase):
|
||||
self.NEW_GRDP_CONTENTS2, # old_contents
|
||||
action='M'),
|
||||
# Unmodified files:
|
||||
MockFile(os.path.join('test_grd', 'IDS_TEST1.png.sha1'), 'binary', ''),
|
||||
MockFile(os.path.join('test_grd', 'IDS_TEST2.png.sha1'), 'binary', ''),
|
||||
MockFile(os.path.join('test_grd', 'IDS_TEST1.png.sha1'),
|
||||
self.VALID_SHA1, ''),
|
||||
MockFile(os.path.join('test_grd', 'IDS_TEST2.png.sha1'),
|
||||
self.VALID_SHA1, ''),
|
||||
MockFile(os.path.join('part_grdp', 'IDS_PART_TEST1.png.sha1'),
|
||||
'binary', ''),
|
||||
self.VALID_SHA1, ''),
|
||||
MockFile(os.path.join('part_grdp', 'IDS_PART_TEST2.png.sha1'),
|
||||
'binary', '')
|
||||
self.VALID_SHA1, '')
|
||||
])
|
||||
warnings = PRESUBMIT.CheckStrings(input_api, MockOutputApi())
|
||||
self.assertEqual(1, len(warnings))
|
||||
@ -3722,9 +3779,10 @@ class StringTest(unittest.TestCase):
|
||||
self.NEW_GRDP_CONTENTS2, # old_contents
|
||||
action='M'),
|
||||
# Unmodified files:
|
||||
MockFile(os.path.join('test_grd', 'IDS_TEST1.png.sha1'), 'binary', ''),
|
||||
MockFile(os.path.join('test_grd', 'IDS_TEST1.png.sha1'),
|
||||
self.VALID_SHA1, ''),
|
||||
MockFile(os.path.join('part_grdp', 'IDS_PART_TEST1.png.sha1'),
|
||||
'binary', ''),
|
||||
self.VALID_SHA1, ''),
|
||||
# Deleted files:
|
||||
MockAffectedFile(
|
||||
os.path.join('test_grd', 'IDS_TEST2.png.sha1'),
|
||||
@ -3761,19 +3819,19 @@ class StringTest(unittest.TestCase):
|
||||
# Deleted files:
|
||||
MockFile(
|
||||
os.path.join('test_grd', 'IDS_TEST1.png.sha1'),
|
||||
'binary',
|
||||
self.VALID_SHA1,
|
||||
action='D'),
|
||||
MockFile(
|
||||
os.path.join('test_grd', 'IDS_TEST2.png.sha1'),
|
||||
'binary',
|
||||
self.VALID_SHA1,
|
||||
action='D'),
|
||||
MockFile(
|
||||
os.path.join('part_grdp', 'IDS_PART_TEST1.png.sha1'),
|
||||
'binary',
|
||||
self.VALID_SHA1,
|
||||
action='D'),
|
||||
MockFile(
|
||||
os.path.join('part_grdp', 'IDS_PART_TEST2.png.sha1'),
|
||||
'binary',
|
||||
self.VALID_SHA1,
|
||||
action='D')
|
||||
])
|
||||
warnings = PRESUBMIT.CheckStrings(input_api, MockOutputApi())
|
||||
|
||||
Reference in New Issue
Block a user