New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 736941 link

Starred by 2 users
Status: Fixed
Owner:
dpranke@chromium.org
Closed: Jul 2017
Cc:
█ phajdan.jr@chromium.org
█ brettw@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug



Sign in to add a comment

GN rolls bypass `analyze`, which is potentially dangerous

Project Member Reported by dpranke@chromium.org, Jun 26 2017 
As seen in  bug 736215 , when you roll in a new version of GN, that is rolled in as a DEPS change, and DEPS changes are in the analyze "exclusion" list, which assumes that we don't need to run analyze.

This is particularly dangerous, since GN is the thing that implements analyze, and so if there's a bug in it it won't be caught in the CQ tryjobs.

We should figure out how to fix that; one possible idea would be to always run analyze, but ignore the results assuming the step runs successfully to completion (Actually, I feel like I've already reported this idea in a bug somewhere a year or more ago). 

Another idea would be to figure out if we can get rid of the exclusions altogether :).

Comment 1 by dpranke@chromium.org, Jun 30 2017

Labels: -Pri-2 Pri-1
Owner: dpranke@chromium.org
Status: Assigned (was: Untriaged)

Comment 2 by dpranke@chromium.org, Jun 30 2017

Labels: cit-pm-52

Comment 3 by dpranke@chromium.org, Jun 30 2017

Labels: -cit-pm-52 cit-pm-53

Comment 4 by dpranke@chromium.org, Jul 8 2017

Labels: cit-pm-55

Comment 5 by dpranke@chromium.org, Jul 11 2017 

Started: crrev.com/c/566087 .

Comment 6 by dpranke@chromium.org, Jul 11 2017

Status: Started (was: Assigned)
Project Member

Comment 7 by bugdroid1@chromium.org, Jul 11 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/b94e6a639c3c1561bc948878e3b17a1b11aff2fd

commit b94e6a639c3c1561bc948878e3b17a1b11aff2fd
Author: Dirk Pranke <dpranke@chromium.org>
Date: Tue Jul 11 15:49:14 2017

Modify the way analyze is implemented in the chromium recipes.

Previously, if a file matches something in the trybot exlusion
list, we wouldn't call `mb analyze` at all, and would exit early,
assuming everything needed to rebuild. However, this would let
bugs in mb or gn get through the CQ accidentally (particularly
leading to bugs in analyze that would show up on subsequent tryjobs).

This CL changes the behavior so that we *always* call `mb analyze`.
If analyze errors out, then we raise an error. However, if it runs
properly, and a file matches the exclusion rules, we then ignore
the results from MB.

One effect of this change is that a build will now have a new
"analyze_matched_exclusion" step as well as the "analyze" step,
if the exclusions were matched.

R=phajdan.jr@chromium.org
BUG= 736941 

Change-Id: Ic2c3d2f72f94e32e128605c94e7e3cced0181c4f
Reviewed-on: https://chromium-review.googlesource.com/566087
Commit-Queue: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Paweł Hajdan Jr. <phajdan.jr@chromium.org>

[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/chromium_trybot.expected/non_cq_blink_tryjob.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/chromium_trybot.expected/invalid_results.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/chromium_trybot.expected/recipe_config_changes_not_retried_without_patch.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipe_modules/filter/examples/full.expected/match_additional_name_exclusion.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/chromium_trybot.expected/use_webrtc_patch_on_chromium_trybot_compile_failure.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/chromium_trybot.expected/compile_failure_with_component_rev.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipe_modules/filter/tests/suppress_analyze.expected/basic.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/chromium_trybot.expected/swarming_test_with_priority_expiration_and_timeout.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/chromium_trybot.expected/dynamic_isolated_script_test_on_trybot_passing.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/chromium_trybot.expected/swarmed_webkit_tests_minimal_pass_continues.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/ios/try.expected/basic.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/chromium_trybot.expected/swarming_trigger_failure.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipe_modules/filter/examples/full.expected/match_exclusion.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/chromium_trybot.expected/compile_failure_ng.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/chromium_trybot.expected/swarming_missing_isolated.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/chromium_trybot.expected/compile_failure_without_patch_deapply_fn.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/chromium_trybot.expected/dont_deapply_patch.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/chromium_trybot.expected/swarming_basic_cq.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/ios/try.expected/without_patch_success.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/ios/try.expected/gn.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/chromium_trybot.expected/dynamic_isolated_script_test_on_trybot_failing.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/chromium_trybot.expected/compile_because_of_analyze_matching_exclusion.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/chromium_trybot.expected/compile_failure_without_patch_ng.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipe_modules/chromium_tests/tests/api/trybot_steps.expected/basic.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/chromium_trybot.expected/swarmed_layout_tests_with_and_without_patch_fail.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/ios/try.expected/goma_compilation_failure.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/ios/try.expected/parent.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/chromium_trybot.expected/dynamic_swarmed_isolated_script_test_failure_no_result_json.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/chromium_trybot.expected/swarmed_webkit_tests_compile_without_patch_fails.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/chromium_trybot.expected/swarmed_webkit_tests_interrupted.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/chromium_trybot.expected/swarmed_webkit_tests_unexpected_error.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipe_modules/filter/api.py
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/chromium_trybot.expected/swarmed_layout_tests_too_many_failures_for_retcode.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/ios/try.expected/without_patch_failure.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/chromium_trybot.expected/dynamic_isolated_script_test_with_args_on_trybot.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/chromium_trybot.expected/process_dumps_failure.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/chromium_trybot.expected/compile_failure_infra.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/README.recipes.md
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/ios/try.expected/no_tests.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/chromium_trybot.expected/swarming_test_failure.json
[modify] https://crrev.com/b94e6a639c3c1561bc948878e3b17a1b11aff2fd/scripts/slave/recipes/chromium_trybot.expected/swarming_basic_try_job.json

Comment 8 by dpranke@chromium.org, Jul 12 2017

Status: Fixed (was: Started)
►

Sign in to add a comment