From dcce3e64485dbc15f85c10a18db3ad1dc04558e4 Mon Sep 17 00:00:00 2001
From: aiuto <aiuto@google.com>
Date: Mon, 25 Mar 2024 10:51:05 -0400
Subject: [PATCH] Improve handling of sub rpms. (#833)

- Do not create the subrpms files when we don't have to
- Do not add `buildsubdir .`  That seems to cause rpmbuild failures with
  older rpmbuild.
- General cleanup: Remove tests against None which is needlessly brittle.
---
 pkg/make_rpm.py | 20 +++++++++++---------
 pkg/rpm_pfg.bzl | 18 ++++++------------
 2 files changed, 17 insertions(+), 21 deletions(-)

diff --git a/pkg/make_rpm.py b/pkg/make_rpm.py
index 28c8e891..ded2832b 100644
--- a/pkg/make_rpm.py
+++ b/pkg/make_rpm.py
@@ -259,17 +259,17 @@ def SetupWorkdir(self,
 
     # Slurp in the scriptlets...
     self.pre_scriptlet = \
-      SlurpFile(os.path.join(original_dir, pre_scriptlet_path)) if pre_scriptlet_path is not None else ''
+      SlurpFile(os.path.join(original_dir, pre_scriptlet_path)) if pre_scriptlet_path else ''
     self.post_scriptlet = \
-      SlurpFile(os.path.join(original_dir, post_scriptlet_path)) if post_scriptlet_path is not None else ''
+      SlurpFile(os.path.join(original_dir, post_scriptlet_path)) if post_scriptlet_path else ''
     self.preun_scriptlet = \
-      SlurpFile(os.path.join(original_dir, preun_scriptlet_path)) if preun_scriptlet_path is not None else ''
+      SlurpFile(os.path.join(original_dir, preun_scriptlet_path)) if preun_scriptlet_path else ''
     self.postun_scriptlet = \
-      SlurpFile(os.path.join(original_dir, postun_scriptlet_path)) if postun_scriptlet_path is not None else ''
+      SlurpFile(os.path.join(original_dir, postun_scriptlet_path)) if postun_scriptlet_path else ''
     self.posttrans_scriptlet = \
-      SlurpFile(os.path.join(original_dir, posttrans_scriptlet_path)) if posttrans_scriptlet_path is not None else ''
+      SlurpFile(os.path.join(original_dir, posttrans_scriptlet_path)) if posttrans_scriptlet_path else ''
     self.subrpms = \
-      SlurpFile(os.path.join(original_dir, subrpms_file)) if subrpms_file is not None else ''
+      SlurpFile(os.path.join(original_dir, subrpms_file)) if subrpms_file else ''
 
     # Then prepare for textual substitution.  This is typically only the case for the
     # experimental `pkg_rpm`.
@@ -279,7 +279,7 @@ def SetupWorkdir(self,
       'PREUN_SCRIPTLET': ("%preun\n" + self.preun_scriptlet) if self.preun_scriptlet else "",
       'POSTUN_SCRIPTLET': ("%postun\n" + self.postun_scriptlet) if self.postun_scriptlet else "",
       'POSTTRANS_SCRIPTLET': ("%posttrans\n" + self.posttrans_scriptlet) if self.posttrans_scriptlet else "",
-      'SUBRPMS' : (self.subrpms if self.subrpms else ""),
+      'SUBRPMS' : self.subrpms,
       'CHANGELOG': ""
     }
 
@@ -362,11 +362,13 @@ def CallRpmBuild(self, dirname, rpmbuild_args):
       args.append('-vv')
 
     # Common options
+    # NOTE: There may be a need to add '--define', 'buildsubdir .' for some
+    # rpmbuild versions. But that breaks other rpmbuild versions, so before
+    # adding it back in, add extensive tests.
     args += [
         '--define', '_topdir %s' % dirname,
         '--define', '_tmppath %s/TMP' % dirname,
         '--define', '_builddir %s/BUILD' % dirname,
-        '--define', 'buildsubdir .',
         '--bb',
         '--buildroot=%s' % buildroot,
     ]  # yapf: disable
@@ -467,7 +469,7 @@ def Build(self, spec_file, out_file, subrpm_out_files=None,
     spec_file = os.path.join(original_dir, spec_file)
     out_file = os.path.join(original_dir, out_file)
 
-    if subrpm_out_files is not None:
+    if subrpm_out_files:
       subrpm_out_files = (s.split(':') for s in subrpm_out_files)
       subrpm_out_files = [
          (s[0], os.path.join(original_dir, s[1])) for s in subrpm_out_files]
diff --git a/pkg/rpm_pfg.bzl b/pkg/rpm_pfg.bzl
index b0a86954..2fd6b6a1 100644
--- a/pkg/rpm_pfg.bzl
+++ b/pkg/rpm_pfg.bzl
@@ -666,27 +666,21 @@ def _pkg_rpm_impl(ctx):
         _process_dep(dep, rpm_ctx)
 
     #### subrpms
-    subrpm_file = ctx.actions.declare_file(
-        "{}.spec.subrpms".format(rpm_name),
-    )
     if ctx.attr.subrpms:
         subrpm_lines = []
-
         for s in ctx.attr.subrpms:
-            subrpm_lines += _process_subrpm(ctx, rpm_name, s[PackageSubRPMInfo], rpm_ctx)
+            subrpm_lines.extend(_process_subrpm(ctx, rpm_name, s[PackageSubRPMInfo], rpm_ctx))
 
-        ctx.actions.write(
-            output = subrpm_file,
-            content = "\n".join(subrpm_lines),
+        subrpm_file = ctx.actions.declare_file(
+            "{}.spec.subrpms".format(rpm_name),
         )
-    else:
         ctx.actions.write(
             output = subrpm_file,
-            content = "# no subrpms",
+            content = "\n".join(subrpm_lines),
         )
+        files.append(subrpm_file)
+        rpm_ctx.make_rpm_args.append("--subrpms=" + subrpm_file.path)
 
-    files.append(subrpm_file)
-    rpm_ctx.make_rpm_args.append("--subrpms=" + subrpm_file.path)
     #### Procedurally-generated scripts/lists (%install, %files)
 
     # We need to write these out regardless of whether we are using