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

Issue 910465 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 4
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: ----

Blocked on:
issue 899438

Blocking:
issue 330260



Sign in to add a comment

compare_build_artifacts failing on chromium.win/Windows deterministic due to updates to xtb files

Project Member Reported by sheriff-...@appspot.gserviceaccount.com, Nov 30

Issue description

Filed by sheriff-o-matic@appspot.gserviceaccount.com on behalf of ortuno@chromium.org

compare_build_artifacts failing on chromium.win/Windows deterministic

Builders failed on: 
- Windows deterministic: 
  https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Windows%20deterministic


 
Three runs have failed already. There are a bunch of errors e.g.:

unit_tests.exe           : DIFFERENT (unexpected): 53839 out of 118800896 bytes are different (0.05%)
unit_tests.exe.pdb       : DIFFERENT (unexpected): 28 out of 319504384 bytes are different (0.00%)

setup_unittests.exe      : DIFFERENT (unexpected): 48148 out of 2447872 bytes are different (1.97%)
setup_unittests.exe.pdb  : DIFFERENT (unexpected): 29 out of 29736960 bytes are different (0.00%)

setup.exe                : DIFFERENT (unexpected): 47608 out of 2217472 bytes are different (2.15%)
setup.exe.pdb            : DIFFERENT (unexpected): 28 out of 33837056 bytes are different (0.00%)

etc.
Cc: -thakis@chromium.org
Labels: OS-Windows
Owner: thakis@chromium.org
Status: Assigned (was: Available)
Could you take a look, Nico? A lot of CLs made it into the first failing build (https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Windows%20deterministic/10903). Nothing jumped out at me, but I don't know exactly what to look for.

Here's one example:

unit_tests.exe                                                                                                                                                   : DIFFERENT (unexpected): 53839 out of 118800896 bytes are different (0.05%)
unit_tests.exe.pdb                                                                                                                                               : DIFFERENT (unexpected): 28 out of 319504384 bytes are different (0.00%)
  0xa51f160 : 00000000400000402e72737263000000806c04000040e906006e0400007ed106 '....@..@.rsrc....l...@...n...~..'
              00000000400000402e72737263000000886c04000040e906006e0400007ed106 '....@..@.rsrc....l...@...n...~..'
                                               ^
  0x111f7040: 1a0036110a000c000040e906806c0400400000402e727372630000001a003611 '..6......@...l..@..@.rsrc.....6.'
              1a0036110a000c000040e906886c0400400000402e727372630000001a003611 '..6......@...l..@..@.rsrc.....6.'
                                       ^
  0x1170bfa0: 80200000400000401d68000027a372be0000000000002a680400000000000000 '. ..@..@.h..'.r.......*h........'
              80200000400000401d680000752c0ea00000000000002a680400000000000000 '. ..@..@.h..u,........*h........'
                                      ++ ^^^ ^                                              ^^
  0x12e2c2e0: 80200000400000401d68000027a372be000000000a00000080200000004c0400 '. ..@..@.h..'.r.......... ...L..'
              80200000400000401d680000752c0ea0000000000a00000080200000084c0400 '. ..@..@.h..u,........... ...L..'
                                      ++ ^^^ ^                         ^                    ^^
  0x12e2c300: 400000401d680000e17aa063000000000b000000000000006800000040000042 '@..@.h...z.c............h...@..B'
              400000401d6800004d0a89ce000000000b000000000000006800000040000042 '@..@.h..M...............h...@..B'
                              +++++++                                                   +  ^
  0x12edc960: 00000000806c04000901000000000b00ffffffff0000000000d63e0008020000 '.....l....................>.....'
              00000000886c04000901000000000b00ffffffff0000000000d63e0008020000 '.....l....................>.....'
                       ^
  0x13046000: 942e31018121b837010000008121b83784b2769c4c4c44205044422e96000000 '..1..!.7.....!.7..v.LLD PDB.....'
              942e310122b1d8a30100000022b1d8a33348d20b4c4c44205044422e96000000 '..1."......."...3H..LLD PDB.....'
              942e310122b1d8a30100000022b1d8a33348d20b4c4c44205044422e96000000 '..1."......."...3H..LLD PDB.....
Labels: -Pri-2 Pri-1
I tried doing a component build of installer_util_unittests.exe locally and couldn't repro. The bot is doing a static library build, but I'd be a bit surprised if that's the cause (nevertheless, I'll try that next).

Here are the 5 exes (+ their pdbs) that show differences:
installer_util_unittests
notification_helper_unittests
setup
setup_unittests
unit_tests


setup.exe is the only non-test binary.
No local diffs in a release build either.

So I'm guessing there's some incrementality issue. I did full builds in two build dirs. The bot does incremental builds in one and full builds in the other.

Intriguing!
Labels: -Sheriff-Chromium
I'll try to bisect. I'm starting by getting ce48cf76141dac6cd5de72034096cd00cd57f6fa / r612334 (last known good) and doing two full builds. Then I'll sync halfway into the changes in https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Windows%20deterministic/10903 and do an incremental build and one full build and compare installer_util_unittests, then repeat.
I can repro with the approach in comment 7 at 612367. Now doing two full builds at r612334 again and will then try again one incremental and one full build at 612350.

Also attaching the two different exes, diff looks pretty small actually. The compare script should probably print the start of the diff even if the size doesn't match.
installer_util_unittests.exe
2.9 MB Download
installer_util_unittests.exe
2.9 MB Download
Actually, the exe diffs looks like this is .rsrc / grit-related, so maybe it's triggered by 612348. Trying 612347 locally first and if there's no diff there, then 612348.
No diff at 612347 . Trying 612348 / 3a393aebc4b0bbfbfeb7a2fca4b99cb8285ac75e
It's due to 612348 .

This being Windows-only and only affecting a small number of binaries suggests that there's something we need to do on xtb updates that almost everything gets right but some rule somewhere doesn't...
Summary: compare_build_artifacts failing on chromium.win/Windows deterministic due to updates to xtb files (was: compare_build_artifacts failing on chromium.win/Windows deterministic)
I hacked up compare_build_artifacts.py to compare all files and ran it with --recursive, here's the output:

Unexpected files with diffs:

  gen\chrome\installer\util\installer_util_strings.rc
  installer_util_unittests.exe
  installer_util_unittests.exe.pdb
  obj\chrome\installer\util\strings\installer_util_strings.res
  obj\chrome\installer\util\strings\installer_util_strings.res_ms_rc
  toolchain.ninja


diff --git a/tools/determinism/compare_build_artifacts.py b/tools/determinism/compare_build_artifacts.py
index 3546f763980d..ba690736f121 100755
--- a/tools/determinism/compare_build_artifacts.py
+++ b/tools/determinism/compare_build_artifacts.py
@@ -24,17 +24,19 @@ BASE_DIR = os.path.dirname(os.path.abspath(__file__))

 def get_files_to_compare(build_dir, recursive=False):
   """Get the list of files to compare."""
+  #return set(['installer_util_unittests.exe'])
   # TODO(maruel): Add '.pdb'.
   allowed = frozenset(
-      ('', '.apk', '.app', '.dll', '.dylib', '.exe', '.nexe', '.so'))
+      ('.pak',))

   # .bin is for the V8 snapshot files natives_blob.bin, snapshot_blob.bin
-  non_x_ok_exts = frozenset(('.apk', '.bin', '.isolated', '.zip'))
+  non_x_ok_exts = frozenset()
   def check(f):
     if not os.path.isfile(f):
       return False
     if os.path.basename(f).startswith('.'):
       return False
+    return True
     ext = os.path.splitext(f)[1]
     if ext in non_x_ok_exts:
       return True
@@ -325,7 +327,8 @@ def compare_build_artifacts(first_dir, second_dir, ninja_path, target_platform,
         unexpected_diffs.append(f)
         tag = 'unexpected'
       result = 'DIFFERENT (%s): %s' % (tag, result)
-    print('%-*s: %s' % (max_filepath_len, f, result))
+    if tag != 'equal':
+      print('%-*s: %s' % (max_filepath_len, f, result))
   unexpected_diffs.sort()

   print('Equals:           %d' % len(equals))
Blocking: 330260
Filed  bug 911183  for the toolchain.ninja diff, that's unrelated.
Diffing gen\chrome\installer\util\installer_util_strings.rc (opened as `:e ++enc=utf16le` after doing `set encoding=utf-8`) this line is different:

IDS_INSTALL_OS_ERROR_TE "ఇన్‌స్టాలేషన్ సమయంలో ఆపరేటింగ్ సిస్టమ్ లోపం సంభవించింది. దయచేసి

vs

IDS_INSTALL_OS_ERROR_TE "ఇన్‌స్టాలేషన్ సమయంలో ఆపరేటింగ్ సిస్టమ్ ఎర్ర‌ర్ ఏర్ప‌డింది. దయచేసి 


Maybe grit's rc writer doesn't use depfiles or something?


Actually

IDS_INSTALL_OS_ERROR_TE "ఇన్‌స్టాలేషన్ సమయంలో ఆపరేటింగ్ సిస్టమ్ లోపం సంభవించింది. దయచేసి Chromiumని మళ్లీ డౌన్‌లోడ్ చేయండి."

vs

IDS_INSTALL_OS_ERROR_TE "ఇన్‌స్టాలేషన్ సమయంలో ఆపరేటింగ్ సిస్టమ్ ఎర్ర‌ర్ ఏర్ప‌డింది. దయచేసి Chromiumని మళ్లీ డౌన్‌లోడ్ చేయండి."
Status: Started (was: Assigned)
Oh, looks like the h and rc here are created by chrome/installer/util/prebuild/create_string_rc.py which just doesn't use a depfile at all, which is probably the bug, and which explains why this is Windows only. This explains installer_util_unittests for sure, probably setup, setup_unittests, and unit_tests. notification_helper_unittests might be something else, but let's fix this first and then see what the bot does.

create_string_rc has been around since initial.commit, but grt has touched it some over the years. (grt, just cc'ing you so you're not surprised when I send you a patch in a bit, nothing else to do for you here.)
Blockedon: 899438
Found due to the work in issue 899438
Project Member

Comment 21 by bugdroid1@chromium.org, Dec 3

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

commit 1c59d48c61bfd44779d00e7e381cb0a11e0f0d5e
Author: Nico Weber <thakis@chromium.org>
Date: Mon Dec 03 20:33:21 2018

win: Make chrome/installer/util/prebuild/create_string_rc write a depfile.

create_string_rc.py globs a bunch of xtb files off disk behind gn's and
ninja's back, so that they didn't know that create_string_rc.py needs to
rerun when the xtb files are updated. Make the script write a depfile so
that ninja knows to re-run the script if an xtb file is udpated.

(This is incomplete: if an xtb file is added or removed and nothing else
changes, the step still won't re-run. The Right Fix is to list all the xtb
files in the gn file and pass them to the script, then make the script assert
that the glob matches the passed-in line. That way, if an xtb is added or
removed, the command-line tracking will make sure that the command re-runs.
But xtb files being added or removed is super rare, so let's punt on this for
now.)

Bug:  910465 
Change-Id: I7050dee3c62d997b750d8b150504a04d086518b8
Reviewed-on: https://chromium-review.googlesource.com/c/1359087
Reviewed-by: Robert Shield <robertshield@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613222}
[modify] https://crrev.com/1c59d48c61bfd44779d00e7e381cb0a11e0f0d5e/chrome/installer/util/BUILD.gn
[modify] https://crrev.com/1c59d48c61bfd44779d00e7e381cb0a11e0f0d5e/chrome/installer/util/prebuild/create_string_rc.py

Sign in to add a comment