From 2bfad97348bbd535a18cf6925d67bbbea8c040a9 Mon Sep 17 00:00:00 2001 From: Daniel Hilst <392820+dhilst@users.noreply.github.com> Date: Tue, 1 Sep 2026 07:24:48 -0300 Subject: [PATCH] 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 _.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-. 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// 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> --- BuildUtils.pm | 34 +++++++++++++++++++++++++++++++--- Gitinfo | 1 + builddebs.pl | 33 +++++++++++++++++++++++---------- github_action_xcat_test.pl | 10 +++++++++- xCAT-test/unit/build_utils.t | 29 +++++++++++++++++++++++++---- 5 files changed, 89 insertions(+), 18 deletions(-) create mode 100644 Gitinfo diff --git a/BuildUtils.pm b/BuildUtils.pm index 306d7f7cb..b417ef243 100644 --- a/BuildUtils.pm +++ b/BuildUtils.pm @@ -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 +# _.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. diff --git a/Gitinfo b/Gitinfo new file mode 100644 index 000000000..8c55c71c5 --- /dev/null +++ b/Gitinfo @@ -0,0 +1 @@ +5515dfbe3e5ac3a305a60422def89f12e6318ce8 diff --git a/builddebs.pl b/builddebs.pl index 915c792b9..d7214f015 100755 --- a/builddebs.pl +++ b/builddebs.pl @@ -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-. 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); diff --git a/github_action_xcat_test.pl b/github_action_xcat_test.pl index e9ca2e5d7..f4480771b 100644 --- a/github_action_xcat_test.pl +++ b/github_action_xcat_test.pl @@ -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", diff --git a/xCAT-test/unit/build_utils.t b/xCAT-test/unit/build_utils.t index 199872a68..12b118ddc 100644 --- a/xCAT-test/unit/build_utils.t +++ b/xCAT-test/unit/build_utils.t @@ -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 _.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 --