fix(build): stop builddebs.pl leaving the checkout unbuildable and dirty

A second run in the same checkout died once the release string moved: dpkg-genbuildinfo: error: cannot fstat file ../xcat_..._ppc64el.deb debian/files accumulates one line per artifact and survives `dh_clean -d`, which removes directories only. The next build's dpkg-genchanges reads the stale entries and fstats artifacts collect_debs already moved away. The old shell builder deleted debian/files explicitly; that step was not carried over. --force does not help, since it only wipes the output repository. Three further ways the build did not put the tree back as it found it: xCAT/postscripts/{bmcsetup,getipmi} are TRACKED files that the xCAT build rewrites from the genesis sources. They were recorded as created, so cleanup deleted them from the checkout. They are claimed now. Restoring a claimed file lost its mode: File::Copy::copy does not carry permissions, so an executable came back 100644 with identical content -- visible only as a git mode change. backup_file/restore_file record and reapply it. debian/*.substvars are rewritten in place and several are tracked; they are claimed too. debhelper's .debhelper/ and *.debhelper.log are never tracked and are removed with the rest of the residue. clean_debian_residue runs from collect_debs, which is OUTSIDE with_prepared_tree -- the restore has already happened by then. That is why it must not remove *.substvars: doing so would delete the tracked ones it just put back. The claim mechanism handles those instead. Verified on xcat-master-ub: two consecutive full 14-package builds in one checkout both succeed, and a build against a git checkout now leaves zero tracked files modified or deleted (was five, two of them deleted). The remaining untracked residue -- pods/, share/, pod2htmd.tmp and the substvars of packages that do not track one -- is inherited from build-ubunturepo and unchanged here. Also covers the guard that gives this PR its name: deleting `return if $opts{source_only}` from buildall left buildrpms_source_only.t green, so nothing checked that --source-only skips the binary rebuild. It does now, and the mutation reddens two assertions. Same for the changelog trailer: making its substitution global left the suite green because only the older header was asserted, not its author and date. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
2026-09-04 12:07:56 +00:00 · 2026-09-01 08:26:30 -03:00
parent 96610f0e5c
commit 63c9ef8a1f
4 changed files with 208 additions and 14 deletions
@@ -14,8 +14,9 @@ package BuildUtils;
 use strict;
 use warnings;
 use Exporter 'import';
-use File::Copy qw(copy);
-use File::Path qw(make_path);
+use File::Copy qw(copy move);
+use File::Basename qw(basename);
+use File::Path qw(make_path remove_tree);
 use POSIX qw(strftime);

 our @EXPORT_OK = qw(
@@ -26,7 +27,8 @@ our @EXPORT_OK = qw(
    pin_control_version rewrite_changelog_header
    reprepro_distributions reprepro_options
    lock_id_for take_build_lock
-    sh_quote
+    sh_quote clean_debian_residue
+    backup_file restore_file
 );

 # The xCAT-probe helpers. xcat-probe reuses functions shipped by xCAT; they are COPIED
@@ -56,6 +58,75 @@ my @DEFAULT_DISTS = qw(focal jammy noble resolute);
 sub default_dists { return @DEFAULT_DISTS; }

 # sh_quote: single-quote a string for safe use in a shell command.
+# clean_debian_residue: remove what dpkg-buildpackage leaves inside a package's
+# debian/ directory.
+#
+# debian/files accumulates one line per artifact and is never truncated by
+# `dh_clean -d`, which only removes directories. dpkg-genchanges then reads the
+# stale entries on the next build and fstats artifacts that are no longer there:
+#   dpkg-genchanges: error: cannot fstat file ../perl-xcat_<old release>_amd64.buildinfo
+# so a second build in the same checkout dies as soon as the release string moves.
+# The staging directories go for the same reason the old shell builder removed
+# them -- they are the previous build's payload, not source.
+#
+# Call this only after a package's LAST architecture: debian/files carries the
+# amd64 artifacts that the ppc64el run's dpkg-genchanges still needs.
+# backup_file / restore_file: put a file back exactly as it was.
+#
+# File::Copy::copy does NOT carry permissions, so a naive backup-and-restore returns
+# an executable with its exec bit stripped -- the content compares equal and only
+# `git diff` notices the mode change. xCAT/postscripts/{bmcsetup,getipmi} are shipped
+# executable and are rewritten during the xCAT build, so this is not hypothetical.
+sub backup_file {
+    my ($path) = @_;
+    return unless defined $path && -f $path;
+    my $backup = "$path.build.save";
+    my $mode   = ( stat $path )[2] & 07777;
+    copy( $path, $backup ) or die "Cannot back up $path: $!\n";
+    return [ $backup, $path, $mode ];
+}
+
+sub restore_file {
+    my ($entry) = @_;
+    return 0 unless $entry;
+    my ( $backup, $path, $mode ) = @{$entry};
+    move( $backup, $path ) or do { warn "Could not restore $path: $!\n"; return 0; };
+    chmod $mode, $path if defined $mode;
+    return 1;
+}
+
+sub clean_debian_residue {
+    my ($package_root) = @_;
+    return () unless defined $package_root && -d "$package_root/debian";
+
+    my @removed;
+    my $files = "$package_root/debian/files";
+    if (-e $files) {
+        unlink $files or die "Cannot remove $files: $!\n";
+        push @removed, $files;
+    }
+
+    my $stem = lc(basename($package_root));
+    foreach my $dir (glob("$package_root/debian/$stem*")) {
+        next unless -d $dir;
+        remove_tree($dir);
+        push @removed, $dir;
+    }
+
+    # debhelper's own bookkeeping. Never tracked, and it accumulates per build.
+    # glob returns a wildcard-free pattern verbatim whether or not it exists, so
+    # the -e guard is what makes a second call a no-op rather than a fatal unlink.
+    foreach my $residue (glob("$package_root/debian/*.debhelper.log"),
+                         "$package_root/debian/.debhelper") {
+        next unless -e $residue;
+        if (-d $residue) { remove_tree($residue); }
+        else { unlink $residue or die "Cannot remove $residue: $!\n"; }
+        push @removed, $residue;
+    }
+
+    return @removed;
+}
+
 sub sh_quote {
    my ($s) = @_;
    $s = '' if !defined $s;
@@ -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 upstream_version resolve_dest
+    orig_tarball_name upstream_version resolve_dest clean_debian_residue
+    backup_file restore_file
    pin_control_version rewrite_changelog_header
    reprepro_distributions reprepro_options
    lock_id_for take_build_lock sh_quote
@@ -148,9 +149,7 @@ sub with_prepared_tree {
        my ($rel) = @_;
        my $path = "$dir/$rel";
        if (-f $path) {
-            my $backup = "$path.build.save";
-            copy($path, $backup) or die "Cannot back up $path: $!\n";
-            push @restore, [$backup, $path];
+            push @restore, backup_file($path);
        }
        else {
            push @remove, $path;
@@ -159,6 +158,8 @@ sub with_prepared_tree {

    $claim->('debian/control');
    $claim->('debian/changelog');
+    # dpkg rewrites debian/<pkg>.substvars in place, and several of them are tracked.
+    $claim->("debian/" . basename($_)) for glob("$dir/debian/*.substvars");

    # Pin the intra-xCAT dependencies to this exact build, so a partial upgrade cannot
    # mix versions.
@@ -191,13 +192,16 @@ sub with_prepared_tree {
            my $src = "$ROOT/xCAT-genesis-scripts/usr/bin/$f";
            next unless -f $src;
            my $dst = "$dir/postscripts/$f";
+            # Both are TRACKED files. Claiming them backs the originals up and puts
+            # them back; treating them as created would delete them from the
+            # checkout, which is what happened before.
+            $claim->("postscripts/$f");
            my $text = do { open my $fh, '<', $src or die; local $/; <$fh> };
            $text =~ s/xcat\.genesis\.\Q$f\E/$f/g;
            open my $out, '>', $dst or die "Cannot write $dst: $!\n";
            print {$out} $text;
            close $out;
            chmod 0755, $dst;
-            push @added, $dst;
        }
    }
    # xCAT-genesis-scripts keeps a control file per architecture.
@@ -214,10 +218,7 @@ sub with_prepared_tree {
    my $err = $@;

    unlink @added, @remove;
-    for my $pair (reverse @restore) {
-        my ($backup, $path) = @$pair;
-        move($backup, $path) or warn "Could not restore $path: $!\n";
-    }
+    restore_file($_) for reverse @restore;
    die $err if $rc;
    return;
 }
@@ -270,6 +271,11 @@ sub collect_debs {
    # The rest of the dpkg output is build residue, not an artifact.
    unlink glob("$ROOT/*.buildinfo"), glob("$ROOT/*.changes"), glob("$ROOT/*.dsc"),
           glob("$ROOT/*.tar.xz"), glob("$ROOT/*.tar.gz");
+    # And the residue dpkg leaves inside the package -- debian/files survives
+    # `dh_clean -d` and would make the next build with a different release fstat
+    # artifacts this run already moved away. Safe here: this runs after the last
+    # architecture, so no dpkg-genchanges still needs it.
+    clean_debian_residue("$ROOT/$pkg");
    return $moved;
 }

@@ -24,7 +24,8 @@ use BuildUtils qw(
    deb_package_arches dist_arches
    orig_tarball_name upstream_version resolve_dest
    pin_control_version rewrite_changelog_header
-    reprepro_distributions reprepro_options sh_quote
+    reprepro_distributions reprepro_options sh_quote clean_debian_residue
+    backup_file restore_file
 );

 # ------------------------------------------------------------------- versions --
@@ -137,6 +138,14 @@ like( $rewritten, qr/^ -- xCAT Build <build\@xcat\.invalid>  Sat, 24 Aug 2026 08
    'and the deterministic date, so two builds of one commit match' );
 like( $rewritten, qr/^xcat \(2\.17\.0\) unstable/m,
    'the older stanza is left alone -- the history is not ours to rewrite' );
+# Its trailer too. This is the defect the old shell builder shipped: its sed had no
+# line address, so every trailer in the file was restamped and 2023 entries went out
+# authored by today's builder. Only the top stanza may move.
+like( $rewritten,
+    qr/^ -- Somebody Else <nobody\@example\.invalid>  Mon, 01 Jan 2023 00:00:00 \+0000$/m,
+    'including its author and date, which this build did not write' );
+is( scalar( () = $rewritten =~ /^ -- xCAT Build /mg ), 1,
+    'exactly one trailer is restamped, however many stanzas the file has' );

 # ------------------------------------------------------------------ reprepro --

@@ -200,4 +209,81 @@ like( reprepro_options('/some/gnupghome'), qr/^basedir \.$/m,
 is( sh_quote(q{it's}), q{'it'"'"'s'}, 'a single quote survives shell quoting' );
 is( sh_quote(undef), q{''}, 'undef quotes to the empty string' );

+# ------------------------------------------------ dpkg residue inside a package --
+# debian/files accumulates one line per artifact and survives `dh_clean -d`, so the
+# next build's dpkg-genchanges fstats artifacts that are no longer on disk and the
+# whole build dies. Only the residue goes; the packaging itself must stay.
+{
+    my $pkgroot = tempdir( CLEANUP => 1 ) . '/perl-xCAT';
+    make_path("$pkgroot/debian/perl-xcat/usr/share");
+    make_path("$pkgroot/debian/source");
+    for my $f (qw(debian/files debian/control debian/rules debian/changelog debian/source/format)) {
+        open my $fh, '>', "$pkgroot/$f" or die $!;
+        print {$fh} "stale\n";
+        close $fh;
+    }
+
+    # debhelper bookkeeping, never tracked, accumulates per build.
+    make_path("$pkgroot/debian/.debhelper/generated");
+    open my $dh, '>', "$pkgroot/debian/perl-xcat.debhelper.log" or die $!;
+    close $dh;
+
+    my @removed = clean_debian_residue($pkgroot);
+
+    ok( !-e "$pkgroot/debian/files",
+        'debian/files does not survive into the next build' );
+    ok( !-d "$pkgroot/debian/perl-xcat",
+        'nor does the staging tree of the build that just finished' );
+    ok( !-e "$pkgroot/debian/perl-xcat.debhelper.log",
+        'nor debhelper\'s per-build log' );
+    ok( !-d "$pkgroot/debian/.debhelper",
+        'nor its generated-state directory' );
+    is( scalar @removed, 4, 'and every one is reported as removed' );
+
+    ok( -f "$pkgroot/debian/control",  'debian/control is left alone' );
+    ok( -f "$pkgroot/debian/rules",    'debian/rules is left alone' );
+    ok( -f "$pkgroot/debian/changelog", 'debian/changelog is left alone' );
+    ok( -f "$pkgroot/debian/source/format", 'and so is the rest of debian/' );
+
+    is_deeply( [ clean_debian_residue($pkgroot) ], [],
+        'a second call has nothing left to remove' );
+    is_deeply( [ clean_debian_residue("$pkgroot/nonexistent") ], [],
+        'and a package that was never built is not an error' );
+}
+
+# ------------------------------------------------- putting a file back as it was --
+# The build rewrites tracked files and restores them afterwards. A restore that
+# loses the mode is invisible in a content diff and strips the exec bit off shipped
+# scripts -- xCAT/postscripts/{bmcsetup,getipmi} are executable and are rewritten
+# during the xCAT build.
+{
+    my $dir = tempdir( CLEANUP => 1 );
+    my $script = "$dir/postscript";
+    open my $fh, '>', $script or die $!;
+    print {$fh} "#!/bin/sh\noriginal\n";
+    close $fh;
+    chmod 0755, $script or die $!;
+
+    my $entry = backup_file($script);
+    ok( -f "$script.build.save", 'the original is set aside before the build edits it' );
+
+    open my $out, '>', $script or die $!;
+    print {$out} "rewritten by the build\n";
+    close $out;
+    chmod 0644, $script;
+
+    ok( restore_file($entry), 'and is put back afterwards' );
+    open my $in, '<', $script or die $!;
+    my $restored = do { local $/; <$in> };
+    close $in;
+    is( $restored, "#!/bin/sh\noriginal\n", 'with its original content' );
+    is( ( stat $script )[2] & 07777, 0755,
+        'and its original mode -- an executable must not come back unexecutable' );
+    ok( !-e "$script.build.save", 'leaving no backup behind' );
+
+    is( backup_file("$dir/never-existed"), undef,
+        'a file that is not there is not claimed' );
+    is( restore_file(undef), 0, 'and restoring nothing is not an error' );
+}
+
 done_testing();
@@ -30,7 +30,7 @@ my $source = read_text($builder);
 # rather than skip: if the extraction stops matching, this file would silently
 # cover nothing.
 my %routine;
-for my $name (qw(index_repo write_repo_metadata_dir)) {
+for my $name (qw(index_repo write_repo_metadata_dir buildall)) {
    my ($body) = $source =~ /\n(sub \Q$name\E \{.*?\n\})\n/s;
    BAIL_OUT("could not extract $name from buildrpms.pl") unless $body;
    $routine{$name} = $body;
@@ -38,12 +38,18 @@ for my $name (qw(index_repo write_repo_metadata_dir)) {

 our @CREATEREPO;
 our @METADATA_WRITTEN;
+our @STAGES;

 {
    package Scratch;
    no warnings 'redefine';
    # Collaborators the lifted code calls. Each records instead of acting.
    sub createrepo_dir { push @main::CREATEREPO, $_[0]; }
+    # The four stages buildall drives. Each records that it was reached.
+    sub createmockconfig { push @main::STAGES, 'createmockconfig'; }
+    sub buildsources     { push @main::STAGES, 'buildsources'; }
+    sub buildspkgs       { push @main::STAGES, 'buildspkgs'; }
+    sub buildpkgs        { push @main::STAGES, 'buildpkgs'; }
 }

 # %opts lives in the scratch package and is set directly. Aliasing it to a hash in
@@ -57,6 +63,7 @@ my $harness = join "\n",
    # write_repo_metadata_dir does real work past the guard; stop it there so the
    # test observes the guard and nothing else.
    $routine{index_repo},
+    $routine{buildall},
    ($routine{write_repo_metadata_dir} =~ s/(return if \$opts\{source_only\};).*\n\}\z/$1\n    push \@main::METADATA_WRITTEN, \$repodir;\n    return 1;\n}/sr),
    '1;';

@@ -101,6 +108,30 @@ ok( !grep({ $_ eq 'BINARY' } @{ run_index(source_only => 1) }),
        'a normal run still emits the repository metadata' );
 }

+
+# ------------------------------------------------------------------ the build --
+# The point of the option: the source rpm is built and the binary rebuild is not.
+# Without this, removing the guard from buildall leaves every other assertion in
+# this file green -- the repository ones only observe what index_repo does.
+sub stages_for {
+    my ($source_only) = @_;
+    %Scratch::opts = (source_only => $source_only);
+    local @STAGES = ();
+    Scratch::buildall('xCAT-vlan', 'alma+epel-9-x86_64');
+    return [@STAGES];
+}
+
+is_deeply( stages_for(0),
+    [qw(createmockconfig buildsources buildspkgs buildpkgs)],
+    'a normal run builds the source rpm and then rebuilds it into binaries' );
+
+is_deeply( stages_for(1),
+    [qw(createmockconfig buildsources buildspkgs)],
+    'a source-only run stops once the source rpm exists' );
+
+ok( !grep( { $_ eq 'buildpkgs' } @{ stages_for(1) } ),
+    'and never enters the binary rebuild, which is the expensive half' );
+
 # ------------------------------------------------------------------- the CLI --
 # Run the real program. --source-only and --merge-core-repos are different modes:
 # one builds, the other assembles trees that are already built.