Properly format the deb Description field, fix format of changes file. (

#600)

* Properly format the deb Description field, fix format of changes file.

A combination of fixes and then tests for the behavior:
- Stop text wrapping the description. The "displayer" should do the wrapping.
- Create the changes "Description" field in the correct format.
- Do not allow newline in single line fields.
- Add leading space to continuation lines in multiline fields.

https://www.debian.org/doc/debian-policy/ch-controlfields.html

Fixes: #522

* do not test description on windows

pkg/private/deb/BUILD

@@ -16,7 +16,7 @@
 All interfaces are subject to change at any time.
 """
 
-load("@rules_python//python:defs.bzl", "py_library")
+load("@rules_python//python:defs.bzl", "py_binary", "py_library")
 
 package(default_applicable_licenses = ["//:license"])
 
@@ -54,3 +54,13 @@ py_binary(
         "//pkg/private:helpers",
     ],
 )
+
+py_library(
+    name = "make_deb_lib",
+    srcs = ["make_deb.py"],
+    srcs_version = "PY3",
+    visibility = ["//tests/deb:__pkg__"],
+    deps = [
+        "//pkg/private:helpers",
+    ],
+)

pkg/private/deb/make_deb.py

@@ -30,7 +30,7 @@
 
 from pkg.private import helpers
 
-# list of debian fields : (name, mandatory, wrap[, default])
+# list of debian fields : (name, mandatory, is_multiline[, default])
 # see http://www.debian.org/doc/debian-policy/ch-controlfields.html
 
 DEBIAN_FIELDS = [
@@ -117,21 +117,34 @@ def AddArFileEntry(fileobj, filename,
     fileobj.write(b'\n')  # 2-byte alignment padding
 
 
-def MakeDebianControlField(name, value, wrap=False):
-  """Add a field to a debian control file."""
-  result = name + ': '
+def MakeDebianControlField(name: str, value: str, is_multiline:bool=False) -> str:
+  """Add a field to a debian control file.
+
+  https://www.debian.org/doc/debian-policy/ch-controlfields.html#syntax-of-control-files
+
+  Args:
+    name: Control field name
+    value: Value for that
+  """
   if isinstance(value, bytes):
     value = value.decode('utf-8')
   if isinstance(value, list):
     value = u', '.join(value)
-  if wrap:
-    result += u' '.join(value.split('\n'))
-    result = textwrap.fill(result,
-                           break_on_hyphens=False,
-                           break_long_words=False)
-  else:
-    result += value
-  return result.replace(u'\n', u'\n ') + u'\n'
+  value = value.rstrip()
+  if not is_multiline:
+    value = value.strip()
+    if '\n' in value:
+      raise ValueError(
+          '\\n is not allowed in simple control fields (%s)' % value)
+
+  lines = value.split('\n')
+  result = name + ': ' +lines[0].strip() + '\n'
+  for line in lines[1:]:
+    if not line.startswith(' '):
+      result += ' '
+    result += line
+    result += '\n'
+  return result
 
 
 def CreateDebControl(extrafiles=None, **kwargs):
@@ -140,9 +153,11 @@ def CreateDebControl(extrafiles=None, **kwargs):
   controlfile = u''
   for values in DEBIAN_FIELDS:
     fieldname = values[0]
+    mandatory = values[1]
+    is_multiline = values[2]
     key = fieldname[0].lower() + fieldname[1:].replace('-', '')
-    if values[1] or (key in kwargs and kwargs[key]):
-      controlfile += MakeDebianControlField(fieldname, kwargs[key], values[2])
+    if mandatory or (key in kwargs and kwargs[key]):
+      controlfile += MakeDebianControlField(fieldname, kwargs[key], is_multiline)
   # Create the control.tar file
   tar = io.BytesIO()
   with gzip.GzipFile('control.tar.gz', mode='w', fileobj=tar, mtime=0) as gz:
@@ -246,7 +261,7 @@ def GetChecksumsFromFile(filename, hash_fns=None):
 def CreateChanges(output,
                   deb_file,
                   architecture,
-                  short_description,
+                  description,
                   maintainer,
                   package,
                   version,
@@ -273,21 +288,22 @@ def CreateChanges(output,
       MakeDebianControlField('Urgency', urgency),
       MakeDebianControlField('Maintainer', maintainer),
       MakeDebianControlField('Changed-By', maintainer),
-      MakeDebianControlField('Description',
-                             '\n%s - %s' % (package, short_description)),
-      MakeDebianControlField('Changes',
-                             ('\n%s (%s) %s; urgency=%s'
-                              '\nChanges are tracked in revision control.') %
-                             (package, version, distribution, urgency)),
+      # The description in the changes file is strange
+      'Description:\n %s - %s\n' % (package, description.split('\n')[0]),
+      MakeDebianControlField('Changes', (
+          '\n %s (%s) %s; urgency=%s'
+          '\n Changes are tracked in revision control.') % (
+              package, version, distribution, urgency),
+          is_multiline=True),
       MakeDebianControlField(
-          'Files', '\n' + ' '.join(
+          'Files', '\n ' + ' '.join(
               [checksums['md5'], debsize, section, priority, deb_basename])),
       MakeDebianControlField(
           'Checksums-Sha1',
-          '\n' + ' '.join([checksums['sha1'], debsize, deb_basename])),
+          '\n ' + ' '.join([checksums['sha1'], debsize, deb_basename])),
       MakeDebianControlField(
           'Checksums-Sha256',
-          '\n' + ' '.join([checksums['sha256'], debsize, deb_basename]))
+          '\n ' + ' '.join([checksums['sha256'], debsize, deb_basename]))
   ])
   with open(output, 'wb') as changes_fh:
     changes_fh.write(changesdata.encode('utf-8'))
@@ -373,7 +389,7 @@ def main():
       output=options.changes,
       deb_file=options.output,
       architecture=options.architecture,
-      short_description=helpers.GetFlagValue(options.description).split('\n')[0],
+      description=helpers.GetFlagValue(options.description),
       maintainer=helpers.GetFlagValue(options.maintainer), package=options.package,
       version=helpers.GetFlagValue(options.version), section=options.section,
       priority=options.priority, distribution=options.distribution,

tests/deb/BUILD

@@ -76,7 +76,7 @@ pkg_deb(
         "dep1",
         "dep2",
     ],
-    description = "toto ®, Й, ק ,م, ๗, あ, 叶, 葉, 말, ü and é",
+    description = "toto ®, Й, ק ,م, ๗, あ, 叶, 葉, 말, ü and é\n more",
     distribution = "trusty",
     maintainer = "somé[email protected]",
     package = "fizzbuzz",
@@ -86,7 +86,7 @@ pkg_deb(
     templates = "templates",
     triggers = "deb_triggers",
     urgency = "low",
-    version = "test",
+    version = "4.5.6",
 )
 
 py_test(
@@ -108,6 +108,18 @@ py_test(
 
 package_naming_test(
     name = "naming_test",
-    expected_name = "fizzbuzz_test_all.deb",
+    expected_name = "fizzbuzz_4.5.6_all.deb",
     target_under_test = ":test_deb",
 )
+
+py_test(
+    name = "control_field_test",
+    size = "small",
+    srcs = [
+        "control_field_test.py",
+    ],
+    python_version = "PY3",
+    deps = [
+        "//pkg/private/deb:make_deb_lib",
+    ],
+)

tests/deb/control_field_test.py

@@ -0,0 +1,75 @@
+# Copyright 2022 The Bazel Authors. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+# -*- coding: utf-8 -*-
+"""Testing for archive."""
+
+import codecs
+from io import BytesIO
+import os
+import sys
+import tarfile
+import unittest
+
+from pkg.private.deb import make_deb
+
+class MakeControlFieldTest(unittest.TestCase):
+  """Tests for MakeControlField.
+
+  https://www.debian.org/doc/debian-policy/ch-controlfields.html#syntax-of-control-files
+  """
+
+  def test_simple(self):
+    self.assertEqual(
+        'Package: fizzbuzz\n',
+        make_deb.MakeDebianControlField('Package', 'fizzbuzz'))
+
+  def test_simple_strip(self):
+    self.assertEqual(
+        'Package: fizzbuzz\n',
+        make_deb.MakeDebianControlField('Package', ' fizzbuzz'))
+    self.assertEqual(
+        'Package: fizzbuzz\n',
+        make_deb.MakeDebianControlField('Package', ' fizzbuzz '))
+
+  def test_simple_no_newline(self):
+    with self.assertRaises(ValueError):
+      make_deb.MakeDebianControlField('Package', ' fizz\nbuzz ')
+
+
+  def test_multiline(self):
+    self.assertEqual(
+        'Description: fizzbuzz\n',
+        make_deb.MakeDebianControlField(
+            'Description', 'fizzbuzz', is_multiline=True))
+    self.assertEqual(
+        'Description: fizz\n buzz\n',
+        make_deb.MakeDebianControlField(
+            'Description', 'fizz\n buzz\n', is_multiline=True))
+
+  def test_multiline_add_required_space(self):
+    self.assertEqual(
+        'Description: fizz\n buzz\n',
+        make_deb.MakeDebianControlField(
+            'Description', 'fizz\nbuzz', is_multiline=True))
+
+  def test_multiline_add_trailing_newline(self):
+    self.assertEqual(
+        'Description: fizz\n buzz\n baz\n',
+        make_deb.MakeDebianControlField(
+            'Description', 'fizz\n buzz\n baz', is_multiline=True))
+
+
+if __name__ == '__main__':
+  unittest.main()

tests/deb/pkg_deb_test.py

@@ -63,7 +63,7 @@ def setUp(self):
     super(PkgDebTest, self).setUp()
     self.runfiles = runfiles.Create()
     # Note: Rlocation requires forward slashes. os.path.join() will not work.
-    self.deb_path = self.runfiles.Rlocation('rules_pkg/tests/deb/fizzbuzz_test_all.deb')
+    self.deb_path = self.runfiles.Rlocation('rules_pkg/tests/deb/fizzbuzz_4.5.6_all.deb')
     self.deb_file = DebInspect(self.deb_path)
 
   def assert_control_content(self, expected, match_order=False):
@@ -163,7 +163,7 @@ def test_description(self):
     # pass on Windows Until we rewrite how description is passed
     if sys.platform != 'win32':
       fields.extend([
-	  'Description: toto ®, Й, ק ,م, ๗, あ, 叶, 葉, 말, ü and é',
+	  'Description: toto ®, Й, ק ,م, ๗, あ, 叶, 葉, 말, ü and é\n more\n',
 	  'Maintainer: somé[email protected]',
       ])
     for field in fields:
@@ -214,7 +214,7 @@ def test_triggers(self):
 
   def test_changes(self):
     changes_path = self.runfiles.Rlocation(
-        'rules_pkg/tests/deb/fizzbuzz_test_all.changes')
+        'rules_pkg/tests/deb/fizzbuzz_4.5.6_all.changes')
     with open(changes_path, 'r', encoding='utf-8') as f:
       content = f.read()
       for field in (
@@ -223,6 +223,31 @@ def test_changes(self):
         if content.find(field) < 0:
           self.fail('Missing template field: <%s> in <%s>' % (field, content))
 
+      # TODO(https://github.com/bazelbuild/rules_pkg/issues/214): This can not
+      # pass on Windows until we rewrite how description is passed.
+      if sys.platform == 'win32':
+        return
+
+      # From the spec:
+      #   In a .changes file, the Description field contains a summary of the
+      #   descriptions for the packages being uploaded. For this case, the first
+      #   line of the field value (the part on the same line as Description:) is
+      #   always empty. It is a multiline field, with one line per package. Each
+      #   line is indented by one space and contains the name of a binary package,
+      #   a space, a hyphen (-), a space, and the short description line from that
+      #   package.
+      d_expect = 'Description:\n fizzbuzz - toto ®, Й, ק ,م, ๗, あ, 叶, 葉, 말, ü and é\n'
+      d_start = content.find(d_expect)
+      if d_start < 0:
+        self.fail(
+            'Could not find expected description (%s) in\n=====%s=====' %
+            (d_expect, content))
+      # Check that the next line is the start of a new description, rather than
+      # a continuation.
+      self.assertTrue(
+          content[d_start + len(d_expect)].isupper(),
+          'Description has unexpected characters at end (%s)' % content)
+
 
 if __name__ == '__main__':
   unittest.main()