xCAT/xcat-dep
2
0
Fork 0
mirror of https://github.com/xcat2/xcat-dep.git synced 2026-09-12 04:26:25 +00:00
Code Issues Projects Releases Wiki Activity

fix(build): address code review — zero-tolerance comment, gpg quoting, manifest-derived required set, testable release bump

Browse Source 
Follow-up to the @viniciusferrao review of the EL matrix build:

- Rewrite the stale run_build_steps_parallel comment that still described the
  removed 'tolerate genesis failure' workaround; the code is strict
  zero-tolerance (xcat-core #7696 made buildrpms.pl exit 0 iff it built the
  genesis rpm), so the comment now matches.
- sh_quote the operator-supplied --gpg-key-name at every rpmsign/gpg site
  (was interpolated raw into the shell).
- Derive assert_required_deps' required set from the target's
  packages-manifest.conf section (the single source of truth) instead of a
  second hard-coded list that could drift.
- Move bump_dep_release_suffix into MockBuildUtils (pure, arg-driven) and add a
  File::Temp fixture test (stamp, xcat-core prune, no-Release skip, idempotency)
  -- the paths the review asked to cover. Its temp file now carries hostname+pid
  so the two arch build hosts can't collide on the shared NFS tree.

prove t/mockbuild-all.t: 50/50.

Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
This commit is contained in:
Daniel Hilst
2026-08-12 11:41:46 -03:00
parent 9d32988ae2
commit 441f13c034
3 changed files with 124 additions and 74 deletions
Download Patch File Download Diff File Expand all files Collapse all files
MockBuildUtils.pm
+58 -1
View File
@@ -8,12 +8,14 @@ use warnings;
use Exporter 'import';
use File::Basename qw(basename);
use File::Copy qw(copy);
use File::Find;
use Sys::Hostname;
our @EXPORT_OK = qw(
sh_quote print_step
version_matches required_pkgs have_rpm read_manifest
rpm_version rpm_release rpm_sigmd5 rpm_is_signed restamp_release_line
cross_copy_genesis finalize_xcat_dep
cross_copy_genesis finalize_xcat_dep bump_dep_release_suffix
);
# sh_quote: single-quote a string for safe use in a shell command.
@@ -272,4 +274,59 @@ sub finalize_xcat_dep {
print_step('Finalize complete');
}
# bump_dep_release_suffix: append $suffix (e.g. ".snap202607161200.57") to the Release: line of
# every xcat-dep package spec under $repo_root, so the CD build stamps a fresh, monotonic NVR.
# Idempotent: a spec already carrying this exact suffix is left alone (a re-run in the same tree
# does not double-stamp). Preserves any %{?dist}/%{?distver} macro already on the line. Returns the
# count of specs newly stamped. Dies only if NO spec under $repo_root carries a Release: line.
# Pure (takes everything as args) so t/mockbuild-all.t can exercise it directly.
sub bump_dep_release_suffix {
my ($repo_root, $suffix) = @_;
my @specs;
# Only stamp xcat-dep's OWN specs. If someone checked xcat-core out NESTED under $repo_root (the
# legacy `xcat-source-code`/`xcat-core` layout), do NOT descend into it -- rewriting a core spec
# (e.g. xCAT-genesis-base.spec's dynamic Release) would break the lockstep with genesis-scripts.
find(sub {
if (-d $_ && ($_ eq 'xcat-core' || $_ eq 'xcat-source-code')) { $File::Find::prune = 1; return; }
push @specs, $File::Find::name if /\.spec$/ && -f $_;
}, $repo_root);
my ($with_release, $bumped, $already) = (0, 0, 0);
for my $spec (sort @specs) {
open my $in, '<', $spec or die "open $spec: $!\n";
my @lines = <$in>;
close $in;
my ($has_release, $changed) = (0, 0);
for my $line (@lines) {
# case-insensitive: some specs (e.g. Sys-Virt.spec) use a lowercase `release:`
next unless $line =~ /^Release:\s*\S/i;
$has_release = 1;
# restamp_release_line is idempotent (no-op if already carrying $suffix) and strips any
# prior .snap stamp before applying the new one, so a re-run with a different
# --build-number replaces rather than accumulates (unit-tested in t/mockbuild-all.t).
my ($new, $ch) = restamp_release_line($line, $suffix);
if ($ch) { $line = $new; $changed = 1; }
last; # only the first Release: line
}
$with_release++ if $has_release;
$already++ if $has_release && !$changed;
next unless $changed;
# atomic write (temp + rename) so a concurrent per-arch build on the shared NFS tree never
# sees a torn spec; identical suffix -> identical content, so last-writer-wins is safe. The
# temp name carries the hostname AND pid: the two arch build hosts share the NFS tree and can
# reuse the same pid, so pid alone could collide across hosts.
my $tmp = "$spec.bump." . hostname() . ".$$";
open my $out, '>', $tmp or die "open> $tmp: $!\n";
print {$out} @lines;
close $out;
rename $tmp, $spec or die "rename $tmp -> $spec: $!\n";
$bumped++;
}
print "Release bump '$suffix': $bumped newly stamped, $already already stamped, of $with_release spec(s) with a Release line under $repo_root\n";
# Only a genuine "no dep specs at all" is fatal. All-already-stamped is the expected idempotent
# case (re-run in the same tree, or the other arch bumped first) -- NOT an error.
die "FATAL: --build-number given but NO spec carried a Release: line under $repo_root (wrong tree?)\n"
if $with_release == 0;
return $bumped;
}
1;
mockbuild-all.pl
+27 -72
View File
@@ -15,7 +15,7 @@ use FindBin qw($RealBin);
use lib $RealBin;
use MockBuildUtils qw(sh_quote print_step version_matches required_pkgs have_rpm
read_manifest rpm_version rpm_release rpm_sigmd5 restamp_release_line
cross_copy_genesis finalize_xcat_dep);
cross_copy_genesis finalize_xcat_dep bump_dep_release_suffix);
my $script_dir = abs_path(dirname(__FILE__));
my $repo_root = abs_path($script_dir);
@@ -151,7 +151,7 @@ if ($finalize_xcat_dep) {
sign => ($gpg_sign ? sub {
my ($rpm) = @_;
local $ENV{GNUPGHOME} = $gpg_home if $gpg_home;
run_simple(qq(rpmsign --define "%_gpg_name $gpg_key_name" --addsign ) . sh_quote($rpm));
run_simple("rpmsign --define " . sh_quote("%_gpg_name $gpg_key_name") . " --addsign " . sh_quote($rpm));
} : undef),
reindex => \&reindex_and_sign_repo,
);
@@ -268,56 +268,6 @@ die "FATAL: $tgt_fail target(s) failed\n" if $tgt_fail;
print_step('All targets completed');
exit 0;
# Append $suffix (e.g. ".snap202607161200.57") to the Release: line of every xcat-dep
# package spec under $root, so the CD build stamps a fresh, monotonic NVR. Idempotent:
# a spec already carrying this exact suffix is left alone (so a re-run in the same tree
# does not double-stamp). Preserves any %{?dist}/%{?distver} macro already on the line.
sub bump_dep_release_suffix {
my ($root, $suffix) = @_;
my @specs;
# Only stamp xcat-dep's OWN specs. If someone checked xcat-core out NESTED under $repo_root (the
# legacy `xcat-source-code`/`xcat-core` layout), do NOT descend into it -- rewriting a core spec
# (e.g. xCAT-genesis-base.spec's dynamic Release) would break the lockstep with genesis-scripts.
find(sub {
if (-d $_ && ($_ eq 'xcat-core' || $_ eq 'xcat-source-code')) { $File::Find::prune = 1; return; }
push @specs, $File::Find::name if /\.spec$/ && -f $_;
}, $root);
my ($with_release, $bumped, $already) = (0, 0, 0);
for my $spec (sort @specs) {
open my $in, '<', $spec or die "open $spec: $!\n";
my @lines = <$in>;
close $in;
my ($has_release, $changed) = (0, 0);
for my $line (@lines) {
# case-insensitive: some specs (e.g. Sys-Virt.spec) use a lowercase `release:`
next unless $line =~ /^Release:\s*\S/i;
$has_release = 1;
# restamp_release_line is idempotent (no-op if already carrying $suffix) and strips any
# prior .snap stamp before applying the new one, so a re-run with a different
# --build-number replaces rather than accumulates (unit-tested in t/mockbuild-all.t).
my ($new, $ch) = restamp_release_line($line, $suffix);
if ($ch) { $line = $new; $changed = 1; }
last; # only the first Release: line
}
$with_release++ if $has_release;
$already++ if $has_release && !$changed;
next unless $changed;
# atomic write (temp + rename) so a concurrent per-arch build on the shared NFS tree never
# sees a torn spec; identical suffix -> identical content, so last-writer-wins is safe.
my $tmp = "$spec.bump.$$";
open my $out, '>', $tmp or die "open> $tmp: $!\n";
print {$out} @lines;
close $out;
rename $tmp, $spec or die "rename $tmp -> $spec: $!\n";
$bumped++;
}
print "Release bump '$suffix': $bumped newly stamped, $already already stamped, of $with_release spec(s) with a Release line under $root\n";
# Only a genuine "no dep specs at all" is fatal. All-already-stamped is the expected idempotent
# case (re-run in the same tree, or the other arch bumped first) -- NOT an error.
die "FATAL: --build-number given but NO spec carried a Release: line under $root (wrong tree?)\n"
if $with_release == 0;
}
# Build a single target into its own build-output/<target-runid> tree and return
# { repo_dir, rel }. Everything below through the summary is per-target work.
sub build_one_target {
@@ -782,7 +732,12 @@ sub deploy_target {
copy($rpm, "$dest/" . basename($rpm))
or die "Failed to copy $rpm -> $dest: $!\n";
}
assert_required_deps($dest);
# Derive the required package set from THIS target's manifest section (the single source of
# truth), dropping any package whose builder was skipped, and assert each landed in the repo.
my %MAN = read_manifest("$repo_root/packages-manifest.conf");
my %req = %{ $MAN{$tgt} // {} };
my @required = required_pkgs([sort keys %req], $skip_genesis, $skip_perl, $skip_xcat_dep);
assert_required_deps($dest, \@required);
sign_and_index_repo($dest);
write_dep_repo_metadata($dest, $rel);
my $n = scalar(grep { !/\.src\.rpm$/ } glob("$dest/*.rpm"));
@@ -804,7 +759,7 @@ sub sign_and_index_repo {
my @rpms = grep { !/\.src\.rpm$/ } glob("$dir/*.rpm");
if ($gpg_sign && @rpms) {
local $ENV{GNUPGHOME} = $gpg_home if $gpg_home;
run_simple(qq(rpmsign --define "%_gpg_name $gpg_key_name" --addsign )
run_simple("rpmsign --define " . sh_quote("%_gpg_name $gpg_key_name") . " --addsign "
. join(' ', map { sh_quote($_) } @rpms));
}
run_simple(createrepo_c_cmd($dir));
@@ -812,8 +767,8 @@ sub sign_and_index_repo {
local $ENV{GNUPGHOME} = $gpg_home if $gpg_home;
my $repomd = "$dir/repodata/repomd.xml";
unlink "$repomd.asc" if -f "$repomd.asc";
run_simple(qq(gpg -a --detach-sign --default-key "$gpg_key_name" ) . sh_quote($repomd));
run_simple(qq(gpg -a --export "$gpg_key_name" > ) . sh_quote("$repomd.key"));
run_simple("gpg -a --detach-sign --default-key " . sh_quote($gpg_key_name) . ' ' . sh_quote($repomd));
run_simple("gpg -a --export " . sh_quote($gpg_key_name) . " > " . sh_quote("$repomd.key"));
}
}
@@ -883,8 +838,8 @@ sub reindex_and_sign_repo {
local $ENV{GNUPGHOME} = $gpg_home if $gpg_home;
my $repomd = "$dir/repodata/repomd.xml";
unlink "$repomd.asc" if -f "$repomd.asc";
run_simple(qq(gpg -a --detach-sign --default-key "$gpg_key_name" ) . sh_quote($repomd));
run_simple(qq(gpg -a --export "$gpg_key_name" > ) . sh_quote("$repomd.key"));
run_simple("gpg -a --detach-sign --default-key " . sh_quote($gpg_key_name) . ' ' . sh_quote($repomd));
run_simple("gpg -a --export " . sh_quote($gpg_key_name) . " > " . sh_quote("$repomd.key"));
}
}
@@ -1045,11 +1000,12 @@ sub run_build_steps_parallel {
return if !@{$steps};
# Returns the ids of any steps that failed; the caller (build_one_target) enforces
# zero-tolerance -- any failed manifest package fails the whole run. We build only packages
# required for the target (per packages-manifest.conf), so there is no "expected to fail on this
# arch/el" case left to tolerate. genesis is the sole exception the CALLER handles: xcat-core's
# buildrpms.pl exits non-zero on an unrelated post-build xCAT-release-latest cp even when the
# genesis rpm IS built, so the caller treats genesis as failed only if its rpm is absent.
# zero-tolerance -- ANY failed step fails the whole run, genesis included, with no special-case.
# We build only packages required for the target (per packages-manifest.conf), so there is no
# "expected to fail on this arch/el" case left to tolerate. There is likewise no genesis
# exception: since xcat-core #7696, buildrpms.pl exits 0 iff it produced the genesis rpm, so a
# non-zero genesis exit is a real failure (the old "tolerate if the rpm is already present"
# workaround is gone -- a stale artifact must never mask a failed build).
if ($dry_run || $max_processes <= 1 || @{$steps} == 1) {
my @failed;
for my $step (@{$steps}) {
@@ -1125,20 +1081,19 @@ sub run_build_steps_parallel {
# assert_required_deps: the per-EL dep repo is unusable without these, so a MISSING one is
# fatal even though individual builder failures are tolerated above. genesis-base is required
# unless --skip-genesis.
# fatal even though individual builder failures are tolerated above. The required set is derived
# by the caller from this target's packages-manifest.conf section (the single source of truth) and
# passed in as $required_ref, rather than duplicated as a hard-coded list here.
sub assert_required_deps {
my ($dir) = @_;
my ($dir, $required_ref) = @_;
# xCAT Requires all of these on every arch, and every one of them builds natively on every
# arch (the noarch deps -- grub2-xcat, xnba-undi -- just repackage committed artifacts), so
# a self-sufficient per-arch build produces the whole set with no cross-arch import.
# elilo-xcat is noarch but xCAT hard-requires it (Requires: elilo-xcat >= 3.14-6) on EVERY arch,
# so a missing elilo makes the whole dep repo uninstallable -- it MUST be required here, not
# silently tolerated (it builds from a tracked prebuilt on ppc64le/EL8, compiled elsewhere).
# A package whose builder was skipped is not required (else a clean --skip-* run fails).
my @all = qw(elilo-xcat ipmitool-xcat syslinux-xcat grub2-xcat xnba-undi
perl-IO-Stty perl-HTTP-Async perl-Net-HTTPS-NB xCAT-genesis-base);
my @req = required_pkgs(\@all, $skip_genesis, $skip_perl, $skip_xcat_dep);
# so a missing elilo makes the whole dep repo uninstallable -- it is listed in every manifest
# target section, so it is always part of the required set below (not silently tolerated).
# A package whose builder was skipped is not required (the caller applies required_pkgs()).
my @req = @$required_ref;
my @missing = grep { !have_rpm($dir, $_) } @req;
die "FATAL: required deps missing from $dir: @missing\n" if @missing;
print "[deps] required set present in $dir: @req\n";
t/mockbuild-all.t
+39 -1
View File
@@ -11,7 +11,8 @@ 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 rpm_version rpm_release rpm_is_signed
restamp_release_line cross_copy_genesis finalize_xcat_dep read_manifest);
restamp_release_line cross_copy_genesis finalize_xcat_dep read_manifest
bump_dep_release_suffix);
# Run a printing sub with STDOUT muted so its progress lines do not pollute TAP.
sub quiet(&) {
@@ -210,4 +211,41 @@ is(rpm_release(tempdir(CLEANUP => 1), 'nonexistent-pkg'), undef, 'rpm_release is
or diag("missing conserver-xcat in: @missing");
}
# ---- bump_dep_release_suffix: stamps xcat-dep specs, prunes nested xcat-core, idempotent --------
# Reviewer asked for a test on this path. It walks a tree, stamps the first Release: line of every
# xcat-dep spec, prunes a nested xcat-core/ checkout, and is idempotent on a re-run.
{
my $tmp = tempdir(CLEANUP => 1);
# (a) a top-level dep spec that MUST be stamped
open my $a, '>', "$tmp/a.spec" or die;
print $a "Name: a\nVersion: 1.0\nRelease: 5%{?dist}\n";
close $a;
# (b) a spec NESTED under xcat-core/ that MUST be pruned (left untouched)
make_path("$tmp/xcat-core");
open my $b, '>', "$tmp/xcat-core/b.spec" or die;
print $b "Name: b\nVersion: 1.0\nRelease: 9\n";
close $b;
# (c) a spec with no Release: line at all (ignored, never stamped)
open my $c, '>', "$tmp/c.spec" or die;
print $c "Name: c\nVersion: 1.0\n";
close $c;
my $n = quiet { bump_dep_release_suffix($tmp, '.snap202601010000') };
is($n, 1, 'bump_dep_release_suffix stamps exactly the one dep spec with a Release line');
my $a_after = do { open my $fh, '<', "$tmp/a.spec" or die; local $/; <$fh> };
like($a_after, qr/^Release: 5%\{\?dist\}\.snap202601010000$/m,
'a.spec Release now carries the CD suffix, macro preserved');
my $b_after = do { open my $fh, '<', "$tmp/xcat-core/b.spec" or die; local $/; <$fh> };
is($b_after, "Name: b\nVersion: 1.0\nRelease: 9\n",
'nested xcat-core/b.spec is pruned and left untouched');
# A SECOND call is idempotent: nothing newly stamped, a.spec content unchanged.
my $n2 = quiet { bump_dep_release_suffix($tmp, '.snap202601010000') };
is($n2, 0, 'a second bump_dep_release_suffix call stamps nothing (idempotent)');
my $a_again = do { open my $fh, '<', "$tmp/a.spec" or die; local $/; <$fh> };
is($a_again, $a_after, 'a.spec content unchanged on the idempotent second call');
}
done_testing;