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

@@ -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

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 };

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).

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();