New issue
Advanced search Search tips

Issue 919437 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 9
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows
Pri: 1
Type: Bug

Blocking:
issue 899438
issue 912946



Sign in to add a comment

compare_build_artifacts failing on multiple builders

Project Member Reported by sheriff-...@appspot.gserviceaccount.com, Jan 7

Issue description

Components: UI>Browser>PrintPreview
Labels: OS-Linux OS-Windows
Owner: rbpotter@chromium.org
Status: Assigned (was: Available)
Example failure: https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8925212607930961024/+/steps/compare_build_artifacts/0/stdout

Different list of files in both directories:
  test_data/chrome/browser/resources/print_preview
  test_data/chrome/browser/resources/print_preview/data/measurement_system.js
  test_data/chrome/browser/resources/print_preview/print_preview_utils.js
  test_data/chrome/browser/resources/print_preview/print_preview_utils_unittest.gtestjs

Must be caused by https://chromium-review.googlesource.com/c/chromium/src/+/1396682 somehow.
Project Member

Comment 2 by bugdroid1@chromium.org, Jan 7

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d05c56e17bb3f85dd3cb7511bd67cd3b853c6cd6

commit d05c56e17bb3f85dd3cb7511bd67cd3b853c6cd6
Author: Max Morin <maxmorin@chromium.org>
Date: Mon Jan 07 10:35:27 2019

Revert "Print Preview cleanup: Remove some unused utils functions"

This reverts commit e444634d69b43c5f9b3833330cf5d6561ee4534a.

Reason for revert:  https://crbug.com/919437 

Original change's description:
> Print Preview cleanup: Remove some unused utils functions
> 
> - Remove utils functions that are now unused due to deletion of old UI
> - Remove arrayContains usage in favor of Array.includes
> - Remove utils unittests. Page range tests have been migrated to
>   chrome/test/data/webui/print_preview/pages_settings_test.js.
> 
> Bug:  908705 
> Change-Id: I832bbfd095e8ab671fa71300fef554f49fcf512f
> Reviewed-on: https://chromium-review.googlesource.com/c/1396682
> Commit-Queue: Rebekah Potter <rbpotter@chromium.org>
> Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#620137}

TBR=dpapad@chromium.org,rbpotter@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  908705 ,  919437 
Change-Id: I22b869cd7a5172234e6432aa62c9cc91e12cf926
Reviewed-on: https://chromium-review.googlesource.com/c/1397622
Reviewed-by: Max Morin <maxmorin@chromium.org>
Commit-Queue: Max Morin <maxmorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620284}
[modify] https://crrev.com/d05c56e17bb3f85dd3cb7511bd67cd3b853c6cd6/chrome/browser/resources/print_preview/data/cloud_parsers.js
[modify] https://crrev.com/d05c56e17bb3f85dd3cb7511bd67cd3b853c6cd6/chrome/browser/resources/print_preview/data/destination.js
[modify] https://crrev.com/d05c56e17bb3f85dd3cb7511bd67cd3b853c6cd6/chrome/browser/resources/print_preview/data/destination_match.js
[modify] https://crrev.com/d05c56e17bb3f85dd3cb7511bd67cd3b853c6cd6/chrome/browser/resources/print_preview/print_preview_utils.js
[add] https://crrev.com/d05c56e17bb3f85dd3cb7511bd67cd3b853c6cd6/chrome/browser/resources/print_preview/print_preview_utils_unittest.gtestjs
[modify] https://crrev.com/d05c56e17bb3f85dd3cb7511bd67cd3b853c6cd6/chrome/test/data/webui/BUILD.gn

Labels: -Sheriff-Chromium
Cc: h...@chromium.org thakis@chromium.org rbpotter@chromium.org
Labels: Type-Bug
Owner: thestig@chromium.org
I think this happened because the bot config is broken. It tries to clobber both out/Release.1 and out/Release.2 to give each build a clean start, but then uses out/Release and out/Release.2 for the actual builds. Let me try fixing scripts/slave/recipes/swarming/deterministic_build.py.
This bot is configured this way intentionally, to make sure incremental and full builds produce the same deterministic output. Will provide more details in a sec, but the bot config is right and something else regressed for these bots used to be happy in this config.
Issue 899438 has the background for the bot setup.
The problem is this line:

https://cs.chromium.org/chromium/src/chrome/test/BUILD.gn?q=test_data/chrome/browser/resources/print_preview&sq=package:chromium&g=0&l=3408

      data += [
        "$root_out_dir/test_data/chrome/browser/resources/print_preview/",

Adding a directory below $root_out_dir to data breaks incremental determinism since we'll zip up the whole build dir each time, but the list of files in there depends on what got put there over time.

The fix is to do something like https://chromium-review.googlesource.com/c/chromium/src/+/1366443 or https://chromium-review.googlesource.com/c/1368584 for "$root_out_dir/test_data/chrome/browser/resources/print_preview/" instead and then reland. (See the two bugs linked to from that CL for more information.)
Blocking: 912946 899438
(I'm slowly going through these in issue 912946; apologies I didn't get to this one yet.)
Thanks for pointing out the GN data issue. I'll work on it.

It was not obvious out/Release is being used on https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Deterministic%20Linux/19396 for an incremental build. Since the bot does:

1) rmtree out/Release.1
2) rmtree out/Release.2
3) build out/Release
4) build out/Release.2
5) compare out/Release out/Release.2

step (1) just looks "wrong" though really it is just a no-op.
Cc: -rbpotter@chromium.org thestig@chromium.org
Owner: rbpotter@chromium.org
Status: Started (was: Assigned)
Think we may be able to just remove that line entirely and reland, since we won't need any print preview JS files for unit_tests after deleting print_preview_utils_unittests.gtestjs in the CL. 
Project Member

Comment 11 by bugdroid1@chromium.org, Jan 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a9de86bea3a7eed539186a2463cddb99dd087673

commit a9de86bea3a7eed539186a2463cddb99dd087673
Author: rbpotter <rbpotter@chromium.org>
Date: Tue Jan 08 20:43:10 2019

Reland Print Preview cleanup: Remove some unused utils functions

Bug:  919437 
Change-Id: I58fad7c05c976d149a3db839647e449f92badf27
Reviewed-on: https://chromium-review.googlesource.com/c/1398388
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Commit-Queue: Rebekah Potter <rbpotter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620865}
[modify] https://crrev.com/a9de86bea3a7eed539186a2463cddb99dd087673/chrome/browser/resources/print_preview/BUILD.gn
[modify] https://crrev.com/a9de86bea3a7eed539186a2463cddb99dd087673/chrome/browser/resources/print_preview/data/BUILD.gn
[modify] https://crrev.com/a9de86bea3a7eed539186a2463cddb99dd087673/chrome/browser/resources/print_preview/data/cloud_parsers.js
[modify] https://crrev.com/a9de86bea3a7eed539186a2463cddb99dd087673/chrome/browser/resources/print_preview/data/destination.js
[modify] https://crrev.com/a9de86bea3a7eed539186a2463cddb99dd087673/chrome/browser/resources/print_preview/data/destination_match.js
[modify] https://crrev.com/a9de86bea3a7eed539186a2463cddb99dd087673/chrome/browser/resources/print_preview/new/BUILD.gn
[modify] https://crrev.com/a9de86bea3a7eed539186a2463cddb99dd087673/chrome/browser/resources/print_preview/new/preview_area.html
[modify] https://crrev.com/a9de86bea3a7eed539186a2463cddb99dd087673/chrome/browser/resources/print_preview/print_preview_utils.html
[modify] https://crrev.com/a9de86bea3a7eed539186a2463cddb99dd087673/chrome/browser/resources/print_preview/print_preview_utils.js
[delete] https://crrev.com/f3ee0b02475e5845f2404e707fb2f197410ca873/chrome/browser/resources/print_preview/print_preview_utils_unittest.gtestjs
[modify] https://crrev.com/a9de86bea3a7eed539186a2463cddb99dd087673/chrome/test/BUILD.gn
[modify] https://crrev.com/a9de86bea3a7eed539186a2463cddb99dd087673/chrome/test/data/webui/BUILD.gn

Status: Fixed (was: Started)

Sign in to add a comment