diff --git a/BuildUtils.pm b/BuildUtils.pm index ce4078d8c..dcefcebf3 100644 --- a/BuildUtils.pm +++ b/BuildUtils.pm @@ -32,7 +32,7 @@ our @EXPORT_OK = qw( lock_id_for take_build_lock sh_quote clean_debian_residue git_revision backup_file restore_file - sh usage + sh sh_or_die usage rewrite_file write_script read_line buildinfo_text ); @@ -115,6 +115,20 @@ sub sh { # 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. +# Run a command and stop the build when it fails. The same operation was +# spelled in opposite polarities -- `sh(...) == 0 or die` in builddebs.pl, +# `sh(...) and die` in buildrpms.pl, which also used both -- and the `and die` +# form reads as though the die is what happens next rather than what happens on +# failure. One name, one direction, and the exit code lands in the message. +sub sh_or_die { + my ($cmd, $message) = @_; + my $rc = sh($cmd); + return 0 if $rc == 0; + $message = "FATAL: command failed: $cmd" unless defined $message; + $message =~ s/\n\z//; + die "$message (exit $rc)\n"; +} + sub usage { my (%args) = @_; pod2usage( diff --git a/builddebs.pl b/builddebs.pl index 2f4e49b1b..60de1dafd 100755 --- a/builddebs.pl +++ b/builddebs.pl @@ -37,7 +37,7 @@ use BuildUtils qw( pin_control_version rewrite_changelog_header reprepro_distributions reprepro_options lock_id_for take_build_lock sh_quote - sh usage rewrite_file write_script read_line buildinfo_text + sh sh_or_die usage rewrite_file write_script read_line buildinfo_text ); # The xcat-core packages that ship as debs. xCAT-openbmc-py, xCAT-rmc and xCAT-release @@ -223,17 +223,17 @@ sub build_package { if ($text =~ /3\.0 \(quilt\)/) { my $tar = "$ROOT/" . orig_tarball_name($pkg, $PKGVER); unless (-f $tar) { - sh(sprintf('tar czf %s --exclude debian -C %s .', - sh_quote($tar), sh_quote($dir))) == 0 - or die "FATAL: could not create $tar\n"; + sh_or_die(sprintf('tar czf %s --exclude debian -C %s .', + sh_quote($tar), sh_quote($dir)), + "FATAL: could not create $tar\n"); } } } my $arch_flag = $arch eq 'all' ? '' : " -a$arch"; my $quiet = $opts{verbose} ? '' : ' >/dev/null'; - sh("cd " . sh_quote($dir) . " && dpkg-buildpackage -rfakeroot -uc -us$arch_flag$quiet") == 0 - or die "FATAL: dpkg-buildpackage failed for $pkg ($arch)\n"; + sh_or_die("cd " . sh_quote($dir) . " && dpkg-buildpackage -rfakeroot -uc -us$arch_flag$quiet", + "FATAL: dpkg-buildpackage failed for $pkg ($arch)\n"); }); return; @@ -301,9 +301,9 @@ sub assemble_repo { for my $deb (@debs) { # A release that predates an architecture must not be handed its packages. next if basename($deb) =~ /_(\w+)\.deb\z/ && $1 ne 'all' && !$ok{$1}; - sh("cd " . sh_quote($repodir) . " && reprepro -b ./ includedeb " - . sh_quote($dist) . ' ' . sh_quote($deb)) == 0 - or die "FATAL: reprepro could not add $deb to $dist\n"; + sh_or_die("cd " . sh_quote($repodir) . " && reprepro -b ./ includedeb " + . sh_quote($dist) . ' ' . sh_quote($deb), + "FATAL: reprepro could not add $deb to $dist\n"); } } return scalar @debs; diff --git a/buildrpms.pl b/buildrpms.pl index f7a6ca49f..f5fe76993 100755 --- a/buildrpms.pl +++ b/buildrpms.pl @@ -43,8 +43,8 @@ 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 sh usage buildinfo_text write_script - read_line); +use BuildUtils qw(git_revision source_date_epoch sh sh_or_die usage buildinfo_text + write_script read_line); use Fcntl qw(:flock); # per-target build lock (concurrency guard; see main()) use Getopt::Long qw(GetOptions); use POSIX qw(strftime); @@ -342,14 +342,14 @@ sub buildsources_genesis_base($) { remove_tree($staging_parent) if -e $staging_parent; make_path("$staging_root/dracut_105"); - sh(qq(cp -a "xCAT-genesis-builder/dracut_105" "$staging_root/")) - and die "Error copying dracut_105 sources"; + sh_or_die(qq(cp -a "xCAT-genesis-builder/dracut_105" "$staging_root/"), + "Error copying dracut_105 sources"); cp "xCAT-genesis-builder/80-net-name-slot.rules", "$staging_root/80-net-name-slot.rules"; unlink $support_tarball if -f $support_tarball; - sh(qq(tar --sort=name --owner=0 --group=0 --mtime="\@$SOURCE_DATE_EPOCH" -cjf "$support_tarball" -C "$staging_parent" xCAT-genesis-base-build-support)) - and die "Error creating $support_tarball"; + sh_or_die(qq(tar --sort=name --owner=0 --group=0 --mtime="\@$SOURCE_DATE_EPOCH" -cjf "$support_tarball" -C "$staging_parent" xCAT-genesis-base-build-support), + "Error creating $support_tarball"); remove_tree($staging_parent); } @@ -360,8 +360,8 @@ sub prepare_xcat_probe_source_tar { my $helper_dir = "$staging_root/lib/perl/xCAT"; my $source_tarball = "$SOURCES/xCAT-probe-$VERSION.tar.gz"; - sh(qq(cp -a "xCAT-probe" "$staging_root")) - and die "Error staging xCAT-probe sources"; + sh_or_die(qq(cp -a "xCAT-probe" "$staging_root"), + "Error staging xCAT-probe sources"); remove_tree($helper_dir) if -e $helper_dir; make_path($helper_dir); @@ -379,8 +379,8 @@ sub prepare_xcat_probe_source_tar { ); close $archive_fh; - sh(qq(tar --sort=name --owner=0 --group=0 --numeric-owner --mtime="\@$SOURCE_DATE_EPOCH" --use-compress-program="gzip -n" -cf "$archive_path" -C "$staging_parent" xCAT-probe)) - and die "Error creating $source_tarball"; + sh_or_die(qq(tar --sort=name --owner=0 --group=0 --numeric-owner --mtime="\@$SOURCE_DATE_EPOCH" --use-compress-program="gzip -n" -cf "$archive_path" -C "$staging_parent" xCAT-probe), + "Error creating $source_tarball"); chmod 0644, $archive_path; rename $archive_path, $source_tarball; @@ -661,9 +661,9 @@ sub setup_local_repos { sub createrepo_dir { my ($dir, $extra) = @_; $extra //= ''; - sh(qq(createrepo_c --update --database ) - . qq(--revision "$SOURCE_DATE_EPOCH" --set-timestamp-to-revision $extra "$dir")) - and die "Failed to createrepo_c $dir\n"; + sh_or_die(qq(createrepo_c --update --database ) + . qq(--revision "$SOURCE_DATE_EPOCH" --set-timestamp-to-revision $extra "$dir"), + "Failed to createrepo_c $dir\n"); } # A core repo dir holds binaries flat plus a SRPMS/ subdir carrying its own @@ -727,15 +727,15 @@ sub sign_repo_dir { say "Signing RPMs in $repodir"; my @bin = glob("$repodir/*.rpm"); if (@bin) { - sh(qq(rpmsign --define "%_gpg_name $key_name" --addsign ) - . join(" ", map { qq("$_") } @bin)) - and die "Failed to sign RPMs in $repodir"; + sh_or_die(qq(rpmsign --define "%_gpg_name $key_name" --addsign ) + . join(" ", map { qq("$_") } @bin), + "Failed to sign RPMs in $repodir"); } my @src = glob("$repodir/SRPMS/*.src.rpm"); if (@src) { - sh(qq(rpmsign --define "%_gpg_name $key_name" --addsign ) - . join(" ", map { qq("$_") } @src)) - and die "Failed to sign SRPMs in $repodir/SRPMS"; + sh_or_die(qq(rpmsign --define "%_gpg_name $key_name" --addsign ) + . join(" ", map { qq("$_") } @src), + "Failed to sign SRPMs in $repodir/SRPMS"); } # Regenerate both indexes (binary + SRPMS) after signing, before signing repomd. @@ -747,10 +747,10 @@ sub sign_repo_dir { next unless -f $repomd; say "Signing $repomd"; unlink "$repomd.asc" if -f "$repomd.asc"; - sh(qq(gpg -a --detach-sign --default-key "$key_name" "$repomd")) - and die "Failed to sign $repomd"; - sh(qq(gpg -a --export "$key_name" > "$rd/repomd.xml.key")) - and die "Failed to export public key to $rd"; + sh_or_die(qq(gpg -a --detach-sign --default-key "$key_name" "$repomd"), + "Failed to sign $repomd"); + sh_or_die(qq(gpg -a --export "$key_name" > "$rd/repomd.xml.key"), + "Failed to export public key to $rd"); } } @@ -832,11 +832,12 @@ sub merge_core_repos { die "FATAL: --merge-core-repos requires at least one --input-core-repos dir\n" unless @ins; -d $_ or die "FATAL: --input-core-repos dir '$_' does not exist\n" for @ins; - sh(qq(rm -rf "$out")) and die "Failed to clean output dir '$out'\n"; + sh_or_die(qq(rm -rf "$out"), + "Failed to clean output dir '$out'\n"); make_path($out); for my $in (@ins) { - sh(qq(rsync -a --exclude 'repodata/' "$in/" "$out/")) - and die "Failed to rsync '$in' into '$out'\n"; + sh_or_die(qq(rsync -a --exclude 'repodata/' "$in/" "$out/"), + "Failed to rsync '$in' into '$out'\n"); } # Index, sign (when --gpg-sign), write the final repository metadata, then create the diff --git a/xCAT-test/unit/build_utils.t b/xCAT-test/unit/build_utils.t index 34dd9af72..1971b39ad 100644 --- a/xCAT-test/unit/build_utils.t +++ b/xCAT-test/unit/build_utils.t @@ -427,4 +427,24 @@ isnt( git_revision( git => sub { '' }, read_file => sub { '' } ), '', like( $out, qr/\ARunning: true/, 'a verbose run echoes the command' ); } +# ---------------------------------------------------------- sh_or_die() -- +# The same run-or-fail step was written `sh(...) == 0 or die` in one builder +# and `sh(...) and die` in the other -- which also used both spellings itself. +# Reversed polarities for one operation are easy to misread, so there is now a +# single name with a single direction. +{ + is( BuildUtils::sh_or_die('true'), 0, + 'a command that succeeds returns 0 and does not die' ); + + my $err = eval { BuildUtils::sh_or_die('sh -c "exit 4"', 'FATAL: it failed'); 1 } + ? '' : $@; + like( $err, qr/FATAL: it failed/, 'a failure dies with the caller\'s message' ); + like( $err, qr/exit 4/, + 'and names the exit code, which the old spellings threw away' ); + + my $bare = eval { BuildUtils::sh_or_die('sh -c "exit 5"'); 1 } ? '' : $@; + like( $bare, qr/\Qsh -c "exit 5"\E/, + 'a caller with no message still gets the command that failed' ); +} + done_testing();