Skip to content

bind a release to the commit it was built from - #330

Merged
FreeAndNil merged 1 commit into
masterfrom
Feature/330-release-from-one-commit
Sep 28, 2026
Merged

FreeAndNil merged 1 commit into
masterfrom
Feature/330-release-from-one-commit

Conversation

@FreeAndNil

@FreeAndNil FreeAndNil commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

build-release.ps1 built the binaries from the working tree but archived the
local master ref, so the signed source zip need not match the signed binaries.

  • Refuse a dirty tree, archive HEAD, and ship a signed .manifest recording the
    commit and the artifact set. No origin/master check: that needs the network
    and forbids cutting a release from a tag or a release branch.
  • Both verifiers check the commit against the zip archive comment and the
    listed set against the files present. The hash and signature loops only see
    the files that are there, so an artifact deleted with its .sha512 and .asc
    passed before.
  • Everything the release writes gets LF, asserted in the tests. Set-Content
    writes CRLF on Windows, where a trailing CR breaks sha512sum on macOS and
    makes every manifest name compare unequal. Includes the .sha512 fix.
  • sign-log4net-libraries.sh gains set -euo pipefail and shopt -s nullglob. An
    empty directory iterated the glob patterns and still exited 0.

audit da18b6f-f025

@FreeAndNil FreeAndNil added this to the 3.5.0 milestone Sep 27, 2026
build-release.ps1 built the binaries from the working tree but archived the
local master ref, so the signed source zip need not match the signed binaries.

* Refuse a dirty tree, archive HEAD, and ship a signed .manifest recording the
  commit and the artifact set. No origin/master check: that needs the network
  and forbids cutting a release from a tag or a release branch.
* Both verifiers check the commit against the zip archive comment and the
  listed set against the files present. The hash and signature loops only see
  the files that are there, so an artifact deleted with its .sha512 and .asc
  passed before.
* Everything the release writes gets LF, asserted in the tests. Set-Content
  writes CRLF on Windows, where a trailing CR breaks sha512sum on macOS and
  makes every manifest name compare unequal. Includes the .sha512 fix from
  #328; #328 gets rebased onto this.
* sign-log4net-libraries.sh gains set -euo pipefail and shopt -s nullglob. An
  empty directory iterated the glob patterns and still exited 0.

audit da18b6f-f025
@FreeAndNil
FreeAndNil force-pushed the Feature/330-release-from-one-commit branch from 7f7cd24 to 1ed2acb Compare September 27, 2026 20:14
FreeAndNil added a commit that referenced this pull request Sep 27, 2026
build-release.ps1 built the binaries from the working tree but archived the
local master ref, so the signed source zip need not match the signed binaries.

* Refuse a dirty tree, archive HEAD, and ship a signed .manifest recording the
  commit and the artifact set. No origin/master check: that needs the network
  and forbids cutting a release from a tag or a release branch.
* Both verifiers check the commit against the zip archive comment and the
  listed set against the files present. The hash and signature loops only see
  the files that are there, so an artifact deleted with its .sha512 and .asc
  passed before.
* Everything the release writes gets LF, asserted in the tests. Set-Content
  writes CRLF on Windows, where a trailing CR breaks sha512sum on macOS and
  makes every manifest name compare unequal. Includes the .sha512 fix from
  #328; #328 gets rebased onto this.
* sign-log4net-libraries.sh gains set -euo pipefail and shopt -s nullglob. An
  empty directory iterated the glob patterns and still exited 0.

audit da18b6f-f025
@FreeAndNil
FreeAndNil merged commit a457590 into master Sep 28, 2026
3 checks passed
@FreeAndNil
FreeAndNil deleted the Feature/330-release-from-one-commit branch September 28, 2026 06:40
@FreeAndNil

Copy link
Copy Markdown
Contributor Author

Implementation plan for #330, kept here for the record. Revised after the merge so that it describes the change as it landed.

Plan

PR #330: bind a release to one commit

Context

f025 (LOW) of audit da18b6fd, scripts/build-release.ps1: the binaries are built from the working
tree (dotnet test, line 31) while the source zip comes from the local master ref
(git archive ... master, line 34). Nothing binds them. A dirty tree, a different branch checked
out, or a local master that is not the built commit produces an officially signed source zip
that does not correspond to the officially signed binaries, and reviewers who read the source
learn nothing about the shipped binaries. Sub-claim, verified: sign-log4net-libraries.sh has no
nullglob and no set -e, so a zero-artifact run iterates the literal glob strings, sets
DID_SOMETHING=1, fails every gpg call and still exits 0. The no-artifacts guard is dead code.

A second hole shows up while writing the verifier side: the hash and signature loops are driven
from the files present, so an artifact deleted together with its .sha512 and .asc passes
verification. Binding the release to a commit therefore ships a signed manifest that records the
commit and the artifact set, and both verifiers check both.

One commit.

Branch

Feature/330-release-from-one-commit, off local master. #328 is open and edits
build-release.ps1 (Write-HashAndSignature) and build-release.Tests.ps1 (one It). The LF
fix for the .sha512 files from #328 lands here, since the manifest needs the same rule, and #328
is rebased onto this.

The change

scripts/build-release.ps1

Restructure around a $Root and an $ArtifactDirectory variable and four small functions, so the
pushd/popd pairs go away:

  • Invoke-InDirectory, Push-Location with the action in a try/finally Pop-Location, so a
    throw inside cannot leave the caller in the wrong directory. Used for the archive, the zip and
    the site build.
  • Get-ReleaseCommit: in the root, git status --porcelain; any output throws, listing the dirty
    paths; otherwise returns git rev-parse --verify HEAD. Maintainer decision: clean tree and
    archive from HEAD, no comparison against origin/master
    , so no network call in the release
    script and a release can still be cut from a tag or a release branch.
  • Write-Manifest -Commit -Names: writes apache-log4net-$Version.manifest with one commit=
    line and one artifact= line per artifact, the manifest itself included. Written with
    -NoNewline and the lines joined by `n: Set-Content writes [Environment]::NewLine,
    so on Windows every name would carry a trailing CR into the comparison in verify-release.sh.
  • Remove-Directory: Test-Path then Remove-Item -Force -Recurse, in place of the
    -ErrorAction SilentlyContinue deletes, which also swallowed a failed delete and left a stale
    artifact behind for whoever copies the directory to dist.

The flow: clean build/, verify the tree and capture the commit, dotnet test, git archive --format=zip ... $CommitHash in place of master, zip the binaries, rename the two nupkgs, write
the manifest, then hash and sign every name in $ArtifactNames in one loop. The artifact list is
declared once at the top: the two nupkgs, the source and binaries zips, the two verifiers and the
manifest, seven artifacts. Write-HashAndSignature writes its .sha512 line with -NoNewline
and an explicit `n for the same reason as the manifest: the macOS sha512sum reads a CR
from a Windows build as part of the file name.

The release candidate number becomes a parameter, [ValidateRange('Positive')] [int]$Rc = 1, so
the tag is rc/$Version-rc$Rc rather than always rc1. build-preview.ps1 gets the same
validation on its $Preview number.

Note for the commit message, not a code change: git archive --format=zip stores the commit id in
the zip's archive comment, which is what makes the verifier check below independent of the
manifest.

scripts/verify-release.ps1 and scripts/verify-release.sh

Both verifiers gain the same two checks between the hash loop and the signature loop, with the
PowerShell side refactored into Get-Manifest, Assert-ArtifactSet, Assert-SourceCommit and
Assert-Signature:

  • The artifact set. Exactly one .manifest among the artifacts, or fail naming the count. Its
    artifact= lines must be non-empty, every listed name must be present, and every present
    artifact must be listed, each failure naming the offending names.
  • The source commit. Exactly one commit= line, exactly one *source*.zip, and the recorded
    commit must equal the zip's archive comment, read with ZipFile.OpenRead(...).Comment in
    PowerShell and unzip -z -qq in bash (-qq, or the Archive: banner lands in the comparison).
    Whitespace is trimmed on both sides.

No backward compatibility: each release ships the verifier built with it, so a release without a
manifest is simply refused.

scripts/sign-log4net-libraries.sh

set -euo pipefail, shopt -s nullglob with a one line comment, and quote "$f" in both gpg
arguments. verify-release.sh already opens exactly this way, so this is the file catching up
with its neighbour, not a new convention. The PowerShell twin does not have the hole: Get-Item
errors on no match.

Tests

scripts/build-release.Tests.ps1 and scripts/verify-release.Tests.ps1, Pester 5, discovered by
directory from .github/workflows/build.yaml. The scaffolding is
scripts/FakeCommands.TestHelper.ps1, which shadows native commands with real shim files on a
prepended PATH, not Pester Mock.

The helper gains a git case, or every new test runs against an empty $CommitHash:

  • git status --porcelain produces no output by default and one modified path when FAKE_DIRTY
    is set, driven by a -Dirty switch on Invoke-InScratchTree. The existing FAKE_FAIL switch
    only makes a command exit 1, which is not the same thing.
  • git rev-parse --verify HEAD echoes a fixed fake sha from FAKE_SHA, exposed to the tests as
    $script:FakeCommitHash, so git archive and the manifest get a real value.
  • The result gains Lines, the whole logged lines, for an assertion about an argument rather than
    about the command; Calls keeps the two-token projection.

Existing release tests change, because two git calls now precede dotnet test: 'signs nothing when the build fails', 'builds no site when signing fails' and the full run get 'git status', 'git rev-parse' prepended, the full run has seven gpg --armor calls and is renamed to "all seven
artifacts", and 'ships no artifact without a hash' expects 7.

New release tests:

  • 'writes every hash as one LF-terminated line': all seven .sha512 files match
    ^[0-9a-f]{128} \*\./\S+\n\z, read as bytes, so the CR cannot hide.
  • 'removes the artifacts of an earlier run': a planted stale nupkg is gone afterwards.
  • 'builds nothing when the working tree is dirty': -Dirty, expect Calls to be
    @('git status') alone and a non-zero exit. This is the regression test for the finding.
  • 'archives the commit it built': the one git archive line ends in the fake sha.
  • 'records the commit and the artifact set in the manifest': one manifest, the commit= line
    holds the fake sha, the seven artifact= lines equal the files present, and the file holds no
    CR.

New verifier tests, with two fixture helpers: Add-SourceArchive writes a zip whose archive
comment is a commit, as git archive produces, and Add-Manifest writes a manifest that lists
itself, as the real one does, both with their .sha512. Add-Artifact gains a -Content written
without a newline, so an empty content really is a zero byte file. The cases: no manifest; an
artifact the manifest does not list; a listed artifact that is missing; a manifest without a
commit line; a source archive whose comment is not the recorded commit. Each asserts the message
of its own check.

sign-log4net-libraries.sh has no test and bash is not in the Pester harness. Do not build one for
it; the change is three lines that mirror verify-release.sh, and the guard it repairs is
reachable by hand (cd to an empty directory, run it, expect exit 2).

Revert to fail: undo the tree check in place, no git stash, and confirm the dirty-tree test
fails.

Changelog

src/changelog/3.5.0/330-release-from-one-commit.xml, type="fixed", <issue id="330" link="https://github.com/apache/logging-log4net/pull/330"/>, both attributes. Prose only, no
bullets. Name what it means for a verifier: the signed source zip and the signed binaries now come
from one commit, a signed manifest ships beside them recording that commit and the artifact set,
and the verification scripts check both against the zip archive comment and the files present.
Close with (audit da18b6fd-f025, fixed by @FreeAndNil).
src/changelog/3.5.0/313-verify-release-keys-bypass.xml is the precedent that release-script
changes do get an entry.

Agent instructions

Add a "Release scripts" section with the three rules this change establishes: every file the
release writes gets LF on every platform, each release ships its own verifier so the verifiers
carry no backward compatibility, and the scripts are tested through the fake command helper on all
three CI operating systems, so the $IsWindows branch stays working and byte-level assertions go
where all three see them.

Verification

Invoke-Pester ./scripts -Output Detailed          # all four test files, was green before
dotnet build src/log4net.sln                      # 0 warnings, untouched by this change
bash -n scripts/sign-log4net-libraries.sh         # plus a by-hand run in an empty directory, expect exit 2

The release script itself is never run for real here: the Pester harness fakes dotnet, git,
gpg, zip and mvnw, so nothing is built, signed, tagged or pushed. CI runs the same suite on
macOS, Ubuntu and Windows, which is where the LF assertions earn their keep.

House rules

The maintainer commits. Leave the work unstaged, write the commit message to
.git/plans/330/commit.txt and stop. No git add, no git commit, no attribution trailer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants