From 5b5515596858b278725343b2d4241742837a511e Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Tue, 22 Sep 2026 09:22:24 +0200 Subject: [PATCH 1/2] ci: the import table is read by a job of its own, and the second start speaks as a note Two items of the owner's queue, both about the software renderer. The guard that builds the window with cgo and reads its PE import table was the test matrix's only cold cgo build: CGO_ENABLED is 0 there, so nothing else compiles the OpenGL binding or GLFW. Measured 2026-09-17, 822 s for the Windows test step against 434 s warm, four minutes under the timeout, and cold again after every change to go.sum - which is every Dependabot pull request. It runs in a job of its own now, with a cache of its own, and the matrix asks it to skip by a variable declared once in the guard's source. A skip is the shape this project has been caught by: a guard that stops reaching the state it watches and is green honestly. So the job reads its log for the test's PASS line under pipefail - go test with a -run pattern matching nothing exits 0, and so does a skip on a runner without gcc, both measured - and a new guard reads the test's name and the variable out of the guard's own source and holds ci.yml to both halves. Three mutations for it, and the notices mutation that #112 left stale points at the built version again. The line written to standard error before the second start said the driver offers no OpenGL 2.1 as a fact. The program has checked only that the first attempt gave no window, so the line says that, names the usual cause as usual, and says what is being done - the owner's decision of 2026-09-17. Rejected: leaving the guard in the matrix with the measured four minutes of room, because a runner thirty percent slower turns that into a timeout that names whichever test happened to be running. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 57 ++++++++++ CHANGELOG.md | 8 ++ internal/guard/importtablejob_test.go | 156 ++++++++++++++++++++++++++ internal/guard/noimport_test.go | 20 +++- internal/gui/text/locale/en.json | 2 +- internal/gui/text/text.go | 14 ++- 6 files changed, 252 insertions(+), 5 deletions(-) create mode 100644 internal/guard/importtablejob_test.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7db803d..5700ebc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -57,6 +57,15 @@ jobs: # Nothing of ours is involved and there is nothing of ours to fix, so that # one runner pays for a C toolchain it already has. CGO_ENABLED: ${{ matrix.os == 'macos-latest' && '1' || '0' }} + # The one guard that builds the window WITH cgo is read by the imports + # job below instead, and this asks it to skip here. Measured 2026-09-17: + # with CGO off, nothing else in this job compiles the OpenGL binding or + # GLFW, so that guard was the run's one cold cgo build - 822 s for the + # Windows test step against 434 s warm, four minutes under the timeout, + # and cold again after every change to go.sum. The variable is named in + # exactly one place, internal/guard/noimport_test.go, and a guard there + # holds this job to setting it and the imports job to not setting it. + TFG_IMPORT_TABLE_JOB: "1" steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -313,6 +322,54 @@ jobs: fi shell: bash + imports: + name: import table of the window binary + # One guard, in a job of its own, because it is the one guard that builds + # the window WITH cgo: TestTheWindowBinaryDoesNotImportOpenGLAtLoadTime + # links tfg-gui.exe the way a release does and reads its import table, + # which is how the software renderer's whole premise is held (O218). In + # the test matrix that build was the run's only cold cgo build - measured + # 2026-09-17, 822 s for the Windows test step against 434 s warm, four + # minutes under the timeout, and cold again after every change to go.sum, + # which is every Dependabot pull request. The matrix asks the guard to + # skip there (TFG_IMPORT_TABLE_JOB), and this job runs it. + # + # The cache is this job's own. setup-go keys its cache on the hash of the + # files named here (source of v7.0.0, cache-restore.ts), and the matrix + # job on the same runner keys on go.sum alone. Under one key the matrix + # would save first, without a single cgo object, and this job would pay + # the cold build on every run. The second file is the sum of the patched + # OpenGL binding, which is what the cgo build actually compiles - so the + # key moves when either input to those objects does. + # + # Two things make the step fail rather than pass on nothing, and both were + # measured before they were written: go test with a -run pattern that + # matches no test exits 0 saying "no tests to run", and the guard skips + # with exit 0 on a runner without gcc. So the log is read for the PASS + # line of the one test this job exists for, under pipefail so a failing + # go test is not hidden behind tee. The name in the pattern is held equal + # to the function in internal/guard/noimport_test.go by a guard. + runs-on: windows-latest + timeout-minutes: 15 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + go-version: ${{ env.GO_VERSION }} + check-latest: false + cache: true + cache-dependency-path: | + go.sum + third_party/go-gl-gl/PATCH.md + + - name: the window binary imports no opengl32.dll at load time + run: | + set -euo pipefail + go test -tags "$(cat .github/build-tags)" ./internal/guard/ -count=1 -run '^TestTheWindowBinaryDoesNotImportOpenGLAtLoadTime$' -v -timeout 12m 2>&1 | tee import-table.log + grep -q -- '--- PASS: TestTheWindowBinaryDoesNotImportOpenGLAtLoadTime' import-table.log + shell: bash + govulncheck: name: known vulnerabilities runs-on: ubuntu-latest diff --git a/CHANGELOG.md b/CHANGELOG.md index 1fb45d8..6dde699 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,14 @@ because it turns other people's test suites red. ### Changed +- **The line before the second start says what was checked, not what was + guessed.** When the window's first attempt gives no window and the program + starts again with the software renderer shipped beside it (Windows), the + line on standard error used to state that the graphics driver offers no + OpenGL 2.1. That is the usual cause and the one measured, but the only + thing the program has checked at that point is that the first attempt did + not succeed. The line now says that, names the usual cause as usual rather + than as fact, and then says what is being done about it. - **Two names are compared with a corrected Unicode normaliser.** The library that decides whether two file names are one name spelled two ways, `golang.org/x/text`, moves from 0.41.0 to 0.42.0, and that release fixes how diff --git a/internal/guard/importtablejob_test.go b/internal/guard/importtablejob_test.go new file mode 100644 index 0000000..b961baa --- /dev/null +++ b/internal/guard/importtablejob_test.go @@ -0,0 +1,156 @@ +package guard + +import ( + "os" + "path/filepath" + "regexp" + "strings" + "testing" +) + +// The one guard that builds the window with cgo is skipped by the test matrix +// and run by a CI job of its own - two halves that only mean anything together. +// +// Half of that is a skip, and a skip is the shape this project has been caught +// by more than once: a guard that stops reaching the state it watches and is +// green honestly. Here the ways are concrete. The matrix sets the variable and +// the job misspells the test's name in its -run pattern, so go test says "no +// tests to run" and exits 0. The test is renamed and the workflow is not. The +// job ends up on a runner without gcc, the guard skips, and the step is green. +// The variable is set in the job too, by a copy and paste, and the guard skips +// everywhere. Each of those leaves every check green and the import table read +// by nobody, which is the whole premise of the software renderer gone (O218). +// +// So this reads the guard's own source for the test's name and the variable it +// consults, and holds ci.yml to both: exactly one job runs that test by name, +// on Windows, without the variable, and reads its own log for the PASS line - +// and the matrix job sets the variable. Nothing here is typed twice. +func TestTheImportTableGuardIsRunByTheJobThatNamesIt(t *testing.T) { + root := repoRoot(t) + raw, err := os.ReadFile(filepath.Join(root, ".github", "workflows", "ci.yml")) + if err != nil { + t.Skipf("the workflow is not here: %v", err) + } + name, variable := importTableGuard(t, root) + + jobs := ciJobs(string(raw)) + if len(jobs) < 5 { + t.Fatalf("only %d job(s) were read out of ci.yml, so this guard is not reading the file it thinks it is", len(jobs)) + } + + // The job that names the test, by its -run pattern anchored on both ends - + // a pattern that merely contains the name would also match a renamed test + // that starts the same way. + pattern := "-run '^" + name + "$'" + var runners []string + for job, block := range jobs { + if strings.Contains(block, pattern) { + runners = append(runners, job) + } + } + if len(runners) != 1 { + t.Fatalf("%d job(s) in ci.yml run %s by name, and exactly one has to.\n"+ + "The matrix skips it when %s is set, so a job that no longer names it leaves the import "+ + "table read by nobody - and go test with a pattern that matches no test exits 0.\n"+ + "Jobs found: %s", len(runners), name, variable, strings.Join(runners, ", ")) + } + job := runners[0] + block := jobs[job] + t.Logf("job %q runs %s", job, name) + + if !strings.Contains(block, "runs-on: windows-latest") { + t.Errorf("job %q runs %s somewhere other than windows-latest, where the guard skips because "+ + "a Windows binary with cgo cannot be built there", job, name) + } + if strings.Contains(block, variable+":") { + t.Errorf("job %q sets %s, which asks the very guard it exists to run to skip", job, variable) + } + // The PASS line, because the two ways this step can be green on nothing + // were both measured: a pattern matching no test, and a skip on a runner + // without a C compiler. Reading the log for the test's own PASS line is + // what turns either into a red step. + if !strings.Contains(block, "'--- PASS: "+name+"'") { + t.Errorf("job %q does not read its log for the line '--- PASS: %s'.\n"+ + "Without it the step is green when go test finds no such test and when the guard skips "+ + "for want of gcc - measured, both exit 0.", job, name) + } + if !strings.Contains(block, "pipefail") { + t.Errorf("job %q pipes go test through tee without pipefail, so a failing go test is "+ + "hidden behind tee's exit code", job) + } + + // And the other half: the matrix skips it, by the variable the guard reads. + matrix, ok := jobs["test"] + if !ok { + t.Fatal("ci.yml has no job called test, so the matrix that is meant to skip the guard cannot be checked") + } + if !strings.Contains(matrix, variable+": \"1\"") { + t.Errorf("the test matrix does not set %s, so the cold cgo build this job exists to take out "+ + "of the matrix is still in it - measured at 822 s against 434 s warm", variable) + } +} + +// importTableGuard reads the name of the guard that builds the window with cgo +// and the name of the variable it skips on, out of its own source. +// +// Anchored on the function whose first statement consults the variable, so +// that the name read here is the name of the test that actually skips - a +// second test in the same file would otherwise answer for it. +func importTableGuard(t *testing.T, root string) (name, variable string) { + t.Helper() + source, err := os.ReadFile(filepath.Join(root, "internal", "guard", "noimport_test.go")) + if err != nil { + t.Fatalf("reading the guard's source: %v", err) + } + head := regexp.MustCompile(`(?m)^func (Test\w+)\(t \*testing\.T\) \{\n\s*if os\.Getenv\(importTableJobVariable\)`) + m := head.FindStringSubmatch(string(source)) + if m == nil { + t.Fatal("noimport_test.go has no test whose first statement consults importTableJobVariable, so there is nothing for the matrix to skip") + } + name = m[1] + decl := regexp.MustCompile(`(?m)^const importTableJobVariable = "([A-Z][A-Z0-9_]*)"`) + v := decl.FindStringSubmatch(string(source)) + if v == nil { + t.Fatal("noimport_test.go does not declare importTableJobVariable as a literal, so the workflow cannot be held to its name") + } + return name, v[1] +} + +// ciJobs cuts the workflow into its jobs, keyed by job name, with comments +// taken out of each. A job runs from its key to the next key at the same +// indentation, the way nextJobAfter reads it, and only keys under jobs: count - +// the same correction the timeout guard needed, because the triggers under on: +// sit at that indentation too. Cut before the comments go rather than after, +// because a comment line at a job's indentation is a line nextJobAfter knows to +// step over and a blank one is not. +func ciJobs(text string) map[string]string { + _, after, found := strings.Cut(text, "\njobs:") + if !found { + return nil + } + jobs := map[string]string{} + key := regexp.MustCompile(`^ ([A-Za-z_][A-Za-z0-9_-]*):`) + for after != "" { + nl := strings.Index(after, "\n") + var line string + if nl < 0 { + line, after = after, "" + } else { + line, after = after[:nl], after[nl+1:] + } + m := key.FindStringSubmatch(line) + if m == nil { + continue + } + block := line + "\n" + after + end := nextJobAfter(block) + if end > 0 { + jobs[m[1]] = withoutYamlComments(block[:end]) + after = block[end:] + } else { + jobs[m[1]] = withoutYamlComments(block) + after = "" + } + } + return jobs +} diff --git a/internal/guard/noimport_test.go b/internal/guard/noimport_test.go index 93d10db..c136ee8 100644 --- a/internal/guard/noimport_test.go +++ b/internal/guard/noimport_test.go @@ -38,8 +38,26 @@ import ( // when it has none rather than passing. It is a skip and not a failure on // the runners that are not Windows, and on a Windows machine without gcc, // because a guard that cannot be run is not a guard that passed - the one -// that can run is the Windows job of CI, which has the compiler. +// that can run is a Windows job of CI, which has the compiler. +// +// Which Windows job is the point of the variable below. Measured on +// 2026-09-17 (docs/GUI-SOFTWARE-RENDERER-2026-09-17.md section 6.1): the test +// matrix runs with CGO_ENABLED=0, so nothing else in that job compiles the +// OpenGL binding or GLFW, and the build here was the one cold cgo build of +// the run - 822 s for the Windows test step against 434 s with a warm cache, +// with four minutes left under the step's timeout. Every change to go.sum +// makes the cache cold again. So CI runs this guard in a job of its own, with +// a cache of its own, and asks the matrix to skip it. The skip is not a +// preference and is never taken by itself: it is taken only when that job +// exists to run the guard instead, and a guard in internal/guard holds the +// workflow to that - the job names this test, the matrix sets this variable, +// and the job does not. +const importTableJobVariable = "TFG_IMPORT_TABLE_JOB" + func TestTheWindowBinaryDoesNotImportOpenGLAtLoadTime(t *testing.T) { + if os.Getenv(importTableJobVariable) != "" { + t.Skipf("skipped here on purpose: %s is set, so the import table is read by the CI job that builds the window with cgo and a warm cache - see ci.yml", importTableJobVariable) + } if runtime.GOOS != "windows" { t.Skipf("the import table being read is a Windows one, and a Windows binary with cgo cannot be built on %s", runtime.GOOS) } diff --git a/internal/gui/text/locale/en.json b/internal/gui/text/locale/en.json index 17e74e2..267c51d 100644 --- a/internal/gui/text/locale/en.json +++ b/internal/gui/text/locale/en.json @@ -330,7 +330,7 @@ }, "RendererStartingAgain": { "description": "About the software renderer shipped beside the program on Windows: in the system dialog and on standard error when the window could not be opened even so, on standard error when a window starts drawing with it, and on the About screen while it does.", - "other": "The graphics driver on this computer offers no OpenGL 2.1. Starting again with the software renderer shipped beside the program." + "other": "The first attempt to open a window did not succeed - usually a graphics driver without OpenGL 2.1. Starting again with the software renderer shipped beside the program." }, "SectionCarriedBeside": { "description": "The heading over a group of fields.", diff --git a/internal/gui/text/text.go b/internal/gui/text/text.go index 3cd0ef6..ca4f40d 100644 --- a/internal/gui/text/text.go +++ b/internal/gui/text/text.go @@ -283,10 +283,18 @@ func WindowRefused(cause string) string { // rather than words and are not translated. // StartingAgainWithSoftwareRenderer is the line the first process writes to -// standard error before it starts the second one: the driver refused, and -// this is what is being done about it. +// standard error before it starts the second one: the first attempt gave no +// window, and this is what is being done about it. +// +// Written as a note rather than a finding, and the owner asked for that on +// 2026-09-17. The sentence used to state that the driver offers no OpenGL +// 2.1, which is the usual cause and the one measured (O218) - but the only +// thing the program has actually checked at this point is that the toolkit +// refused a window. Stating the cause as a fact would be a claim the code +// never made, on the one machine where somebody is reading standard error +// to find out what happened. func StartingAgainWithSoftwareRenderer() string { - return say("RendererStartingAgain", "The graphics driver on this computer offers no OpenGL 2.1. Starting again with the software renderer shipped beside the program.") + return say("RendererStartingAgain", "The first attempt to open a window did not succeed - usually a graphics driver without OpenGL 2.1. Starting again with the software renderer shipped beside the program.") } // DrawingWithSoftwareRenderer is what a window drawn in software says about From 870fd11d5032881682aaf5a2fed805a82b10c13f Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Tue, 22 Sep 2026 09:40:23 +0200 Subject: [PATCH 2/2] ci: two things an outside review of #117 asked for The wording guard. TestTheSecondAttemptIsTakenOnlyWhereTheRendererCanBe reads the expected line from the function it tests, so a revert of the new wording would leave it green. TestTheSecondStartSpeaksAsANoteNotAFinding holds the words that make the line a note rather than a finding, with a mutation that puts the old sentence back. persist-credentials: false on the checkout of the import table job. The checkout keeps the token in .git/config unless told not to, and the job runs go test over the pull request's own code. The other twenty-two checkouts in these workflows are the same shape and are O230, a change of their own. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 7 +++++++ internal/guard/softwarerenderer_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5700ebc..2764959 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -353,6 +353,13 @@ jobs: timeout-minutes: 15 steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + # The checkout keeps the job's token in .git/config unless told not + # to, and this job goes on to run go test over the pull request's + # own code. Nothing here pushes, so the token has no use after the + # checkout. Asked for by an outside review of #117, and the other + # checkouts in these workflows are O230. + with: + persist-credentials: false - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: diff --git a/internal/guard/softwarerenderer_test.go b/internal/guard/softwarerenderer_test.go index 2864cf2..9b816c5 100644 --- a/internal/guard/softwarerenderer_test.go +++ b/internal/guard/softwarerenderer_test.go @@ -284,3 +284,28 @@ func TestTheAboutScreenSaysWhenTheWindowIsDrawnInSoftware(t *testing.T) { t.Errorf("the About screen of a window drawn in software does not say so.\nIt shows:\n%s", shown) } } + +// The line before the second start says what was checked, and names the +// cause as the usual one rather than as a fact. +// +// The program has checked exactly one thing when it writes this line: the +// first attempt gave no window. The sentence used to state that the driver +// offers no OpenGL 2.1, which is the usual cause and the measured one (O218) +// and is still more than the code knows - the owner asked on 2026-09-17 for +// a note, not a finding. The guard above reads the expected text from the +// function it tests, so it would be green after a revert of the wording. This +// one holds the two words that make it a note, and was asked for by an +// outside review of #117. +func TestTheSecondStartSpeaksAsANoteNotAFinding(t *testing.T) { + line := text.StartingAgainWithSoftwareRenderer() + if !strings.Contains(line, "usually") { + t.Errorf("the line before the second start names the cause without saying it is the usual one:\n %s\n"+ + "The program has only checked that the first attempt gave no window, so the driver is the usual cause, not a finding.", line) + } + if !strings.Contains(line, "first attempt") { + t.Errorf("the line before the second start does not say what was actually checked - that the first attempt gave no window:\n %s", line) + } + if strings.Contains(line, "offers no OpenGL") { + t.Errorf("the line before the second start states as a fact that the driver offers no OpenGL, which the program never checked:\n %s", line) + } +}