mirror of
https://github.com/xcat2/xcat-core.git
synced 2026-09-04 12:07:56 +00:00
fix(build): address review on builddebs.pl, and repoint CI at the new repo
Four fixes from @viniciusferrao's review plus the CI break his review predates. orig tarball version. dpkg looks for <source>_<upstream>.orig.tar.gz with no Debian revision, and the call site passed the full Version-Release. The rule now lives in BuildUtils::upstream_version and orig_tarball_name applies it, so the call site cannot get it wrong whichever string it is handed. Currently dormant -- every package is Format: 1.0, so the quilt branch does not run, which is why the differential build did not catch it. --dest could write to the filesystem root. Cwd::abs_path returns undef when a PARENT component is missing (a missing leaf is fine), and the caller interpolated that, so `--dest /no/such/parent/out` became `/debs` and `/xcat-core` at /. Replaced with BuildUtils::resolve_dest, which is rel2abs and purely lexical -- correct for an output directory that does not exist yet. Generated debian/control left behind. xCAT-genesis-scripts has no debian/control of its own; it is generated from control-<arch>. The cleanup restored only files that already existed, so the generated one stayed. Worse than dirty: ppc64el ran last, so the restore put back the amd64 BACKUP and the leftover was the wrong architecture's control, which a later single-arch build would have started from. with_prepared_tree now records created files and removes them. Verified by a real build: the checkout is byte-clean afterwards, matching the oracle. CI install step. build-ubunturepo wrote its repo to $curdir/../../xcat-core, which under GitHub's work/<repo>/<repo> layout IS $RUNNER_WORKSPACE, so install_xcat's `./mklocalrepo.sh` happened to be in the directory it chdir'd to. builddebs.pl writes inside the checkout instead -- that outside-the-checkout path is what used to rm -rf the tree -- so install_xcat now names the script by its real location and fails with a clear message if the build produced no repository. This is what reddened xcat_pr_test at 2m13s; the builder itself was fine (the exact CI invocation, `./builddebs.pl --force` with no --dest, returns 0 with all 14 packages). The executable bit was already fixed before the review landed. Both new helpers are tested and mutation-verified: not stripping the revision reddens 3 assertions, swapping rel2abs back to abs_path reddens 2. Equivalence re-measured after these changes -- all 14 packages identical to build-ubunturepo in control and in every non-changelog file by md5. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
This commit is contained in:
Daniel Hilst
+31
-3
@@ -22,7 +22,8 @@ our @EXPORT_OK = qw(
|
||||
source_date_epoch snap_release deb_version
|
||||
stage_probe_helpers XCAT_PROBE_HELPERS
|
||||
deb_package_arches dist_arches default_dists
|
||||
orig_tarball_name pin_control_version rewrite_changelog_header
|
||||
orig_tarball_name upstream_version resolve_dest
|
||||
pin_control_version rewrite_changelog_header
|
||||
reprepro_distributions reprepro_options
|
||||
lock_id_for take_build_lock
|
||||
sh_quote
|
||||
@@ -132,10 +133,37 @@ sub dist_arches {
|
||||
}
|
||||
|
||||
# orig_tarball_name: the .orig.tar.gz dpkg-source expects for a 3.0 (quilt) package.
|
||||
# The name is lower-cased because dpkg requires a lower-case source package name.
|
||||
#
|
||||
# The name carries the UPSTREAM version only -- dpkg looks for
|
||||
# <source>_<upstream>.orig.tar.gz, with no Debian revision, because one upstream
|
||||
# tarball is shared by every revision built from it. The revision is stripped here
|
||||
# rather than at the call site so passing the full Version-Release cannot produce a
|
||||
# tarball dpkg will not find. Lower-cased because dpkg requires a lower-case source
|
||||
# package name.
|
||||
sub upstream_version {
|
||||
my ($version) = @_;
|
||||
return '' unless defined $version;
|
||||
$version =~ s/-[^-]*\z//; # drop the Debian revision, if any
|
||||
return $version;
|
||||
}
|
||||
|
||||
sub orig_tarball_name {
|
||||
my ($package, $version) = @_;
|
||||
return lc($package) . "_$version.orig.tar.gz";
|
||||
return lc($package) . '_' . upstream_version($version) . '.orig.tar.gz';
|
||||
}
|
||||
|
||||
# resolve_dest: turn a --dest argument into an absolute path.
|
||||
#
|
||||
# NOT Cwd::abs_path: that returns undef when a PARENT component is missing, and the
|
||||
# caller then interpolates undef, so `--dest /no/such/parent/out` silently becomes
|
||||
# `/debs` and `/xcat-core` at the filesystem root. rel2abs is purely lexical and
|
||||
# works for a path that does not exist yet, which is the normal case for an output
|
||||
# directory.
|
||||
sub resolve_dest {
|
||||
my ($dest, $default) = @_;
|
||||
return $default unless defined $dest && length $dest;
|
||||
require File::Spec;
|
||||
return File::Spec->rel2abs($dest);
|
||||
}
|
||||
|
||||
# pin_control_version: pin xCAT's inter-package dependencies to this exact build.
|
||||
|
||||
+23
-10
@@ -31,7 +31,8 @@ use BuildUtils qw(
|
||||
source_date_epoch snap_release deb_version
|
||||
stage_probe_helpers XCAT_PROBE_HELPERS
|
||||
deb_package_arches dist_arches default_dists
|
||||
orig_tarball_name pin_control_version rewrite_changelog_header
|
||||
orig_tarball_name upstream_version resolve_dest
|
||||
pin_control_version rewrite_changelog_header
|
||||
reprepro_distributions reprepro_options
|
||||
lock_id_for take_build_lock sh_quote
|
||||
);
|
||||
@@ -135,17 +136,29 @@ sub with_prepared_tree {
|
||||
my $dir = "$ROOT/$pkg";
|
||||
my @restore;
|
||||
|
||||
my $save = sub {
|
||||
my @remove;
|
||||
|
||||
# Back up a file the build is about to edit, or -- when it does not exist yet --
|
||||
# note that the build is CREATING it so it can be taken away again.
|
||||
# xCAT-genesis-scripts has no debian/control of its own; it is generated from
|
||||
# control-<arch>. Restoring only pre-existing files left that generated file in
|
||||
# the checkout, so the tree ended dirty and a later single-arch build would start
|
||||
# from the other architecture's control.
|
||||
my $claim = sub {
|
||||
my ($rel) = @_;
|
||||
my $path = "$dir/$rel";
|
||||
return unless -f $path;
|
||||
my $backup = "$path.build.save";
|
||||
copy($path, $backup) or die "Cannot back up $path: $!\n";
|
||||
push @restore, [$backup, $path];
|
||||
if (-f $path) {
|
||||
my $backup = "$path.build.save";
|
||||
copy($path, $backup) or die "Cannot back up $path: $!\n";
|
||||
push @restore, [$backup, $path];
|
||||
}
|
||||
else {
|
||||
push @remove, $path;
|
||||
}
|
||||
};
|
||||
|
||||
$save->('debian/control');
|
||||
$save->('debian/changelog');
|
||||
$claim->('debian/control');
|
||||
$claim->('debian/changelog');
|
||||
|
||||
# Pin the intra-xCAT dependencies to this exact build, so a partial upgrade cannot
|
||||
# mix versions.
|
||||
@@ -200,7 +213,7 @@ sub with_prepared_tree {
|
||||
my $rc = eval { $body->($dir); 1 } ? 0 : 1;
|
||||
my $err = $@;
|
||||
|
||||
unlink @added;
|
||||
unlink @added, @remove;
|
||||
for my $pair (reverse @restore) {
|
||||
my ($backup, $path) = @$pair;
|
||||
move($backup, $path) or warn "Could not restore $path: $!\n";
|
||||
@@ -340,7 +353,7 @@ SCRIPT
|
||||
# ----------------------------------------------------------------- main ------
|
||||
my $lock = take_build_lock($ROOT);
|
||||
|
||||
my $dest = $opts{dest} ? abs_path($opts{dest}) : "$ROOT/dist/debs";
|
||||
my $dest = resolve_dest($opts{dest}, "$ROOT/dist/debs");
|
||||
my $pkgdir = "$dest/debs";
|
||||
my $repo = "$dest/xcat-core";
|
||||
make_path($pkgdir);
|
||||
|
||||
@@ -351,7 +351,15 @@ sub build_xcat_core{
|
||||
#--------------------------------------------------------
|
||||
sub install_xcat{
|
||||
|
||||
my @cmds = ("sudo ./mklocalrepo.sh",
|
||||
my $repo = "$srcdir/dist/debs/xcat-core";
|
||||
unless (-x "$repo/mklocalrepo.sh") {
|
||||
print RED "[install_xcat] $repo/mklocalrepo.sh missing -- did the build run?\n";
|
||||
$check_result_str .= "> **INSTALL XCAT ERROR** : the build produced no apt repository ";
|
||||
print $check_result_str;
|
||||
return 1;
|
||||
}
|
||||
|
||||
my @cmds = ("sudo $repo/mklocalrepo.sh",
|
||||
"sudo chmod 777 /etc/apt/sources.list",
|
||||
"sudo echo \"deb [arch=amd64 allow-insecure=yes] http://xcat.org/files/xcat/repos/apt/latest/xcat-dep noble main\" >> /etc/apt/sources.list",
|
||||
"sudo echo \"deb [arch=ppc64el allow-insecure=yes] http://xcat.org/files/xcat/repos/apt/latest/xcat-dep noble main\" >> /etc/apt/sources.list",
|
||||
|
||||
@@ -22,7 +22,8 @@ use BuildUtils qw(
|
||||
source_date_epoch snap_release deb_version
|
||||
stage_probe_helpers XCAT_PROBE_HELPERS
|
||||
deb_package_arches dist_arches
|
||||
orig_tarball_name pin_control_version rewrite_changelog_header
|
||||
orig_tarball_name upstream_version resolve_dest
|
||||
pin_control_version rewrite_changelog_header
|
||||
reprepro_distributions reprepro_options sh_quote
|
||||
);
|
||||
|
||||
@@ -70,9 +71,29 @@ is_deeply( [dist_arches('noble')], ['amd64', 'ppc64el'],
|
||||
is_deeply( [dist_arches('saucy')], ['amd64'],
|
||||
'saucy predates ppc64el and serves only amd64' );
|
||||
|
||||
is( orig_tarball_name('xCAT-server', '2.19.0-snap1'),
|
||||
'xcat-server_2.19.0-snap1.orig.tar.gz',
|
||||
'the orig tarball name is lower-cased, as dpkg requires' );
|
||||
# dpkg looks for <source>_<upstream>.orig.tar.gz -- no Debian revision, because one
|
||||
# upstream tarball is shared by every revision built from it.
|
||||
is( orig_tarball_name('xCAT-server', '2.19.0-snap202608240826'),
|
||||
'xcat-server_2.19.0.orig.tar.gz',
|
||||
'the orig tarball carries the upstream version, not the Debian revision' );
|
||||
is( orig_tarball_name('xCAT-server', '2.19.0'),
|
||||
'xcat-server_2.19.0.orig.tar.gz',
|
||||
'and is the same name when handed the upstream version directly' );
|
||||
is( upstream_version('2.19.0-snap1'), '2.19.0', 'the Debian revision is stripped' );
|
||||
is( upstream_version('2.19.0'), '2.19.0', 'a bare upstream version is unchanged' );
|
||||
is( upstream_version('1.2.3-4-5'), '1.2.3-4', 'only the LAST hyphen separates the revision' );
|
||||
is( upstream_version(undef), '', 'an undefined version does not blow up' );
|
||||
|
||||
# resolve_dest must not use Cwd::abs_path: that returns undef when a PARENT component
|
||||
# is missing, and the caller then builds "/debs" and "/xcat-core" at the root.
|
||||
is( resolve_dest(undef, '/default/out'), '/default/out',
|
||||
'no --dest falls back to the default' );
|
||||
is( resolve_dest('', '/default/out'), '/default/out',
|
||||
'an empty --dest falls back too' );
|
||||
is( resolve_dest('/no-such-parent-xyz/out', '/default'), '/no-such-parent-xyz/out',
|
||||
'a --dest whose parent does not exist resolves to itself, never undef' );
|
||||
like( resolve_dest('relative/out', '/default'), qr{^/.*relative/out$},
|
||||
'a relative --dest becomes absolute' );
|
||||
|
||||
# ------------------------------------------------------------------- control --
|
||||
|
||||
|
||||
Reference in New Issue
Block a user