[Notes] [Git][BuildStream/buildstream][bschubert/set-as-set] 3 commits:

Benjamin Schubert pushed to branch bschubert/set-as-set at BuildStream / buildstream

Commits:

39febfda

by Angelos Evripiotis at 2019-02-15T11:10:51Z

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/

8b34e356

by Angelos Evripiotis at 2019-02-15T12:25:21Z

Merge branch 'aevri/mtime1' into 'master'

storage.Directory.export_to_tar: default mtime=utils._magic_timestamp

Closes #914

See merge request BuildStream/buildstream!1149

3bbb90e7

by Benjamin Schubert at 2019-02-15T12:34:36Z

Use sets when checking for existence of an element

7 changed files:

buildstream/_loader/loader.py
buildstream/_profile.py
buildstream/plugins/sources/tar.py
buildstream/storage/_casbaseddirectory.py
buildstream/storage/_filebaseddirectory.py
buildstream/storage/directory.py
tests/frontend/buildcheckout.py

Changes:

buildstream/_loader/loader.py

@@ -284,17 +284,17 @@ class Loader():
      def _check_circular_deps(self, element, check_elements=None, validated=None, sequence=None):
          if check_elements is None:
 -            check_elements = {}
 +            check_elements = set()
          if validated is None:
 -            validated = {}
 +            validated = set()
          if sequence is None:
              sequence = []
          # Skip already validated branches
 -        if validated.get(element) is not None:
 +        if element in validated:
              return
 -        if check_elements.get(element) is not None:
 +        if element in check_elements:
              # Create `chain`, the loop of element dependencies from this
              # element back to itself, by trimming everything before this
              # element from the sequence under consideration.
@@ -306,15 +306,15 @@ class Loader():
                              .format(element.full_name, " -> ".join(chain)))
          # Push / Check each dependency / Pop
 -        check_elements[element] = True
 +        check_elements.add(element)
          sequence.append(element.full_name)
          for dep in element.dependencies:
              dep.element._loader._check_circular_deps(dep.element, check_elements, validated, sequence)
 -        del check_elements[element]
 +        check_elements.remove(element)
          sequence.pop()
          # Eliminate duplicate paths
 -        validated[element] = True
 +        validated.add(element)
      # _sort_dependencies():
+     #

buildstream/_profile.py

@@ -26,7 +26,7 @@ import datetime
  import time
  # Track what profile topics are active
 -active_topics = {}
 +active_topics = set()
  active_profiles = {}
  initialized = False
@@ -144,14 +144,10 @@ def profile_init():
          if setting:
              topics = setting.split(':')
              for topic in topics:
 -                active_topics[topic] = True
 +                active_topics.add(topic)
          initialized = True
  def profile_enabled(topic):
      profile_init()
 -    if active_topics.get(topic):
 -        return True
 -    if active_topics.get(Topics.ALL):
 -        return True
 -    return False
 +    return topic in active_topics or Topics.ALL in active_topics

buildstream/plugins/sources/tar.py

@@ -154,7 +154,7 @@ class TarSource(DownloadableFileSource):
      # directory paths for the archived files.
      def _list_tar_paths(self, tar):
 -        visited = {}
 +        visited = set()
          for member in tar.getmembers():
              # Remove any possible leading './', offer more consistent behavior
@@ -170,7 +170,7 @@ class TarSource(DownloadableFileSource):
                  for i in range(len(components) - 1):
                      dir_component = '/'.join([components[j] for j in range(i + 1)])
                      if dir_component not in visited:
 -                        visited[dir_component] = True
 +                        visited.add(dir_component)
                          try:
                              # Dont yield directory members which actually do
                              # exist in the archive

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

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][bschubert/set-as-set] 3 commits: storage.Directory.export_to_tar: mtime=_magic...

Benjamin Schubert pushed to branch bschubert/set-as-set at BuildStream / buildstream

Commits:

7 changed files:

Changes: