[Notes] [Git][BuildStream/buildstream][aevri/mtime1] storage.Directory.e

Angelos Evripiotis pushed to branch aevri/mtime1 at BuildStream / buildstream

Commits:

ed49da18

by Angelos Evripiotis at 2019-02-14T14:41:42Z

storage.Directory.export_to_tar: mtime=_magic...

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 changed files:

buildstream/storage/_casbaseddirectory.py
buildstream/storage/_filebaseddirectory.py
buildstream/storage/directory.py
tests/frontend/buildcheckout.py

Changes:

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():
@@ -718,7 +718,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):

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)):

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:

tests/frontend/buildcheckout.py

@@ -252,6 +252,26 @@ def test_build_checkout_tarball_stdout(datafiles, cli):
      assert os.path.join('.', 'usr', 'include', 'pony.h') in tar.getnames()
 +@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)

[Notes] [Git][BuildStream/buildstream][aevri/mtime1] storage.Directory.export_to_tar: mtime=_magic...

Angelos Evripiotis pushed to branch aevri/mtime1 at BuildStream / buildstream

Commits:

4 changed files:

Changes: