Bug 1461490 - Add an allowlist of expected update verify differences, r?aki
authorNick Thomas <nthomas@mozilla.com>
Mon, 21 May 2018 10:16:46 +1200
changeset 8413 e94302d4b606
parent 8412 da92c53fdd68
child 8414 74a7df6d3fc5
push id6141
push usernthomas@mozilla.com
push dateMon, 21 May 2018 22:16:36 +0000
reviewersaki
bugs1461490
Bug 1461490 - Add an allowlist of expected update verify differences, r?aki This reimplements our source+update vs target comparison in Python, allowing the definition of a set of transforms to resolve expected differences. These relate to preprocessed lines in channel-prefs.js (which depend on CI implementation), and updating users on the beta channel with X.0 candidate builds for the release channel (RCs). The transforms * are restricted to specific files and channel prefixes, * may act on the source or target directory, * can delete lines and/or replace line content The comparison script exits with status 2 on any file differences remaining after applying transforms, or if files/directories are found only in source or target. This signals a test failure to the calling script. Previously files/directories on only one side didn't change the exit status, merely logging a warning. Only unresolved differences make it into the diff-summary.log artifact on the update verify tasks. MozReview-Commit-ID: AtSfq0dVowA
release/common/check_updates.sh
release/common/unpack.sh
release/compare-directories.py
release/updates/verify.sh
--- a/release/common/check_updates.sh
+++ b/release/common/check_updates.sh
@@ -1,20 +1,21 @@
 check_updates () {
-  # called with 8 args - platform, source package, target package, update package, old updater boolean,
-  # a path to the updater binary to use for the tests, a file to write diffs to,
+  # called with 9 args - platform, source package, target package, update package, old updater boolean,
+  # a path to the updater binary to use for the tests, a file to write diffs to, the update channel,
   # and (sometimes) update-settings.ini values
   update_platform=$1
   source_package=$2
   target_package=$3
   locale=$4
   use_old_updater=$5
   updater=$6
   diff_file=$7
-  mar_channel_IDs=$8
+  channel=$8
+  mar_channel_IDs=$9
 
   # cleanup
   rm -rf source/*
   rm -rf target/*
 
   unpack_build $update_platform source "$source_package" $locale '' $mar_channel_IDs
   if [ "$?" != "0" ]; then
     echo "FAILED: cannot unpack_build $update_platform source $source_package"
@@ -32,27 +33,16 @@ check_updates () {
           ;;
       WINNT*) 
           platform_dirname="bin"
           ;;
       Linux_x86-gcc | Linux_x86-gcc3 | Linux_x86_64-gcc3) 
           platform_dirname=`echo $product | tr '[A-Z]' '[a-z]'`
           ;;
   esac
-  case `uname` in
-      Darwin)
-          binary_file_pattern='^Binary files'
-          ;;
-      MINGW*)
-          binary_file_pattern='^Files.*and.*differ$'
-          ;;
-      Linux)
-          binary_file_pattern='^Binary files'
-          ;;
-  esac
 
   if [ -f update/update.status ]; then rm update/update.status; fi
   if [ -f update/update.log ]; then rm update/update.log; fi
 
   if [ -d source/$platform_dirname ]; then
     if [ `uname | cut -c-5` == "MINGW" ]; then
       # windows
       # change /c/path/to/pwd to c:\\path\\to\\pwd
@@ -104,41 +94,21 @@ check_updates () {
   cd ../..
   cd `echo "target/$platform_dirname"`
   if [[ -f "Contents/Resources/precomplete" && -f "precomplete" ]]
   then
     rm "precomplete"
   fi
   cd ../..
 
-  diff -r source/${platform_dirname} target/${platform_dirname}  > "${diff_file}"
+  ../compare-directories.py source/${platform_dirname} target/${platform_dirname}  ${channel} > "${diff_file}"
   diffErr=$?
   cat "${diff_file}"
-  grep ^Only "${diff_file}" | sed 's/^Only in \(.*\): \(.*\)/\1\/\2/' | \
-  while read to_test; do
-    if [ -d "$to_test" ]; then 
-      echo Contents of $to_test dir only in source or target
-      find "$to_test" -ls | grep -v "${to_test}$"
-    fi
-  done
-  grep "${binary_file_pattern}" "${diff_file}" > /dev/null
-  grepErr=$?
-  if [ $grepErr == 0 ]
+  if [ $diffErr == 2 ]
   then
-    echo "FAIL: binary files found in diff"
+    echo "FAIL: differences found after update"
     return 1
-  elif [ $grepErr == 1 ]
-  then
-    if [ -s "${diff_file}" ]
-    then
-      echo "WARN: non-binary files found in diff"
-      return 2
-    fi
-  else
-    echo "FAIL: unknown error from grep: $grepErr"
-    return 3
-  fi
-  if [ $diffErr != 0 ]
+  elif [ $diffErr != 0 ]
   then
     echo "FAIL: unknown error from diff: $diffErr"
     return 3
   fi
 }
--- a/release/common/unpack.sh
+++ b/release/common/unpack.sh
@@ -99,17 +99,17 @@ unpack_build () {
         for f in `find . -name '*.jar' -o -name '*.ja'`; do
             unzip -o "$f" -d "$f.dir" > /dev/null
         done
     fi
 
     if [ ! -z $update_settings_string ]; then
        echo "Modifying update-settings.ini"
        cat  "${update_settings_file}" | sed -e "s/^ACCEPTED_MAR_CHANNEL_IDS.*/ACCEPTED_MAR_CHANNEL_IDS=${update_settings_string}/" > "${update_settings_file}.new"
-       diff -u "{$update_settings_file}" "${update_settings_file}.new"
+       diff -u "${update_settings_file}" "${update_settings_file}.new"
        echo " "
        rm "${update_settings_file}"
        mv "${update_settings_file}.new" "${update_settings_file}"
     fi
 
     popd > /dev/null
 
 }
new file mode 100755
--- /dev/null
+++ b/release/compare-directories.py
@@ -0,0 +1,206 @@
+#! /usr/bin/env python
+
+import argparse
+import difflib
+import hashlib
+import logging
+import os
+import sys
+
+
+""" Define the transformations needed to make source + update == target
+
+Required:
+The files list describes the files which a transform may be used on.
+The 'side' is one of ('source', 'target') and defines where each transform is applied
+The 'channel_prefix' list controls which channels a transform may be used for, where a value of
+'beta' means all of beta, beta-localtest, beta-cdntest, etc.
+
+One or more:
+A 'deletion' specifies a start of line to match on, removing the whole line
+A 'substitution' is a list of full string to match and its replacement
+
+Future note - this may need to move into the tree to make staging releases or release-on-try easier
+"""
+TRANSFORMS = [
+    # channel-prefs.js
+    {
+        # preprocessor comments, eg //@line 6 "/builds/worker/workspace/...
+        # this can be removed once each channel has a watershed above 59.0b2 (from bug 1431342)
+        'files': ['defaults/pref/channel-prefs.js', 'Contents/Resources/defaults/pref/channel-prefs.js'],
+        'channel_prefix': ['aurora', 'beta', 'release', 'esr'],
+        'side': 'source',
+        'deletion': '//@line 6 "',
+    },
+    {
+        # updates from a beta to an RC build, the latter specifies the release channel
+        'files': ['defaults/pref/channel-prefs.js', 'Contents/Resources/defaults/pref/channel-prefs.js'],
+        'channel_prefix': ['beta'],
+        'side': 'target',
+        'substitution': [
+            'pref("app.update.channel", "release");\n',
+            'pref("app.update.channel", "beta");\n'
+        ],
+    },
+    {
+        # updates from an RC to a beta build
+        'files': ['defaults/pref/channel-prefs.js', 'Contents/Resources/defaults/pref/channel-prefs.js'],
+        'channel_prefix': ['beta'],
+        'side': 'source',
+        'substitution': [
+            'pref("app.update.channel", "release");\n',
+            'pref("app.update.channel", "beta");\n'
+        ],
+    },
+    # update-settings.ini
+    {
+        # updates from a beta to an RC build, the latter specifies the release channel
+        # on mac, we actually have both files. The second location is the real one but we copy to the first
+        # to run the linux64 updater
+        'files': ['update-settings.ini', 'Contents/Resources/update-settings.ini'],
+        'channel_prefix': ['beta'],
+        'side': 'target',
+        'substitution': [
+            'ACCEPTED_MAR_CHANNEL_IDS=firefox-mozilla-release\n',
+            'ACCEPTED_MAR_CHANNEL_IDS=firefox-mozilla-beta,firefox-mozilla-release\n'
+        ],
+    },
+    {
+        # updates from an RC to a beta build
+        # on mac, we only need to modify the legit file this time. unpack_build handles the copy for the updater in
+        # both source and target
+        'files': ['Contents/Resources/update-settings.ini'],
+        'channel_prefix': ['beta'],
+        'side': 'source',
+        'substitution': [
+            'ACCEPTED_MAR_CHANNEL_IDS=firefox-mozilla-release\n',
+            'ACCEPTED_MAR_CHANNEL_IDS=firefox-mozilla-beta,firefox-mozilla-release\n'
+        ],
+    },
+]
+
+
+
+def walk_dir(path):
+    all_files = []
+    all_dirs = []
+
+    for root, dirs, files in os.walk(path):
+        all_dirs.extend([os.path.join(root, d) for d in dirs])
+        all_files.extend([os.path.join(root, f) for f in files])
+
+    # trim off directory prefix for easier comparison
+    all_dirs = [d.replace(path + '/', '') for d in all_dirs]
+    all_files = [f.replace(path + '/', '') for f in all_files]
+
+    return all_dirs, all_files
+
+
+def compare_listings(source_list, target_list, label, source_dir, target_dir):
+    obj1 = set(source_list)
+    obj2 = set(target_list)
+    difference_found = False
+
+    left_diff = obj1 - obj2
+    if left_diff:
+        logging.error('{} only in {}:'.format(label, source_dir))
+        for d in left_diff:
+            logging.error('  {}'.format(d))
+        difference_found = True
+
+    right_diff = obj2 - obj1
+    if right_diff:
+        logging.error('{} only in {}:'.format(label, target_dir))
+        for d in right_diff:
+            logging.error('  {}'.format(d))
+        difference_found = True
+
+    return difference_found
+
+
+def hash_file(filename):
+    h = hashlib.sha256()
+    with open(filename, 'rb', buffering=0) as f:
+        for b in iter(lambda: f.read(128 * 1024), b''):
+            h.update(b)
+    return h.hexdigest()
+
+
+def compare_common_files(files, channel, source_dir, target_dir):
+    difference_found = False
+    for filename in files:
+        source_file = os.path.join(source_dir, filename)
+        target_file = os.path.join(target_dir, filename)
+
+        if os.stat(source_file).st_size != os.stat(target_file).st_size or \
+                hash_file(source_file) != hash_file(target_file):
+            logging.info('Difference found in {}'.format(filename))
+            file_contents = {
+                'source': open(source_file).readlines(),
+                'target': open(target_file).readlines(),
+            }
+
+            transforms = [t for t in TRANSFORMS if filename in t['files'] and
+                          channel.startswith(tuple(t['channel_prefix']))]
+            logging.debug('Got {} transform(s) to consider for {}'.format(len(transforms), filename))
+            for transform in transforms:
+                side = transform['side']
+
+                if 'deletion' in transform:
+                    d = transform['deletion']
+                    logging.debug('Trying deleting lines starting {} from {}'.format(d, side))
+                    file_contents[side] = [l for l in file_contents[side] if not l.startswith(d)]
+
+                if 'substitution' in transform:
+                    r = transform['substitution']
+                    logging.debug('Trying replacement for {} in {}'.format(r, side))
+                    file_contents[side] = [l.replace(r[0], r[1]) for l in file_contents[side]]
+
+                if file_contents['source'] == file_contents['target']:
+                    logging.info('Transforms removed all differences')
+                    break
+
+            if file_contents['source'] != file_contents['target']:
+                difference_found = True
+                logging.error('{} still differs after transforms, residual diff:'.format(filename))
+                for l in difflib.unified_diff(file_contents['source'], file_contents['target']):
+                    logging.error(l.rstrip())
+
+    return difference_found
+
+
+if __name__ == '__main__':
+    parser = argparse.ArgumentParser('Compare two directories recursively, with transformations for expected diffs')
+    parser.add_argument('source', help='Directory containing updated Firefox')
+    parser.add_argument('target', help='Directory containing expected Firefox')
+    parser.add_argument('channel', help='Update channel used')
+    parser.add_argument('--verbose', '-v', action='store_true', help='Enable verbose logging')
+
+    args = parser.parse_args()
+    level = logging.INFO
+    if args.verbose:
+        level = logging.DEBUG
+    logging.basicConfig(level=level, format='%(message)s', stream=sys.stdout)
+
+    source = args.source
+    target = args.target
+    if not os.path.exists(source) or not os.path.exists(target):
+        logging.error("Source and/or target directory doesn't exist")
+        sys.exit(3)
+
+    logging.info('Comparing {} with {}...'.format(source, target))
+    source_dirs, source_files = walk_dir(source)
+    target_dirs, target_files = walk_dir(target)
+
+    dir_list_diff = compare_listings(source_dirs, target_dirs, 'Directories', source, target)
+    file_list_diff = compare_listings(source_files, target_files, 'Files', source, target)
+    file_diff = compare_common_files(set(source_files) & set(target_files), args.channel, source, target)
+
+    if file_diff:
+        # Use status of 2 since python will use 1 if there is an error running the script
+        sys.exit(2)
+    elif dir_list_diff or file_list_diff:
+        # this has traditionally been a WARN, but we don't have files on one side anymore so lets FAIL
+        sys.exit(2)
+    else:
+        logging.info('No differences found')
--- a/release/updates/verify.sh
+++ b/release/updates/verify.sh
@@ -13,16 +13,19 @@ create_cache
 ftp_server_to="http://stage.mozilla.org/pub/mozilla.org"
 ftp_server_from="http://stage.mozilla.org/pub/mozilla.org"
 aus_server="https://aus4.mozilla.org"
 to=""
 to_build_id=""
 to_app_version=""
 to_display_version=""
 diff_summary_log="$PWD/diff-summary.log"
+if [ -e ${diff_summary_log} ]; then
+  rm ${diff_summary_log}
+fi
 touch ${diff_summary_log}
 
 pushd `dirname $0` &>/dev/null
 MY_DIR=$(pwd)
 popd &>/dev/null
 retry="$MY_DIR/../../buildfarm/utils/retry.py -s 1 -r 3"
 
 runmode=0
@@ -217,30 +220,30 @@ do
           continue
         fi
         source_file=`basename "$from_path"`
         target_file=`basename "$to_path"`
         diff_file="results.diff"
         if [ -e ${diff_file} ]; then
           rm ${diff_file}
         fi
-        check_updates "${platform}" "downloads/${source_file}" "downloads/${target_file}" ${locale} ${use_old_updater} ${updater} ${diff_file} ${mar_channel_IDs}
+        check_updates "${platform}" "downloads/${source_file}" "downloads/${target_file}" ${locale} ${use_old_updater} ${updater} ${diff_file} ${channel} ${mar_channel_IDs}
         err=$?
         if [ "$err" == "0" ]; then
           continue
         elif [ "$err" == "1" ]; then
           echo "FAIL: [$release $locale $patch_type] check_updates returned failure for $platform downloads/$source_file vs. downloads/$target_file: $err"
         elif [ "$err" == "2" ]; then
           echo "WARN: [$release $locale $patch_type] check_updates returned warning for $platform downloads/$source_file vs. downloads/$target_file: $err"
         else
           echo "FAIL: [$release $locale $patch_type] check_updates returned unknown error for $platform downloads/$source_file vs. downloads/$target_file: $err"
         fi
 
-        if [ -s $diff_file ]; then
-          echo "Found diffs for ${patch_type} update from ${aus_server}/update/3/${update_path}/update.xml?force=1" >> $diff_summary_log
+        if [ -s ${diff_file} ]; then
+          echo "Found diffs for ${patch_type} update from ${aus_server}/update/3/${update_path}/update.xml?force=1" >> ${diff_summary_log}
           cat ${diff_file} >> ${diff_summary_log}
           echo "" >> ${diff_summary_log}
         fi
       fi
     done
     if [ -f update/partial.size ] && [ -f update/complete.size ]; then
         partial_size=`cat update/partial.size`
         complete_size=`cat update/complete.size`