Merge ~nacc/git-ubuntu:source_builder-file_contents into git-ubuntu:master

[Nish Aravamudan]

Make jenkins happy

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:d138fb8ee58d7d8c3025a67fe2a33b5dd0c532f5
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/386/
Executed test runs:
    SUCCESS: VM Setup
    FAILED: Build

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/386/rebuild

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:d138fb8ee58d7d8c3025a67fe2a33b5dd0c532f5
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/387/
Executed test runs:
    SUCCESS: VM Setup
    SUCCESS: Build
    SUCCESS: Unit Tests
    SUCCESS: Integration Tests
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/387/rebuild

review: Approve (continuous-integration)

[Andreas Hasenack (ahasenack)]

Comments inline. Some questions, suggestions, test improvements.

review: Needs Fixing

[Nish Aravamudan (nacc)]

On 10.04.2018 [13:26:29 -0000], Andreas Hasenack wrote:
> Review: Needs Fixing
>
> Comments inline. Some questions, suggestions, test improvements.
>
> Diff comments:
>
> > diff --git a/gitubuntu/source_builder.py b/gitubuntu/source_builder.py
> > index f991be3..dc0ca43 100644
> > --- a/gitubuntu/source_builder.py
> > +++ b/gitubuntu/source_builder.py
> > @@ -77,6 +78,10 @@ class SourceSpec:
> > and the changelog is generated with this list of versions instead.
> > changelog_versions[0] is taken to be the version string of the
> > package itself.
> > + :param dict(str -> str) file_contents: a dictionary of file
> > + names to string contents. The source package should contain the
>
> Suggest to clarify that "file names" may have paths. i.e., "a
> dictionary of file names (with relative to the root of the source
> package) to string contents.". phew, that got long and ugly. Would be
> better to find another phrase for it, or just leave it alone really.
> It's just that whenever I see variables like "filename" or "path" I
> wonder if the former can include a path, an if the latter includes the
> file.

Fixed.

>
> > + specified file names with the specified contents. This is used
> > + to control the tree hashes of generated source packages.
> >
> > Keyword arguments to the constructor map directly to class instances
> > properties. Properties may be manipulated after construction.
> > @@ -141,6 +154,27 @@ class SourceFiles:
> > )
> >
> > @property
> > + def spec_files(self):
> > + """The contents of any files explicitly specified in self.spec
> > +
> > + :returns: a mapping of filename to content
> > + :rtype: dict(str, str)
> > + """
> > + if self.spec.file_contents:
> > + reserved_files_specified = [
> > + f for f in self.spec.file_contents
> > + if f in self.reserved_files
> > + ]
> > + if any(reserved_files_specified):
>
> This might be simpler: "if reserved_files_specified:", since bool([])
> is false and the list comprehension will either return [] or a list
> with one or more elements.

Fixed

> > + raise RuntimeError(
> > + "The file names %s are reserved" %
> > + ", ".join(reserved_files_specified)
>
> Shouldn't we run this check at class instantiation time? Why only now,
> when the attribute that was set a "long time ago" in a different class
> even (SourceSpec)? We check the version in SourceSpec, for example
> (see the two ValueErrors there). Also, shouldn't this exception be of
> the ValueError type as well?

Fixed

> > + )
> > + return self.spec.file_contents
> > + else:
> > + return {}
> > +
> > + @property
> > def source_format(self):
> > """The contents of debian/source/format
> >
> > @@ -231,6 +265,12 @@ class Source:
> > encoding='utf-8',
> > ensure=True,
> > )
> > + for (filename, contents) in self.files.spec_files.items():
> > + tree.joi...

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:03bdb46078e2a45f28f51e45ab6c427c2aa5a202
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/388/
Executed test runs:
    SUCCESS: VM Setup
    FAILED: Build

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/388/rebuild

review: Needs Fixing (continuous-integration)

[Andreas Hasenack (ahasenack)]

Thanks! +1, as soon as CI approves it.

review: Approve

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:03bdb46078e2a45f28f51e45ab6c427c2aa5a202
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/389/
Executed test runs:
    SUCCESS: VM Setup
    SUCCESS: Build
    SUCCESS: Unit Tests
    SUCCESS: Integration Tests
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/389/rebuild

review: Approve (continuous-integration)

diff --git a/gitubuntu/source_builder.py b/gitubuntu/source_builder.py
index f991be3..22e8885 100644
--- a/gitubuntu/source_builder.py
+++ b/gitubuntu/source_builder.py
@@ -66,6 +66,12 @@ class SourceSpec:
     native = True
     has_patches = False
     changelog_versions = None
+    file_contents = None
+    reserved_files = [
+        'debian/changelog',
+        'debian/control',
+        'debian/rules',
+    ]
 
     def __init__(self, **kwargs):
         """Instantiate a new SourceSpec class instance
@@ -77,6 +83,11 @@ class SourceSpec:
             and the changelog is generated with this list of versions instead.
             changelog_versions[0] is taken to be the version string of the
             package itself.
+        :param dict(str -> str) file_contents: a dictionary of string
+            relative file path names to string contents. The source
+            package should contain the specified file names with the
+            specified contents.  This is used to control the tree hashes
+            of generated source packages.
 
         Keyword arguments to the constructor map directly to class instances
         properties. Properties may be manipulated after construction.
@@ -89,9 +100,34 @@ class SourceSpec:
         if 'version' not in kwargs and not self.native:
             self.version = '1-1'
 
+        if self.native and '-' in self.version:
+            raise ValueError("Version must not have a '-' in a native package")
+
         if not self.native and '-' not in self.version:
             raise ValueError("Version must have a '-' in a non-native package")
 
+        if self.file_contents is None:
+            self.file_contents = {}
+
+        reserved_files_specified = [
+            f for f in self.file_contents
+            if f in self.reserved_files
+        ]
+        if reserved_files_specified:
+            raise ValueError(
+                "The file names %s are reserved" %
+                ", ".join(reserved_files_specified)
+            )
+
+        absolute_path_files_specified = [
+            f for f in self.file_contents
+            if f.startswith('/')
+        ]
+        if absolute_path_files_specified:
+            raise ValueError(
+                "File names %s must be relative" %
+                ", ".join(absolute_path_files_specified)
+            )
 
 class SourceFiles:
     """Representation of a Debian source package in terms of its files
@@ -141,6 +177,15 @@ class SourceFiles:
         )
 
     @property
+    def spec_files(self):
+        """The contents of any files explicitly specified in self.spec
+
+        :returns: a mapping of filename to content
+        :rtype: dict(str, str)
+        """
+        return self.spec.file_contents
+
+    @property
     def source_format(self):
         """The contents of debian/source/format
 
@@ -231,6 +276,12 @@ class Source:
                 encoding='utf-8',
                 ensure=True,
             )
+        for (filename, contents) in self.files.spec_files.items():
+            tree.join(filename).write_text(
+                contents,
+                encoding='utf-8',
+                ensure=True,
+            )
         debian.join('source/format').write_text(
             self.files.source_format,
             encoding='utf-8',
diff --git a/gitubuntu/source_builder_test.py b/gitubuntu/source_builder_test.py
index 41a95de..81095ba 100644
--- a/gitubuntu/source_builder_test.py
+++ b/gitubuntu/source_builder_test.py
@@ -1,4 +1,6 @@
 import os
+import subprocess
+import tempfile
 
 import pytest
 
@@ -62,6 +64,50 @@ def test_source_create_with_versions(repo):
         assert changelog.all_versions == ['3', '4']
 
 
[e-mail]('file_contents', [
+    {'debian/random': 'Some contents'},
+    {
+        'debian/random': 'Some contents',
+        'debian/subdir/random': 'Some other contents',
+    },
+])
+def test_source_create_with_file_contents(repo, file_contents):
+    with target.Source(
+        target.SourceSpec(file_contents=file_contents)
+    ) as f:
+        with tempfile.TemporaryDirectory() as tmpdir:
+            subprocess.check_call(
+                ['dpkg-source', '-x', f, os.path.join(tmpdir, 'x')],
+            )
+            for _file in file_contents:
+                assert os.path.exists(os.path.join(tmpdir, 'x', _file))
+                with open(
+                    os.path.join(tmpdir, 'x', _file)
+                ) as random_file:
+                    assert random_file.read() == file_contents[_file]
+
+
[e-mail]('_file', [
+    'debian/changelog',
+    'debian/control',
+    'debian/rules',
+])
+def test_source_create_fails_with_reserved_file_contents(repo, _file):
+    with pytest.raises(ValueError):
+        with target.Source(
+            target.SourceSpec(file_contents={_file: 'Some contents'})
+        ) as f:
+            pass
+
+
+def test_source_create_fails_with_absolute_path(repo):
+    with pytest.raises(ValueError):
+        with target.Source(
+            target.SourceSpec(file_contents={'/root/path': 'Some contents'})
+        ) as f:
+            pass
+
+
 @pytest.mark.parametrize('native,expected', [
     (True, b"3.0 (native)\n"),
     (False, b"3.0 (quilt)\n"),
@@ -126,35 +172,48 @@ def test_source_quilt_with_patches_applied(repo):
 
 
 def test_source_version_native_default(repo):
-    # The default version string for a native package should not have a '-' in
-    # it.
+    """The default version string for a native package should not have a
+    '-' in it.
+    """
     version = get_spec_changelog_version(repo, native=True)
     assert '-' not in version
 
 
 def test_source_version_non_native_default(repo):
-    # The default version string for a non-native package should have a '-' in
-    # it.
+    """The default version string for a non-native package should have a
+    '-' in it.
+    """
     version = get_spec_changelog_version(repo, native=False)
     assert '-' in version
 
 
 def test_source_version_native_specific(repo):
-    # We should be able to create a native package with a native-looking
-    # version string.
+    """We should be able to create a native package with a
+    native-looking version string.
+    """
     version = get_spec_changelog_version(repo, native=True, version='1.0')
     assert version == '1.0'
 
 
 def test_source_version_non_native_specific(repo):
-    # We should be able to create a non-native package with a non-native
-    # -looking version string.
+    """We should be able to create a non-native package with a
+    non-native-looking version string.
+    """
     version = get_spec_changelog_version(repo, native=False, version='1.0-1')
     assert version == '1.0-1'
 
 
 def test_source_non_native_version_no_hyphen_raises(repo):
-    # Creating a non-native package with a native-looking version string should
-    # fail.
+    """Creating a non-native package with a native-looking version
+    string should fail.
+    """
     with pytest.raises(ValueError):
         get_spec_changelog_version(repo, native=False, version='1')
+
+
+def test_source_native_version_hyphen_raises(repo):
+    """Creating a native package with a non-native-looking version
+    string should fail.
+    """
+    with pytest.raises(ValueError):
+        print(get_spec_changelog_version(repo, native=True, version='1-1'))