From 42e4c160d4a838682e4687028ca4d1928f4fb848 Mon Sep 17 00:00:00 2001
From: Daniel Hilst <392820+dhilst@users.noreply.github.com>
Date: Tue, 1 Sep 2026 16:18:45 -0300
Subject: [PATCH] refactor(build): share sh() and usage() between the two
 builders

Both builders defined their own sh() and usage(). The usage() copies differed
only in spelling, but the sh() copies disagreed about what they return:
buildrpms.pl shifted system()'s result into an exit code, builddebs.pl returned
the raw wait status. The same command therefore reported 1 from one builder and
256 from the other, and a caller comparing sh() against a particular code was
correct in only one of them. Both current call sites only test for zero, so
nothing was broken yet.

BuildUtils.pm now provides both, shifting the status as buildrpms.pl did.
pod2usage reads the POD of the running program, so each builder keeps its own
help text while sharing how it is printed and the status it exits with.

sh() echoed the command under the builder's own --verbose flag, which a shared
function cannot see. $BuildUtils::VERBOSE is set once after option parsing
instead of threading the flag through every call site.

Covered by tests: that sh() returns the exit code rather than the wait status
it is packed into, and that a verbose run still echoes the command.

Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
---
 BuildUtils.pm                | 30 ++++++++++++++++++++++++++++++
 builddebs.pl                 | 18 +++---------------
 buildrpms.pl                 | 24 +++---------------------
 xCAT-test/unit/build_utils.t | 24 ++++++++++++++++++++++++
 4 files changed, 60 insertions(+), 36 deletions(-)

diff --git a/BuildUtils.pm b/BuildUtils.pm
index 69e98dc51..6d8dbf70e 100644
--- a/BuildUtils.pm
+++ b/BuildUtils.pm
@@ -18,6 +18,8 @@ use File::Copy qw(copy move);
 use File::Basename qw(basename);
 use File::Path qw(make_path remove_tree);
 use POSIX qw(strftime);
+use Pod::Usage qw(pod2usage);
+use feature 'say';
 
 our @EXPORT_OK = qw(
     source_date_epoch snap_release deb_version
@@ -29,11 +31,39 @@ our @EXPORT_OK = qw(
     lock_id_for take_build_lock
     sh_quote clean_debian_residue git_revision
     backup_file restore_file
+    sh usage
 );
 
+# Both builders echo the commands they run under --verbose.  Set once, after
+# option parsing, rather than threaded through every sh() call site.
+our $VERBOSE = 0;
+
 # The xCAT-probe helpers. xcat-probe reuses functions shipped by xCAT; they are COPIED
 # rather than symlinked because a symlink does not survive packaging, and rather than
 # maintained twice because they would drift. Both builders stage them the same way.
+# Run a shell command, returning its EXIT STATUS.  system() yields the raw wait
+# status, which is the exit code times 256, so it is shifted here: a caller
+# comparing the result against a specific code gets the code it expects, not a
+# multiple of it.
+sub sh {
+    my ($cmd) = @_;
+    say "Running: $cmd" if $VERBOSE;
+    system($cmd);
+    return $? >> 8;
+}
+
+# pod2usage reads the POD of the running program, so each builder keeps its own
+# help text while sharing the way it is printed and the status it exits with.
+sub usage {
+    my (%args) = @_;
+    pod2usage(
+        -verbose => $args{verbose} // 1,
+        -exitval => $args{exitval} // 2,
+        (defined($args{message}) && length($args{message})
+            ? (-message => "$args{message}\n") : ()),
+    );
+}
+
 use constant XCAT_PROBE_HELPERS => qw(
     GlobalDef.pm
     NetworkUtils.pm
diff --git a/builddebs.pl b/builddebs.pl
index 57b952292..83decdb05 100755
--- a/builddebs.pl
+++ b/builddebs.pl
@@ -36,6 +36,7 @@ use BuildUtils qw(
     pin_control_version rewrite_changelog_header
     reprepro_distributions reprepro_options
     lock_id_for take_build_lock sh_quote
+    sh usage
 );
 
 # The xcat-core packages that ship as debs. xCAT-openbmc-py, xCAT-rmc and xCAT-release
@@ -75,6 +76,8 @@ GetOptions(
 ) or usage();
 usage(exitval => 0, verbose => 2) if $opts{help};
 
+$BuildUtils::VERBOSE = $opts{verbose};
+
 $opts{packages} = @cli_packages ? \@cli_packages : \@PACKAGES;
 $opts{dists}    = @cli_dists    ? \@cli_dists    : \@DISTS;
 $opts{gpg_key_name} //= 'xCAT Signing Key';
@@ -84,21 +87,6 @@ for my $pkg ($opts{packages}->@*) {
         unless grep { $_ eq $pkg } @PACKAGES;
 }
 
-sub usage {
-    my (%args) = @_;
-    pod2usage(
-        -verbose => $args{verbose} // 1,
-        -exitval => $args{exitval} // 2,
-        (defined $args{message} ? (-message => "$args{message}\n") : ()),
-    );
-}
-
-sub sh {
-    my ($cmd) = @_;
-    say "+ $cmd" if $opts{verbose};
-    return system($cmd);
-}
-
 my $ROOT    = abs_path($FindBin::Bin);
 my $VERSION = do { open my $fh, '<', "$ROOT/Version" or die "Cannot read Version: $!\n";
                    my $v = <$fh>; chomp $v; $v };
diff --git a/buildrpms.pl b/buildrpms.pl
index 92d96014d..9057bed41 100755
--- a/buildrpms.pl
+++ b/buildrpms.pl
@@ -43,7 +43,7 @@ use File::Slurper qw(read_text write_text);
 use File::Temp qw(tempdir tempfile);
 use FindBin qw($Bin);
 use lib $Bin;
-use BuildUtils qw(git_revision source_date_epoch);
+use BuildUtils qw(git_revision source_date_epoch sh usage);
 use Fcntl qw(:flock);           # per-target build lock (concurrency guard; see main())
 use Getopt::Long qw(GetOptions);
 use POSIX qw(strftime);
@@ -189,6 +189,8 @@ GetOptions(
     "source-only" => \$opts{source_only},
 ) or usage();
 
+$BuildUtils::VERBOSE = $opts{verbose};
+
 # --package REPLACES the default set (build exactly what was asked), so
 # `--package xCAT-genesis-base` builds only genesis-base for the dep pipeline.
 # The full default set is built on every arch (x86_64 and ppc64le alike), so each
@@ -231,26 +233,6 @@ if (@cli_targets) {
 my $RELEASE = $opts{release} || strftime("snap%Y%m%d%H%M", gmtime($SOURCE_DATE_EPOCH));
 write_text("Release", "$RELEASE\n");
 
-sub usage {
-    my (%args) = @_;
-    my $verbose = $args{verbose} // 1;
-    my $exitval = $args{exitval} // 2;
-    my $message = $args{message};
-    pod2usage(
-        -verbose => $verbose,
-        -exitval => $exitval,
-        (defined($message) && length($message) ? (-message => "$message\n") : ()),
-    );
-}
-
-sub sh {
-    my ($cmd) = @_;
-    say "Running: $cmd"
-        if $opts{verbose};
-    system($cmd);
-    $? >> 8;
-}
-
 # sh_retry: run $cmd, retrying up to $tries times on non-zero exit. Absorbs transient mock/nspawn
 # flakes (e.g. the systemd-nspawn ENOMEDIUM cgroup race, dnf mirror hiccups) so one bad attempt does
 # not silently drop a package from the core. Returns the last exit code (0 on eventual success).
diff --git a/xCAT-test/unit/build_utils.t b/xCAT-test/unit/build_utils.t
index 00f975308..14697770b 100644
--- a/xCAT-test/unit/build_utils.t
+++ b/xCAT-test/unit/build_utils.t
@@ -308,4 +308,28 @@ is( git_revision( git => sub { "\n" }, read_file => sub { "  \n" } ),
 isnt( git_revision( git => sub { '' }, read_file => sub { '' } ), '',
     'the one thing it must never return is empty' );
 
+# ---------------------------------------------------------------- sh() --
+# system() returns the raw wait status, which is the exit code times 256. The
+# two builders disagreed about shifting it, so a caller comparing sh() against
+# a specific code got the code from one and a multiple of it from the other.
+{
+    is( BuildUtils::sh('true'), 0, 'a command that succeeds reports 0' );
+    is( BuildUtils::sh('sh -c "exit 3"'), 3,
+        'the exit code is returned, not the wait status it is packed into' );
+    isnt( BuildUtils::sh('sh -c "exit 3"'), 768,
+        'and specifically not the exit code times 256' );
+}
+
+{
+    # --verbose echoes the command; the default does not.
+    local $BuildUtils::VERBOSE = 1;
+    my $out = '';
+    open my $fh, '>', \$out or die;
+    my $old = select $fh;
+    BuildUtils::sh('true');
+    select $old;
+    close $fh;
+    like( $out, qr/\ARunning: true/, 'a verbose run echoes the command' );
+}
+
 done_testing();