storage.Directory.export_to_tar: mtime=_magic...aevri/mtime1

Change the default value of mtime when doing export_to_tar() from `0` to
`_utils._magic_timestamp`. This avoids problems in other software, which
assume that an mtime of `0` means the file does not exist.

There is more than one example where files with an mtime of zero are
treated as non-existant by other software. e.g. [ninja][1] and [Template
Toolkit][2].

The OSTree project also [express an intention][3] to move from an mtime
of 0 to an mtime of 1:

> For this reason, OSTree acts as though all timestamps are set to
> time_t 0, so that comparisons will be considered up-to-date. Note that
> for a few releases, OSTree used 1 to fix warnings such as GNU Tar
> emitting "implausibly old time stamp" with 0; however, until we have a
> mechanism to transition cleanly to 1, for compatibilty OSTree is
> reverted to use zero again.

From the comments on export_to_tar(), the motivation for having an mtime
of 0 was to have reproducible results, rather than it specifically being
the value 0.

Additionally, the reproducible builds project has a [page on
archives][4]; it mentions the benefits of setting all the files to have
the same mtime, or clamping the mtime. It makes no mention of a
motivation for the mtime to be specifically 0.

Fixes #914

[1]: https://github.com/ninja-build/ninja/issues/1120
[2]: https://github.com/abw/Template2/blob/8d7d37200af436f1ad43628278d3caad257c8e27/lib/Template/Provider.pm#L635
[3]: https://ostree.readthedocs.io/en/latest/manual/repo/
[4]: https://reproducible-builds.org/docs/archives/

4 files changed, 25 insertions, 4 deletions

diff --git a/buildstream/storage/_casbaseddirectory.py b/buildstream/storage/_casbaseddirectory.py
index b8a247134..96a7aa79d 100644
--- a/buildstream/storage/_casbaseddirectory.py
+++ b/buildstream/storage/_casbaseddirectory.py
@@ -36,7 +36,7 @@ from .._protos.build.bazel.remote.execution.v2 import remote_execution_pb2
 from .._exceptions import BstError
 from .directory import Directory, VirtualDirectoryError
 from ._filebaseddirectory import FileBasedDirectory
-from ..utils import FileListResult, safe_copy, list_relative_paths
+from ..utils import FileListResult, safe_copy, list_relative_paths, _magic_timestamp
 
 
 class IndexEntry():
@@ -535,7 +535,7 @@ class CasBasedDirectory(Directory):
                                 " The original error was: {}").
                                format(src_name, entry.target, e))
 
-    def export_to_tar(self, tarfile, destination_dir, mtime=0):
+    def export_to_tar(self, tarfile, destination_dir, mtime=_magic_timestamp):
         raise NotImplementedError()
 
     def mark_changed(self):
diff --git a/buildstream/storage/_filebaseddirectory.py b/buildstream/storage/_filebaseddirectory.py
index 735179cb6..b919413f0 100644
--- a/buildstream/storage/_filebaseddirectory.py
+++ b/buildstream/storage/_filebaseddirectory.py
@@ -157,7 +157,7 @@ class FileBasedDirectory(Directory):
     # First, it sorts the results of os.listdir() to ensure the ordering of
     # the files in the archive is the same.  Second, it sets a fixed
     # timestamp for each entry. See also https://bugs.python.org/issue24465.
-    def export_to_tar(self, tf, dir_arcname, mtime=0):
+    def export_to_tar(self, tf, dir_arcname, mtime=_magic_timestamp):
         # We need directories here, including non-empty ones,
         # so list_relative_paths is not used.
         for filename in sorted(os.listdir(self.external_directory)):
diff --git a/buildstream/storage/directory.py b/buildstream/storage/directory.py
index 838741231..66b93a7f1 100644
--- a/buildstream/storage/directory.py
+++ b/buildstream/storage/directory.py
@@ -32,6 +32,7 @@ See also: :ref:`sandboxing`.
 """
 
 from .._exceptions import BstError, ErrorDomain
+from ..utils import _magic_timestamp
 
 
 class VirtualDirectoryError(BstError):
@@ -114,7 +115,7 @@ class Directory():
 
         raise NotImplementedError()
 
-    def export_to_tar(self, tarfile, destination_dir, mtime=0):
+    def export_to_tar(self, tarfile, destination_dir, mtime=_magic_timestamp):
         """ Exports this directory into the given tar file.
 
         Args:
diff --git a/tests/frontend/buildcheckout.py b/tests/frontend/buildcheckout.py
index 80d710f6f..598edbbbc 100644
--- a/tests/frontend/buildcheckout.py
+++ b/tests/frontend/buildcheckout.py
@@ -253,6 +253,26 @@ def test_build_checkout_tarball_stdout(datafiles, cli):
 
 
 @pytest.mark.datafiles(DATA_DIR)
+def test_build_checkout_tarball_mtime_nonzero(datafiles, cli):
+    project = os.path.join(datafiles.dirname, datafiles.basename)
+    tarpath = os.path.join(cli.directory, 'mtime_tar.tar')
+
+    result = cli.run(project=project, args=['build', 'target.bst'])
+    result.assert_success()
+
+    checkout_args = ['artifact', 'checkout', '--tar', tarpath, 'target.bst']
+    result = cli.run(project=project, args=checkout_args)
+    result.assert_success()
+
+    tar = tarfile.TarFile(tarpath)
+    for tarinfo in tar.getmembers():
+        # An mtime of zero can be confusing to other software,
+        # e.g. ninja build and template toolkit have both taken zero mtime to
+        # mean 'file does not exist'.
+        assert tarinfo.mtime > 0
+
+
+@pytest.mark.datafiles(DATA_DIR)
 def test_build_checkout_tarball_is_deterministic(datafiles, cli):
     project = os.path.join(datafiles.dirname, datafiles.basename)
     tarball1 = os.path.join(cli.directory, 'tarball1.tar')