Bug 948787 - Print diffs during config.status; r=glandium

Build system developers commonly need to see what changes have on the
generated build files. We often put our objdir under version control and
diff commits before and after running config.status.

This patch adds a --diff option to config.status that will print diffs
of changes made during config.status. This functionality is implemented
on top of FileAvoidWrite, using Python's built-in diffing library.

While display of diffs is opt-in, diffs are always being captured when
config.status runs. There could be an unwanted performance regression
from this. Because diffs are only computed if files change and most
files don't change during most config.status runs, this greatly reduces
the surface area of the concern. The area for largest concern is clobber
builds. On my machine, I measured an increase of 0.2 to 0.3s from 2.0s.
While this is 10-15%, the total time is so small that I don't feel
snaking a "capture diff" flag through the build system is worth the
effort. This would make a decent followup bug if this turns out to be a
problem in the future.

I also snuck in a change to reindent all-tests.json because displaying
diffs for this massive 11MB all-in-one-line JSON file results in an
extremely large string being printed to my terminal.

--HG--
extra : rebase_source : c0f7ff69cad282e63a050e67f156dbe96b49a142

build/ConfigStatus.py

@@ -64,7 +64,9 @@ def config_status(topobjdir='.', topsrcdir='.',
                       help='display verbose output')
     parser.add_option('-n', dest='not_topobjdir', action='store_true',
                       help='do not consider current directory as top object directory')
-    (options, args) = parser.parse_args()
+    parser.add_option('-d', '--diff', action='store_true',
+                      help='print diffs of changed files.')
+    options, args = parser.parse_args()
 
     # Without -n, the current directory is meant to be the top object directory
     if not options.not_topobjdir:
@@ -98,3 +100,7 @@ def config_status(topobjdir='.', topsrcdir='.',
 
     for line in summary.summaries():
         print(line, file=sys.stderr)
+
+    if options.diff:
+        for path, diff in sorted(summary.file_diffs.items()):
+            print(diff)

js/src/build/ConfigStatus.py

@@ -64,7 +64,9 @@ def config_status(topobjdir='.', topsrcdir='.',
                       help='display verbose output')
     parser.add_option('-n', dest='not_topobjdir', action='store_true',
                       help='do not consider current directory as top object directory')
-    (options, args) = parser.parse_args()
+    parser.add_option('-d', '--diff', action='store_true',
+                      help='print diffs of changed files.')
+    options, args = parser.parse_args()
 
     # Without -n, the current directory is meant to be the top object directory
     if not options.not_topobjdir:
@@ -98,3 +100,7 @@ def config_status(topobjdir='.', topsrcdir='.',
 
     for line in summary.summaries():
         print(line, file=sys.stderr)
+
+    if options.diff:
+        for path, diff in sorted(summary.file_diffs.items()):
+            print(diff)

python/mozbuild/mozbuild/backend/base.py

@@ -78,6 +78,9 @@ class BackendConsumeSummary(object):
         # backend_execution_time.
         self.other_time = 0.0
 
+        # Mapping of changed file paths to diffs of the changes.
+        self.file_diffs = {}
+
     @property
     def reader_summary(self):
         return 'Finished reading {:d} moz.build files in {:.2f}s'.format(
@@ -276,7 +279,7 @@ class BuildBackend(LoggingMixin):
 
         if path is not None:
             assert fh is None
-            fh = FileAvoidWrite(path)
+            fh = FileAvoidWrite(path, capture_diff=True)
         else:
             assert fh is not None
 
@@ -295,6 +298,8 @@ class BuildBackend(LoggingMixin):
             self.summary.created_count += 1
         elif updated:
             self.summary.updated_count += 1
+            if fh.diff:
+                self.summary.file_diffs[fh.name] = fh.diff
         else:
             self.summary.unchanged_count += 1
 

python/mozbuild/mozbuild/backend/common.py

@@ -125,7 +125,8 @@ class CommonBackend(BuildBackend):
         # Write out a machine-readable file describing every test.
         path = mozpath.join(self.environment.topobjdir, 'all-tests.json')
         with self._write_file(path) as fh:
-            json.dump(self._test_manager.tests_by_path, fh, sort_keys=True)
+            json.dump(self._test_manager.tests_by_path, fh, sort_keys=True,
+                indent=2)
 
     def _create_config_header(self, obj):
         '''Creates the given config header. A config header is generated by

python/mozbuild/mozbuild/backend/recursivemake.py

@@ -90,7 +90,7 @@ class BackendMakeFile(object):
         self.idls = []
         self.xpt_name = None
 
-        self.fh = FileAvoidWrite(self.name)
+        self.fh = FileAvoidWrite(self.name, capture_diff=True)
         self.fh.write('# THIS FILE WAS AUTOMATICALLY GENERATED. DO NOT EDIT.\n')
         self.fh.write('\n')
         self.fh.write('MOZBUILD_DERIVED := 1\n')
@@ -117,6 +117,10 @@ class BackendMakeFile(object):
 
         return self.fh.close()
 
+    @property
+    def diff(self):
+        return self.fh.diff
+
 
 class RecursiveMakeTraversal(object):
     """

python/mozbuild/mozbuild/mach_commands.py

@@ -526,13 +526,20 @@ class Build(MachCommandBase):
 
     @Command('build-backend', category='build',
         description='Generate a backend used to build the tree.')
-    def build_backend(self):
+    @CommandArgument('-d', '--diff', action='store_true',
+        help='Show a diff of changes.')
+    def build_backend(self, diff=False):
         # When we support multiple build backends (Tup, Visual Studio, etc),
         # this command will be expanded to support choosing what to generate.
         python = self.virtualenv_manager.python_path
         config_status = os.path.join(self.topobjdir, 'config.status')
-        return self._run_command_in_objdir(args=[python, config_status],
-            pass_thru=True, ensure_exit_code=False)
+
+        args = [python, config_status]
+        if diff:
+            args.append('--diff')
+
+        return self._run_command_in_objdir(args=args, pass_thru=True,
+            ensure_exit_code=False)
 
 
 @CommandProvider

python/mozbuild/mozbuild/test/test_util.py

@@ -7,7 +7,9 @@ from __future__ import unicode_literals
 import hashlib
 import os
 import unittest
+import shutil
 import sys
+import tempfile
 
 from mozfile.mozfile import NamedTemporaryFile
 from mozunit import (
@@ -108,6 +110,40 @@ class TestFileAvoidWrite(unittest.TestCase):
             faw.write('content')
             self.assertEqual(faw.close(), (True, False))
 
+    def test_diff_not_default(self):
+        """Diffs are not produced by default."""
+
+        faw = FileAvoidWrite('doesnotexist')
+        faw.write('dummy')
+        faw.close()
+        self.assertIsNone(faw.diff)
+
+    def test_diff_update(self):
+        """Diffs are produced on file update."""
+
+        with MockedOpen({'file': 'old'}):
+            faw = FileAvoidWrite('file', capture_diff=True)
+            faw.write('new')
+            faw.close()
+
+            self.assertIsInstance(faw.diff, unicode)
+            self.assertIn('-old', faw.diff)
+            self.assertIn('+new', faw.diff)
+
+    def test_diff_create(self):
+        """Diffs are produced when files are created."""
+
+        tmpdir = tempfile.mkdtemp()
+        try:
+            path = os.path.join(tmpdir, 'file')
+            faw = FileAvoidWrite(path, capture_diff=True)
+            faw.write('new')
+            faw.close()
+
+            self.assertIsInstance(faw.diff, unicode)
+            self.assertIn('+new', faw.diff)
+        finally:
+            shutil.rmtree(tmpdir)
 
 class TestResolveTargetToMake(unittest.TestCase):
     def setUp(self):

python/mozbuild/mozbuild/util.py

@@ -8,6 +8,7 @@
 from __future__ import unicode_literals
 
 import copy
+import difflib
 import errno
 import hashlib
 import os
@@ -115,10 +116,16 @@ class FileAvoidWrite(StringIO):
     it. When we close the file object, if the content in the in-memory buffer
     differs from what is on disk, then we write out the new content. Otherwise,
     the original file is untouched.
+
+    Instances can optionally capture diffs of file changes. This feature is not
+    enabled by default because it a) doesn't make sense for binary files b)
+    could add unwanted overhead to calls.
     """
-    def __init__(self, filename):
+    def __init__(self, filename, capture_diff=False):
         StringIO.__init__(self)
         self.name = filename
+        self._capture_diff = capture_diff
+        self.diff = None
 
     def close(self):
         """Stop accepting writes, compare file contents, and rewrite if needed.
@@ -126,10 +133,16 @@ class FileAvoidWrite(StringIO):
         Returns a tuple of bools indicating what action was performed:
 
             (file existed, file updated)
+
+        If ``capture_diff`` was specified at construction time and the
+        underlying file was changed, ``.diff`` will be populated with the diff
+        of the result.
         """
         buf = self.getvalue()
         StringIO.close(self)
         existed = False
+        old_content = None
+
         try:
             existing = open(self.name, 'rU')
             existed = True
@@ -137,7 +150,8 @@ class FileAvoidWrite(StringIO):
             pass
         else:
             try:
-                if existing.read() == buf:
+                old_content = existing.read()
+                if old_content == buf:
                     return True, False
             except IOError:
                 pass
@@ -148,6 +162,22 @@ class FileAvoidWrite(StringIO):
         with open(self.name, 'w') as file:
             file.write(buf)
 
+        if self._capture_diff:
+            try:
+                old_lines = old_content.splitlines() if old_content else []
+                new_lines = buf.splitlines()
+
+                self.diff = '\n'.join(difflib.unified_diff(old_lines, new_lines,
+                    self.name, self.name, n=4, lineterm=''))
+            # FileAvoidWrite isn't unicode/bytes safe. So, files with non-ascii
+            # content or opened and written in different modes may involve
+            # implicit conversion and this will make Python unhappy. Since
+            # diffing isn't a critical feature, we just ignore the failure.
+            # This can go away once FileAvoidWrite uses io.BytesIO and
+            # io.StringIO. But that will require a lot of work.
+            except (UnicodeDecodeError, UnicodeEncodeError):
+                self.diff = 'Binary or non-ascii file changed: %s' % self.name
+
         return existed, True
 
     def __enter__(self):