From e75b56a405d8bf8af8363b7fe0a6eeb1d3c33f72 Mon Sep 17 00:00:00 2001
From: Daniel Hilst <392820+dhilst@users.noreply.github.com>
Date: Tue, 28 Jul 2026 17:23:39 -0300
Subject: [PATCH] fix(xcat-dep): harden finalize peer requirement + rpm
 version/identity edges

Follow-up self-review hardening on top of the PR #62 review response:

- finalize_xcat_dep now treats a missing ppc64le PEER repo (not just missing
  genesis rpms) as fatal instead of silently skipping the OS -- in the CD both
  arches build every EL, so a missing peer is an incomplete input that would
  otherwise leave the x86_64 repo without the ppc64 genesis and still exit 0.

- cross_copy_genesis treats an empty SIGMD5 (unreadable rpm) as "cannot confirm
  identical" and refreshes, rather than risking a false up-to-date match when two
  unreadable rpms both return an empty digest.

- rpm_version fails when a directory holds more than one distinct version of a
  package (a stale artifact not cleaned before the build) instead of silently
  returning the first sorted match, which a version pin could pass against while
  the stale rpm still ships. Both arches share a Version for genesis, so a normal
  x86_64+ppc64 pair is a single entry.

t/mockbuild-all.t: +4 cases (30 total) -- missing-peer fatal, rpm_sigmd5 on a
missing rpm, and rpm_version multi-version failure.

Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
---
 MockBuildUtils.pm | 26 +++++++++++++++++++-------
 t/mockbuild-all.t | 27 +++++++++++++++++++++++----
 2 files changed, 42 insertions(+), 11 deletions(-)

diff --git a/MockBuildUtils.pm b/MockBuildUtils.pm
index 7924f6a..30e0cf3 100644
--- a/MockBuildUtils.pm
+++ b/MockBuildUtils.pm
@@ -84,6 +84,7 @@ sub rpm_version {
     my $glob = ($name eq 'xCAT-genesis-base')
         ? "$dir/xCAT-genesis-base-*.rpm"
         : "$dir/${name}-*.rpm";
+    my %vers;   # distinct %{version}s of the matching binary rpms
     for my $f (sort glob($glob)) {
         next if $f =~ /\.src\.rpm$/ || $f =~ /-debug(?:info|source)-/;
         my $n = `rpm -qp --qf '%{name}' ${\ sh_quote($f)} 2>/dev/null`;
@@ -92,9 +93,16 @@ sub rpm_version {
         next unless $match;
         my $v = `rpm -qp --qf '%{version}' ${\ sh_quote($f)} 2>/dev/null`;
         chomp $v;
-        return $v;
+        $vers{$v} = 1 if $v ne '';
     }
-    return undef;
+    return undef unless %vers;
+    # More than one distinct version present means a stale artifact was not cleaned before the
+    # build -- a version pin could then pass against the wrong rpm and both could be shipped.
+    # (For genesis both arches share the same Version, so a normal x86_64+ppc64 pair is one entry.)
+    die "Multiple versions of $name present in $dir: " . join(', ', sort keys %vers)
+      . " (stale artifact not cleaned before the build)\n" if keys(%vers) > 1;
+    my ($v) = keys %vers;
+    return $v;
 }
 
 # read_manifest: parse packages-manifest.conf into %{ target => { package => version|'*' } }.
@@ -137,7 +145,10 @@ sub cross_copy_genesis {
         my $up_to_date = 1;
         for my $base (keys %want) {
             my $dst = "$to/$base";
-            if (!-f $dst || rpm_sigmd5($want{$base}) ne rpm_sigmd5($dst)) { $up_to_date = 0; last; }
+            my $src_sig = rpm_sigmd5($want{$base});
+            # An empty SIGMD5 (unreadable rpm) means "cannot confirm identical" -> refresh rather
+            # than risk skipping on a false match (two '' would otherwise compare equal).
+            if (!-f $dst || $src_sig eq '' || $src_sig ne rpm_sigmd5($dst)) { $up_to_date = 0; last; }
         }
         return 0 if $up_to_date;
     }
@@ -175,10 +186,11 @@ sub finalize_xcat_dep {
         my $osdir  = basename($p);
         my $x86dir = "$x86_64_repo/$osdir/x86_64";
         my $ppcdir = "$ppc64le_repo/$osdir/ppc64le";
-        if (!-d $ppcdir) {
-            print "[finalize] $osdir: no ppc64le peer at $ppcdir -- skipping\n";
-            next;
-        }
+        # Require the peer repo itself: in the CD both arches build every EL, so a missing
+        # ppc64le peer for an x86_64 OS means an incomplete input, not something to skip past
+        # (skipping would leave that OS's x86_64 repo without the ppc64 genesis and still exit 0).
+        die "FATAL: [finalize] $osdir: no ppc64le peer repo at $ppcdir\n"
+          . "  (both arches must build every EL before finalize)\n" if !-d $ppcdir;
         # Require the expected inputs: each arch's build must have produced its OWN genesis rpm
         # before finalize cross-populates them. Without this, a pair whose builds produced no
         # genesis rpms would make finalize a silent no-op that still exits 0 (the bug this guards).
diff --git a/t/mockbuild-all.t b/t/mockbuild-all.t
index 9f0b690..f43d032 100644
--- a/t/mockbuild-all.t
+++ b/t/mockbuild-all.t
@@ -10,7 +10,7 @@ use lib "$RealBin/..";
 use File::Temp qw(tempdir);
 use File::Path qw(make_path);
 use File::Basename qw(basename);
-use MockBuildUtils qw(required_pkgs version_matches rpm_sigmd5
+use MockBuildUtils qw(required_pkgs version_matches rpm_sigmd5 rpm_version
                       cross_copy_genesis finalize_xcat_dep read_manifest);
 
 # Run a printing sub with STDOUT muted so its progress lines do not pollute TAP.
@@ -64,19 +64,24 @@ ok( version_matches('anything', '*'),    "'*' matches any version");
     is_deeply({read_manifest("$dir/nope.conf")}, {}, 'read_manifest: missing file -> empty');
 }
 
+# rpm_sigmd5 on a missing/unreadable rpm returns '' (so cross_copy treats it as "not identical").
+is(rpm_sigmd5('/nonexistent/xCAT-genesis-base-ppc64-9.9.9.noarch.rpm'), '',
+    'rpm_sigmd5 returns empty for a missing rpm');
+
 # ---- RPM-identity comparison + cross_copy_genesis (needs rpmbuild for real rpms) --------------
 SKIP: {
-    skip 'rpmbuild not available', 5 if system('command -v rpmbuild >/dev/null 2>&1') != 0;
+    skip 'rpmbuild not available', 6 if system('command -v rpmbuild >/dev/null 2>&1') != 0;
     my $tmp = tempdir(CLEANUP => 1);
     my $seq = 0;
     my $mk = sub {                       # build a genesis-named rpm with a given marker payload
-        my ($tarch, $content) = @_;
+        my ($tarch, $content, $version) = @_;
+        $version ||= '2.19.0';
         my $out = "$tmp/out" . (++$seq);   # unique dir: same NVR would overwrite in a shared one
         my $spec = "$tmp/$tarch-$seq.spec";
         open my $fh, '>', $spec or die;
         print $fh <<"SPEC";
 Name: xCAT-genesis-base-$tarch
-Version: 2.19.0
+Version: $version
 Release: snapTEST
 Summary: test fixture
 License: EPL
@@ -122,6 +127,13 @@ SPEC
     my @signed;
     quiet { cross_copy_genesis($from2, $to2, 'ppc64', sub { push @signed, $_[0] }) };
     is_deeply(\@signed, ["$to2/$base"], 'the sign callback runs on each copied rpm');
+
+    # rpm_version dies when a dir holds two DIFFERENT versions of the same package (stale artifact).
+    my $vdir = "$tmp/vers"; make_path($vdir);
+    system("cp '" . $mk->('ppc64', 'x', '2.19.0') . "' '$vdir/'");
+    system("cp '" . $mk->('ppc64', 'x', '2.18.0') . "' '$vdir/'");
+    my $vdied = !eval { rpm_version($vdir, 'xCAT-genesis-base'); 1 };
+    ok($vdied, 'rpm_version dies when a dir holds multiple distinct versions of a package');
 }
 
 # ---- finalize_xcat_dep: require the genesis inputs (no silent no-op) --------------------------
@@ -137,6 +149,13 @@ SPEC
     make_path("$tmp2/x", "$tmp2/p");
     my $ok2 = eval { quiet { finalize_xcat_dep("$tmp2/x", "$tmp2/p") }; 1 };
     ok(!$ok2, 'finalize dies when no <os>/x86_64 + <os>/ppc64le pair is found');
+
+    # A missing ppc64le PEER repo (not just missing rpms) is fatal, not a silent skip.
+    my $tmp3 = tempdir(CLEANUP => 1);
+    make_path("$tmp3/x/rh9/x86_64");    # x86_64 OS present, but NO ppc64le peer dir at all
+    my $ok3 = eval { quiet { finalize_xcat_dep("$tmp3/x", "$tmp3/p") }; 1 };
+    ok(!$ok3, 'finalize dies when an x86_64 OS has no ppc64le peer repo (no silent skip)');
+    like($@, qr/no ppc64le peer repo/, 'finalize error names the missing peer');
 }
 
 done_testing;