FreeAndNil commented on PR #330:
URL: https://github.com/apache/logging-log4net/pull/330#issuecomment-6122184519
Implementation plan for #330, kept here for the record. Revised after the
merge so that it describes the change as it landed.
<details>
<summary>Plan</summary>
# 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.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
