From 260eb0f662b4b173c66e9dc892328f982185e83c Mon Sep 17 00:00:00 2001 From: Nodir Turakulov Date: Mon, 23 Nov 2020 16:36:47 +0000 Subject: [PATCH] [tryserver] Report affected files via property To enable aggregate analysis on changed files in BigQuery, conditionally report them via an output property. This CL causes a non-trivial roll only because of the sorting. Recipe-Nontrivial-Roll: build Bug: 1151655 Change-Id: Ie7a6c622196143c1aef07af9a9a356fb4202ea56 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2553349 Reviewed-by: Andrii Shyshkalov Commit-Queue: Nodir Turakulov --- recipes/README.recipes.md | 26 ++++++++++--------- recipes/recipe_modules/tryserver/api.py | 14 +++++++++- .../full.expected/with_gerrit_patch.json | 3 ++- .../with_gerrit_patch_and_target_ref.json | 3 ++- .../full.expected/with_wrong_patch.json | 3 ++- .../full.expected/with_wrong_patch_new.json | 3 ++- .../recipe_modules/tryserver/examples/full.py | 4 ++- 7 files changed, 38 insertions(+), 18 deletions(-) diff --git a/recipes/README.recipes.md b/recipes/README.recipes.md index da747bb5c..59fc533a1 100644 --- a/recipes/README.recipes.md +++ b/recipes/README.recipes.md @@ -828,21 +828,23 @@ Returns gerrit change destination ref, e.g. "refs/heads/master". Populated iff gerrit_change is populated. -— **def [get\_files\_affected\_by\_patch](/recipes/recipe_modules/tryserver/api.py#143)(self, patch_root, \*\*kwargs):** +— **def [get\_files\_affected\_by\_patch](/recipes/recipe_modules/tryserver/api.py#143)(self, patch_root, report_files_via_property=None, \*\*kwargs):** Returns list of paths to files affected by the patch. Args: * patch_root: path relative to api.path['root'], usually obtained from api.gclient.get_gerrit_patch_root(). + * report_files_via_property: name of the output property to report the + list of the files. If None (default), do not report. Returned paths will be relative to to patch_root. -— **def [get\_footer](/recipes/recipe_modules/tryserver/api.py#253)(self, tag, patch_text=None):** +— **def [get\_footer](/recipes/recipe_modules/tryserver/api.py#265)(self, tag, patch_text=None):** Gets a specific tag from a CL description -— **def [get\_footers](/recipes/recipe_modules/tryserver/api.py#233)(self, patch_text=None):** +— **def [get\_footers](/recipes/recipe_modules/tryserver/api.py#245)(self, patch_text=None):** Retrieves footers from the patch description. @@ -861,20 +863,20 @@ Returns true iff the properties exist to match a Gerrit issue. Returns true iff we have a change to check out. -— **def [normalize\_footer\_name](/recipes/recipe_modules/tryserver/api.py#257)(self, footer):** +— **def [normalize\_footer\_name](/recipes/recipe_modules/tryserver/api.py#269)(self, footer):** -— **def [set\_change](/recipes/recipe_modules/tryserver/api.py#260)(self, change):** +— **def [set\_change](/recipes/recipe_modules/tryserver/api.py#272)(self, change):** Set the gerrit change for this module. Args: * change: a self.m.buildbucket.common_pb2.GerritChange. -— **def [set\_compile\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#196)(self):** +— **def [set\_compile\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#208)(self):** Mark the tryjob result as a compile failure. -— **def [set\_invalid\_test\_results\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#208)(self):** +— **def [set\_invalid\_test\_results\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#220)(self):** Mark the tryjob result as having invalid test results. @@ -882,32 +884,32 @@ This means we run some tests, but the results were not valid (e.g. no list of specific test cases that failed, or too many tests failing, etc). -— **def [set\_patch\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#192)(self):** +— **def [set\_patch\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#204)(self):** Mark the tryjob result as failure to apply the patch. -— **def [set\_subproject\_tag](/recipes/recipe_modules/tryserver/api.py#170)(self, subproject_tag):** +— **def [set\_subproject\_tag](/recipes/recipe_modules/tryserver/api.py#182)(self, subproject_tag):** Adds a subproject tag to the build. This can be used to distinguish between builds that execute different steps depending on what was patched, e.g. blink vs. pure chromium patches. -— **def [set\_test\_expired\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#225)(self):** +— **def [set\_test\_expired\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#237)(self):** Mark the tryjob result as a test expiration. This means a test task expired and was never scheduled, most likely due to lack of capacity. -— **def [set\_test\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#200)(self):** +— **def [set\_test\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#212)(self):** Mark the tryjob result as a test failure. This means we started running actual tests (not prerequisite steps like checkout or compile), and some of these tests have failed. -— **def [set\_test\_timeout\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#217)(self):** +— **def [set\_test\_timeout\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#229)(self):** Mark the tryjob result as a test timeout. diff --git a/recipes/recipe_modules/tryserver/api.py b/recipes/recipe_modules/tryserver/api.py index b0d551360..cb06d3f7c 100644 --- a/recipes/recipe_modules/tryserver/api.py +++ b/recipes/recipe_modules/tryserver/api.py @@ -140,12 +140,16 @@ class TryserverApi(recipe_api.RecipeApi): self.m.properties.get('patch_repo_url') and self.m.properties.get('patch_ref')) - def get_files_affected_by_patch(self, patch_root, **kwargs): + def get_files_affected_by_patch(self, patch_root, + report_files_via_property=None, + **kwargs): """Returns list of paths to files affected by the patch. Args: * patch_root: path relative to api.path['root'], usually obtained from api.gclient.get_gerrit_patch_root(). + * report_files_via_property: name of the output property to report the + list of the files. If None (default), do not report. Returned paths will be relative to to patch_root. """ @@ -160,11 +164,19 @@ class TryserverApi(recipe_api.RecipeApi): **kwargs) paths = [self.m.path.join(patch_root, p) for p in step_result.stdout.split()] + paths.sort() if self.m.platform.is_win: # Looks like "analyze" wants POSIX slashes even on Windows (since git # uses that format even on Windows). paths = [path.replace('\\', '/') for path in paths] step_result.presentation.logs['files'] = paths + if report_files_via_property: + step_result.presentation.properties[report_files_via_property] = { + 'total_count': len(paths), + # Do not report too many because it might violate build size limits, + # and isn't very useful anyway. + 'first_100': paths[:100], + } return paths def set_subproject_tag(self, subproject_tag): diff --git a/recipes/recipe_modules/tryserver/examples/full.expected/with_gerrit_patch.json b/recipes/recipe_modules/tryserver/examples/full.expected/with_gerrit_patch.json index 1842a15f0..1ebd1734d 100644 --- a/recipes/recipe_modules/tryserver/examples/full.expected/with_gerrit_patch.json +++ b/recipes/recipe_modules/tryserver/examples/full.expected/with_gerrit_patch.json @@ -119,7 +119,8 @@ "name": "git diff to analyze patch", "~followup_annotations": [ "@@@STEP_LOG_LINE@files@None/foo.cc@@@", - "@@@STEP_LOG_END@files@@@" + "@@@STEP_LOG_END@files@@@", + "@@@SET_BUILD_PROPERTY@affected_files@{\"first_100\": [\"None/foo.cc\"], \"total_count\": 1}@@@" ] }, { diff --git a/recipes/recipe_modules/tryserver/examples/full.expected/with_gerrit_patch_and_target_ref.json b/recipes/recipe_modules/tryserver/examples/full.expected/with_gerrit_patch_and_target_ref.json index 99cf74700..c254f4fab 100644 --- a/recipes/recipe_modules/tryserver/examples/full.expected/with_gerrit_patch_and_target_ref.json +++ b/recipes/recipe_modules/tryserver/examples/full.expected/with_gerrit_patch_and_target_ref.json @@ -119,7 +119,8 @@ "name": "git diff to analyze patch", "~followup_annotations": [ "@@@STEP_LOG_LINE@files@None/foo.cc@@@", - "@@@STEP_LOG_END@files@@@" + "@@@STEP_LOG_END@files@@@", + "@@@SET_BUILD_PROPERTY@affected_files@{\"first_100\": [\"None/foo.cc\"], \"total_count\": 1}@@@" ] }, { diff --git a/recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch.json b/recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch.json index 7f7bb0f83..9247615e0 100644 --- a/recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch.json +++ b/recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch.json @@ -12,7 +12,8 @@ "name": "git diff to analyze patch", "~followup_annotations": [ "@@@STEP_LOG_LINE@files@foo.cc@@@", - "@@@STEP_LOG_END@files@@@" + "@@@STEP_LOG_END@files@@@", + "@@@SET_BUILD_PROPERTY@affected_files@{\"first_100\": [\"foo.cc\"], \"total_count\": 1}@@@" ] }, { diff --git a/recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch_new.json b/recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch_new.json index b67f83fbf..c5452bc37 100644 --- a/recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch_new.json +++ b/recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch_new.json @@ -13,7 +13,8 @@ "name": "git diff to analyze patch", "~followup_annotations": [ "@@@STEP_LOG_LINE@files@sub/project/foo.cc@@@", - "@@@STEP_LOG_END@files@@@" + "@@@STEP_LOG_END@files@@@", + "@@@SET_BUILD_PROPERTY@affected_files@{\"first_100\": [\"sub/project/foo.cc\"], \"total_count\": 1}@@@" ] }, { diff --git a/recipes/recipe_modules/tryserver/examples/full.py b/recipes/recipe_modules/tryserver/examples/full.py index 326394ada..15c237c91 100644 --- a/recipes/recipe_modules/tryserver/examples/full.py +++ b/recipes/recipe_modules/tryserver/examples/full.py @@ -39,7 +39,9 @@ def RunSteps(api): if api.tryserver.is_gerrit_issue: api.tryserver.get_footers() api.tryserver.get_files_affected_by_patch( - api.properties.get('test_patch_root')) + api.properties.get('test_patch_root'), + report_files_via_property='affected_files', + ) if api.tryserver.is_tryserver: api.tryserver.set_subproject_tag('v8')