From f69316a454e5b46e76a93d70f6974270d4621277 Mon Sep 17 00:00:00 2001 From: Igor Barcik Date: Tue, 6 Oct 2026 09:18:37 +0200 Subject: [PATCH] docs: record the round 2 janitor sweep What changed in each phase, the verification evidence and its commands, the findings that were reported rather than fixed, and the limits of what could be checked on this machine. The graft graph was rebuilt after the source moves. --- docs/JANITOR_TASKS.md | 204 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 204 insertions(+) create mode 100644 docs/JANITOR_TASKS.md diff --git a/docs/JANITOR_TASKS.md b/docs/JANITOR_TASKS.md new file mode 100644 index 0000000..bf8c516 --- /dev/null +++ b/docs/JANITOR_TASKS.md @@ -0,0 +1,204 @@ +# Janitor round 2: structure, build separation, scripts + +Executed on branch `main` from the round 1 checkpoint. Round 1 had left all of its +work uncommitted and `HEAD` did not compile without the `FXProcessor` repair, so +this round begins by committing that work and then lands two phases of its own. +Round 1's own task list is archived at `docs/archive/JANITOR_TASKS_2026-05.md`. + +The user confirmed the scope before any file changed. The full set of decisions is +in the commit messages; this file records what changed, the evidence, and what was +left alone. + +## Commits + +| Commit | Contents | +| --- | --- | +| `e4cea30` | Round 1 checkpoint: the uncommitted `FXProcessor` repair, `ExactCompare.h`, `KnobLookAndFeel`, the `AGENT.md` to `AGENTS.md` renames, and graft integration. | +| `facd5c5` | Phase 1: `scripts/`, the toolchain file in `cmake/`, build output under `build/`, `clean.sh`, `docs/`, the Windows manifest generated from CMake, and three code fixes. | +| `4b0db50` | Phase 2: 36 source files into their module directories, and five directories renamed. | + +This file and the refreshed graft graph land in a fourth commit. + +## Phase 1: repository root, build output, scripts + +### Layout + +- `scripts/` holds `build_linux.sh`, `build_macos.sh`, `build_windows.sh`, + `build_harness.sh` and `clean.sh`. Each resolves the repository root from its own + location, so the documented `./scripts/build_harness.sh --run` works from any + directory. +- `cmake/` holds `mingw64-toolchain.cmake` beside `Plugin.cmake` and `Harness.cmake`. +- `docs/` holds `AUDIT_REPORT.md` and `UI_UX_AUDIT.md`; `docs/archive/` holds the + finished round 1 list. +- The repository root now holds `CMakeLists.txt`, `README.md`, `AGENTS.md`, + `.gitignore`, the tool configuration files, and four directories: `Source/`, + `cmake/`, `scripts/`, `docs/`. + +### Build output + +Each script defaults to a directory under `build/`: `build/linux`, `build/macos`, +`build/windows` and `build/harness`. `BUILD_DIR` still overrides any of them, and a +relative override resolves from the repository root. `SerumAlt_Windows/` remains the +staging directory for the shippable Windows bundle, and `OUTPUT_DIR` still overrides +it. + +`scripts/clean.sh` removes generated output: + +| Invocation | Removes | +| --- | --- | +| `./scripts/clean.sh` | the trees under `build/` | +| `--staging` | adds `SerumAlt_Windows/` | +| `--legacy` | adds `build_*` and `cmake-build-*` left in the root by earlier script versions | +| `--all` | both of the above | +| `--dry-run` | nothing; prints each path with its size | + +The script refuses a path outside the repository, a path under `Source/`, +`third_party/`, `.git`, `scripts/`, `cmake/`, `docs/` or `graft/`, a path tracked by +git, and refuses to run at all when `BUILD_DIR` or `OUTPUT_DIR` resolves outside the +repository. Exit status 2 marks a refusal. + +902 MB of stale trees were removed after the new layout built successfully: +`build/`'s original in-place tree, `build_harness`, `build_windows`, +`build_windows_fresh` and the three `build_verify_*` trees. `build_linux/` and +`SerumAlt_Windows/` were left in place; `./scripts/clean.sh --legacy --staging` +removes them when you no longer need the artifacts in them. + +### Windows manifest + +`build_windows.sh` used to carry a 60-line `moduleinfo.json` heredoc with the +version, vendor and class IDs written out by hand. The version also lives in +`project(SerumAlt VERSION 1.0.0)`, so a version bump would have shipped a manifest +that disagreed with the binary. + +`cmake/Plugin.cmake` now defines the plugin metadata once, passes it to +`juce_add_plugin`, and configures `cmake/moduleinfo.json.in` into the build +directory. The three VST3 class IDs are built from `PLUGIN_MANUFACTURER_CODE` and +`PLUGIN_CODE` the same way JUCE builds them, so they cannot drift from the codes +either. The script copies the configured file into the bundle and fails when it is +missing. + +### Code fixes + +- `Source/PluginProcessor.h` gained `using juce::AudioProcessor::processBlock;`. The + float override hid the base class double overload, which produced three warning + lines from three translation units. +- `Source/GUI/Slider.{h,cpp}` were deleted. Nothing constructed `serum::Slider`; it + was reachable only through one include in `PluginEditor.h`. `SERUMALT_INCLUDE_DIRS` + and `GUI/AGENTS.md` were updated to match, and `KnobLookAndFeel` was added to the + owned-files list, where it had been missing. +- Prose in the five shell scripts, the two `.claude/helpers/*.cjs` files and the + CMake files was tightened. The scripts had no em dashes and no banned vocabulary + left from round 1, so what changed was structure: numbered step headings, a + paragraph explaining what a load failure would look like, and the manifest + heredoc, all gone. + +### Unslop scope + +The five shell scripts, `.claude/helpers/graft-hooks.cjs`, +`.claude/helpers/graft-statusline.cjs`, `CMakeLists.txt`, `cmake/Plugin.cmake` and +`cmake/Harness.cmake`. `README.md` and `AGENTS.md` keep their prose and received +path corrections only, so the layout changes stay reviewable. + +## Phase 2: source modules + +The five directories `engine/`, `modulation/`, `params/`, `plugin/` and `synth/` +held nothing but guidance pointing at root-level implementations. Every module doc +already claimed its files, so the move only had to honour those claims. + +| Module | Files moved in | +| --- | --- | +| `Engine/` | `Engine.{h,cpp}` | +| `Synth/` | `Wavetable`, `Oscillator`, `SubOscillator`, `NoiseOscillator`, `Envelope`, `LFO`, `Filter`, `FilterBank`, `SynthVoice` (18 files) | +| `Modulation/` | `ModulationMatrix`, `MacroControls`, `RAVEButton` (6 files) | +| `Params/` | `Params.{h,cpp}` | +| `Plugin/` | `PluginProcessor.{h,cpp}` | +| `GUI/` | `PluginEditor.{h,cpp}` | +| `EffectUnits/` | `FXProcessor.{h,cpp}` | +| `Resources/` | `Resources.{h,cpp}` | + +36 files moved, not the 31 stated when the plan was agreed. The five directories +were renamed to `Engine/`, `Modulation/`, `Params/`, `Plugin/` and `Synth/` so that +every subdirectory of `Source/` uses one casing rule. + +`Source/ExactCompare.h` stays at the `Source/` root: five modules include it, and a +`Common/` directory for one header would cost more than it explains. + +`SERUMALT_INCLUDE_DIRS` now lists `Source/` and one directory per module, which let +all 31 parent-relative includes become plain names such as `Params.h`. No header +basename is duplicated in the tree, so that resolution cannot pick the wrong file. + +The module docs list owned files instead of describing a move that had not happened. +`README.md` and the `Current source layout` section of `AGENTS.md` describe the tree +that exists. + +## Verification + +Every command below was run after the phase it verifies. Build directories are under +`build/`, which is git-ignored. + +- `for s in scripts/*.sh; do bash -n "$s"; done` passes for all five scripts. +- `./scripts/build_harness.sh --run` reaches `100% tests passed out of 1` from a + clean directory, in both phases. +- `./scripts/build_linux.sh` links `SerumAlt.vst3` and the `SerumAlt` Standalone. +- `./scripts/build_windows.sh` links the cross-compiled VST3 and passes its own + checks: the module is x86-64, imports none of `libgcc_s_seh-1.dll`, + `libstdc++-6.dll` or `libwinpthread-1.dll`, and exports `GetPluginFactory`, + `InitDll` and `ExitDll`. +- The generated manifest is identical to the previously hand-written one. Compared + with `python3 -m json.tool` on both, after taking the old one from + `git show HEAD:build_windows.sh`. +- Target hygiene, read from `cmake --build --target help`: the harness + configure offers `SerumAltTest` and no plugin target; the Linux and Windows + configures offer `SerumAlt_VST3` and no `SerumAltTest`. +- First-party warning lines: 3 before the `processBlock` fix, 0 after. The four + warning lines that remain in a full run all name files under `third_party/JUCE` + (a `-Wpedantic` note about `::main` in the Standalone, two MinGW `#warning` lines + in `juce_graphics`, and a deprecated `tzname` in `juce_Time`). +- A full build leaves the source tree unchanged: `git status --porcelain -- Source + third_party/JUCE` before and after a three-target build is identical, so no build + product lands in the source tree. +- `./scripts/clean.sh --dry-run --all` prints each path with its size and removes + nothing. Run before the stale trees were deleted it listed nine paths; it now + lists the three that remain: `build/`, `SerumAlt_Windows/` and `build_linux/`. + `--staging`, `--legacy` and the outside-repository refusal were each run. +- `graft build` was re-run after the moves. The graph now mirrors the new paths, for + example `graft/Source/Engine/Engine.md`. + +## Kept, and why + +- `MacroControls::removeAssignment` is still defined and never called. No control in + the Macro tab would call it, so deleting it would remove the only path to a + feature rather than clean up dead code. +- `Source/GUI/PluginEditor.cpp` is still 1147 lines. It is the largest remaining + readability problem in the tree, and it is a GUI refactor with no headless way to + check the pixels. +- The two `.claude/helpers/*.cjs` files remain 66-line copies that differ in one + line. Each is a self-contained hook that `graft init` can regenerate; merging their + shared body into a third file would break on regeneration. Only their comments + changed. +- `opencode.json` and `.mcp.json` each declare the same graft MCP server for a + different host. Both hosts are configured this way deliberately, so removing + either would break that host. +- `third_party/JUCE/Projucer` and `third_party/JUCE/DemoRunner` are 22 MB of + upstream Linux binaries that ship with the vendored copy. They are not built here + and were not deleted. `.gitignore` now covers them, so they no longer appear as + untracked files. +- `docs/UI_UX_AUDIT.md` and `docs/archive/JANITOR_TASKS_2026-05.md` still contain the + old `./build_harness.sh` commands. They record what was run at the time, and + rewriting them would misreport that. `README.md` and `AGENTS.md` are the + maintained sources for commands. +- Nine em dashes remain inside the generated `` block in + `AGENTS.md`. They come from the installed `@nanonets/graft` template and return on + the next `graft init`; removing them needs a change to that package. + +## Limits + +- The macOS build could not be run, because it needs a Mac and the Apple SDK. + `scripts/build_macos.sh` was syntax checked and reviewed, and nothing else. +- No change in this round touched audio or GUI behaviour except deleting an unused + class, so the harness result is a regression check, not sound or visual proof. + There is still no headless way to verify a pixel. +- The staged cross-build for this round is at + `build/windows-staged-check/SerumAlt.vst3`. It is inside `build/`, so + `./scripts/clean.sh` removes it. `SerumAlt_Windows/SerumAlt.vst3` is the older + bundle and was not overwritten.