Fix rpm tests for newer rpmbuild (fixes #3164)

Two issues: (1) ARCHITECTURE, if supplied, actually needs to be set
in the specfile BuildArch: field (or passed on the command line).
(2) rpmbuild on Fedora builds all kind of debug stuff behind the
scenes (it makes packages not requested by the specfile!). This
causes scons rpm tests to fail, so force in a flag to turn off this
generation. That is done unconditionally at the moment by writing

  %global debug_package %{nil}\n

to the specfile.  If anyone eventually ends up needing to produce
these debug packages through scons, we can add a flag to allow
it by omitting the above setting.

Some doc and comment cleanup done, and one of the tests now builds
a noarch package to test actually supplying an ARCHITECTURE value
that is not the machine default. Also, one of the tools called by
rpm packaging did not supply the right suffix for the Tar builder,
presumably a copy-paste error in a place that is not stressed by
any test.

Fedora packaging now also generates a file containing the build id
and puts it into the package (another behind the scenes bit of magic -
this does not appear in our %files section but is still added to the
package). It is possible to turn this behavior off as well, but for the
tests affected, the choice was made instead to have them now check the
files in the generated package /contain/ the expected filenames rather
than /exactly-match/ the expected filenames.

Testing: on recent Fedora all the rpm-related tests now pass after six
failed previously; on a Centos 7.4 system which did not fail any tests,
the modified code still passes - did not introduce regressions. At
the moment do not believe any of the CI setup will cause rpm tests
to be run.

Signed-off-by: Mats Wichmann <mats@linux.com>

8 files changed, 56 insertions, 33 deletions

diff --git a/src/CHANGES.txt b/src/CHANGES.txt
index 6534f3a..8be3bbe 100644
--- a/src/CHANGES.txt
+++ b/src/CHANGES.txt
@@ -120,6 +120,10 @@ RELEASE 3.1.0.alpha.yyyymmdd - NEW DATE WILL BE INSERTED HERE
     - quiet py3 warning in UtilTests.py
     - fix tests specifying octal constants for py3
     - fix must_contain tests for py3
+    - rpm package generation: fix supplying a build architecture; disable
+      auto debug package generation on certain rpmbuild versions; adjust some tests
+      to be resistant to automatic supplying of build-id file on certain rpmbuild
+      versions; a little doc and comment cleanup.
     
   From Hao Wu
     - typo in customized decider example in user guide 
diff --git a/src/engine/SCons/Tool/packaging/__init__.xml b/src/engine/SCons/Tool/packaging/__init__.xml
index e4b1865..68e4fed 100644
--- a/src/engine/SCons/Tool/packaging/__init__.xml
+++ b/src/engine/SCons/Tool/packaging/__init__.xml
@@ -106,7 +106,9 @@ This is used to fill in the
 <literal>Architecture:</literal>
 field in an Ipkg
 <filename>control</filename> file,
-and as part of the name of a generated RPM file.
+and the <literal>BuildArch:</literal> field
+in the RPM <filename>.spec</filename> file,
+as well as forming part of the name of a generated RPM package file.
 </para>
 </summary>
 </cvar>
@@ -120,7 +122,6 @@ the <filename>control</filename> for Ipkg,
 the <filename>.wxs</filename> for MSI).
 If set, the function will be called
 after the SCons template for the file has been written.
-XXX
 </para>
 </summary>
 </cvar>
@@ -164,10 +165,10 @@ section of an RPM
 <cvar name="LICENSE">
 <summary>
 <para>
-The abbreviated name of the license under which
-this project is released (gpl, lpgl, bsd etc.).
+The abbreviated name, preferably the SPDX code, of the license under which
+this project is released (GPL-3.0, LGPL-2.1, BSD-2-Clause etc.).
 See http://www.opensource.org/licenses/alphabetical
-for a list of license names.
+for a list of license names and SPDX codes.
 </para>
 </summary>
 </cvar>
@@ -383,6 +384,7 @@ This is used to fill in the
 <literal>BuildRequires:</literal>
 field in the RPM
 <filename>.spec</filename> file.
+Note this should only be used on a host managed by rpm as the dependencies will not be resolvable at build time otherwise.
 </para>
 </summary>
 </cvar>
@@ -441,7 +443,8 @@ field in the RPM
 <para>
 This is used to fill in the
 <literal>Epoch:</literal>
-field in the controlling information for RPM packages.
+field in the RPM
+<filename>.spec</filename> file.
 </para>
 </summary>
 </cvar>
diff --git a/src/engine/SCons/Tool/packaging/rpm.py b/src/engine/SCons/Tool/packaging/rpm.py
index a3481a4..e93c4f0 100644
--- a/src/engine/SCons/Tool/packaging/rpm.py
+++ b/src/engine/SCons/Tool/packaging/rpm.py
@@ -51,10 +51,9 @@ def package(env, target, source, PACKAGEROOT, NAME, VERSION,
     if str(target[0])!="%s-%s"%(NAME, VERSION):
         raise UserError( "Setting target is not supported for rpm." )
     else:
-        # This should be overridable from the construction environment,
-        # which it is by using ARCHITECTURE=.
+        # Deduce the build architecture, but allow it to be overridden
+        # by setting ARCHITECTURE in the construction env.
         buildarchitecture = SCons.Tool.rpmutils.defaultMachine()
-
         if 'ARCHITECTURE' in kw:
             buildarchitecture = kw['ARCHITECTURE']
 
@@ -126,20 +125,18 @@ def build_specfile(target, source, env):
     """ Builds a RPM specfile from a dictionary with string metadata and
     by analyzing a tree of nodes.
     """
-    file = open(target[0].get_abspath(), 'w')
-
-    try:
-        file.write( build_specfile_header(env) )
-        file.write( build_specfile_sections(env) )
-        file.write( build_specfile_filesection(env, source) )
-        file.close()
+    with open(target[0].get_abspath(), 'w') as file:
+        try:
+            file.write(build_specfile_header(env))
+            file.write(build_specfile_sections(env))
+            file.write(build_specfile_filesection(env, source))
 
-        # call a user specified function
-        if 'CHANGE_SPECFILE' in env:
-            env['CHANGE_SPECFILE'](target, source)
+            # call a user specified function
+            if 'CHANGE_SPECFILE' in env:
+                env['CHANGE_SPECFILE'](target, source)
 
-    except KeyError as e:
-        raise SCons.Errors.UserError( '"%s" package field for RPM is missing.' % e.args[0] )
+        except KeyError as e:
+            raise SCons.Errors.UserError('"%s" package field for RPM is missing.' % e.args[0])
 
 
 #
@@ -211,6 +208,7 @@ def build_specfile_header(spec):
         'X_RPM_URL'           : 'Url: %s\n',
         'SOURCE_URL'          : 'Source: %s\n',
         'SUMMARY_'            : 'Summary(%s): %s\n',
+        'ARCHITECTURE'        : 'BuildArch: %s\n',
         'X_RPM_DISTRIBUTION'  : 'Distribution: %s\n',
         'X_RPM_ICON'          : 'Icon: %s\n',
         'X_RPM_PACKAGER'      : 'Packager: %s\n',
@@ -232,16 +230,23 @@ def build_specfile_header(spec):
         'X_RPM_BUILDROOT'     : 'BuildRoot: %s\n', }
 
     # fill in default values:
-    # Adding a BuildRequires renders the .rpm unbuildable under System, which
+    # Adding a BuildRequires renders the .rpm unbuildable under systems which
     # are not managed by rpm, since the database to resolve this dependency is
     # missing (take Gentoo as an example)
-#    if not s.has_key('x_rpm_BuildRequires'):
-#        s['x_rpm_BuildRequires'] = 'scons'
+    #if 'X_RPM_BUILDREQUIRES' not in spec:
+    #    spec['X_RPM_BUILDREQUIRES'] = 'scons'
 
     if 'X_RPM_BUILDROOT' not in spec:
         spec['X_RPM_BUILDROOT'] = '%{_tmppath}/%{name}-%{version}-%{release}'
 
     str = str + SimpleTagCompiler(optional_header_fields, mandatory=0).compile( spec )
+
+    # github #3164: if we don't turn off debug package generation
+    # the tests which build packages all fail.  This just slams
+    # a flag into the specfile header, could make it an X_RPM_NODEBUGPKGS
+    # which is controllable by an environment setting.
+    str += '%global debug_package %{nil}\n'
+
     return str
 
 #
diff --git a/src/engine/SCons/Tool/packaging/tarbz2.py b/src/engine/SCons/Tool/packaging/tarbz2.py
index 2e38da2..3806e76 100644
--- a/src/engine/SCons/Tool/packaging/tarbz2.py
+++ b/src/engine/SCons/Tool/packaging/tarbz2.py
@@ -32,7 +32,7 @@ from SCons.Tool.packaging import stripinstallbuilder, putintopackageroot
 
 def package(env, target, source, PACKAGEROOT, **kw):
     bld = env['BUILDERS']['Tar']
-    bld.set_suffix('.tar.gz')
+    bld.set_suffix('.tar.bz2')
     target, source = putintopackageroot(target, source, env, PACKAGEROOT)
     target, source = stripinstallbuilder(target, source, env)
     return bld(env, target, source, TARFLAGS='-jc')
diff --git a/test/packaging/option--package-type.py b/test/packaging/option--package-type.py
index c8f22ca..7ff8535 100644
--- a/test/packaging/option--package-type.py
+++ b/test/packaging/option--package-type.py
@@ -26,6 +26,9 @@ __revision__ = "__FILE__ __REVISION__ __DATE__ __DEVELOPER__"
 
 """
 Test the --package-type option.
+
+Side effect: also tests that we can produce a noarch package
+by supplying the ARCHITECTURE tag.
 """
 
 import TestSCons
@@ -63,12 +66,13 @@ env.Package( NAME           = 'foo',
              X_RPM_INSTALL  = r'%(_python_)s %(scons)s --install-sandbox="$RPM_BUILD_ROOT" "$RPM_BUILD_ROOT"',
              DESCRIPTION    = 'this should be really long',
              source         = [ prog ],
-             SOURCE_URL     = 'http://foo.org/foo-1.2.3.tar.gz'
+             SOURCE_URL     = 'http://foo.org/foo-1.2.3.tar.gz',
+             ARCHITECTURE   = 'noarch'
             )
 """ % locals())
 
 src_rpm = 'foo-1.2.3-0.src.rpm'
-machine_rpm = 'foo-1.2.3-0.%s.rpm' % SCons.Tool.rpmutils.defaultMachine()
+machine_rpm = 'foo-1.2.3-0.noarch.rpm'
 
 test.run(arguments='package PACKAGETYPE=rpm', stderr = None)
 
diff --git a/test/packaging/rpm/multipackage.py b/test/packaging/rpm/multipackage.py
index ab8734d..f756cf4 100644
--- a/test/packaging/rpm/multipackage.py
+++ b/test/packaging/rpm/multipackage.py
@@ -109,8 +109,10 @@ test.must_exist( machine_rpm2 )
 test.must_exist( src_rpm2 )
 
 test.must_not_exist( 'bin/main' )
-test.fail_test( not os.popen('rpm -qpl %s' % machine_rpm).read()=='/bin/main\n')
-test.fail_test( not os.popen('rpm -qpl %s' % src_rpm).read()=='foo-1.2.3.spec\nfoo-1.2.3.tar.gz\n')
+out = os.popen( 'rpm -qpl %s' % machine_rpm).read()
+test.must_contain_all_lines( out, '/bin/main')
+out = os.popen( 'rpm -qpl %s' % src_rpm).read()
+test.fail_test( not out == 'foo-1.2.3.spec\nfoo-1.2.3.tar.gz\n')
 
 test.pass_test()
 
diff --git a/test/packaging/rpm/package.py b/test/packaging/rpm/package.py
index d8c2785..6723a9f 100644
--- a/test/packaging/rpm/package.py
+++ b/test/packaging/rpm/package.py
@@ -88,8 +88,10 @@ machine_rpm = 'foo-1.2.3-0.%s.rpm' % SCons.Tool.rpmutils.defaultMachine()
 test.must_exist( machine_rpm )
 test.must_exist( src_rpm )
 test.must_not_exist( 'bin/main' )
-test.fail_test( not os.popen('rpm -qpl %s' % machine_rpm).read()=='/bin/main\n')
-test.fail_test( not os.popen('rpm -qpl %s' % src_rpm).read()=='foo-1.2.3.spec\nfoo-1.2.3.tar.gz\n')
+out = os.popen( 'rpm -qpl %s' % machine_rpm).read()
+test.must_contain_all_lines( out, '/bin/main')
+out = os.popen( 'rpm -qpl %s' % src_rpm).read()
+test.fail_test( not out == 'foo-1.2.3.spec\nfoo-1.2.3.tar.gz\n')
 
 test.pass_test()
 
diff --git a/test/packaging/rpm/tagging.py b/test/packaging/rpm/tagging.py
index a558242..15f82a7 100644
--- a/test/packaging/rpm/tagging.py
+++ b/test/packaging/rpm/tagging.py
@@ -91,9 +91,12 @@ src_rpm = 'foo-1.2.3-0.src.rpm'
 machine_rpm = 'foo-1.2.3-0.%s.rpm' % SCons.Tool.rpmutils.defaultMachine()
 
 test.must_exist( machine_rpm )
+out = os.popen('rpm -qpl %s' % machine_rpm).read()
+test.must_contain_all_lines( out, '/bin/main')
+
 test.must_exist( src_rpm )
-test.fail_test( not os.popen('rpm -qpl %s' % machine_rpm).read()=='/bin/main\n')
-test.fail_test( not os.popen('rpm -qpl %s' % src_rpm).read()=='foo-1.2.3.spec\nfoo-1.2.3.tar.gz\n')
+out = os.popen('rpm -qpl %s' % src_rpm).read()
+test.fail_test( not out == 'foo-1.2.3.spec\nfoo-1.2.3.tar.gz\n')
 
 expect = '(0755, root, users) /bin/main'
 test.must_contain_all_lines(test.read('foo-1.2.3.spec',mode='r'), [expect])