diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7db803d..2764959 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,61 @@ 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 + # 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: + 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/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) + } +} 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