Conversation
phpclub
commented
Apr 18, 2026
- New migration.rst with Docker, source build, and community repo instructions
- Compatible with FreeUnit v1.35.3+
- Follows installation.rst style guide
- New migration.rst with Docker, source build, and community repo instructions - Compatible with FreeUnit v1.35.3+ - Follows installation.rst style guide
migration.rst: split guide into user vs. maintainer audiences; rewrite Community Repository Maintainers section with per-distro packaging recipes (Alpine, ALT, Arch, FreeBSD, Gentoo, NetBSD, Nix, OpenBSD, Remi RPM); fix Docker section — correct tag format (<VERSION>-<VARIANT>), add full variant table from docker.yml (27 variants), add language EOL policy note, fix rollback example tag, fix verify command. installation.rst: add "coming soon" warnings for official packages, link to migration guide, update Docker section to GHCR registry with correct tag format and variant list, mark repo install script and Homebrew tap as archived/incompatible, fix all residual nginx/unit source URLs.
andypost
left a comment
There was a problem hiding this comment.
Review: docs: add migration guide
Overall: the structure is excellent — clear audience separation, comprehensive per-distro recipes, and good rollback/FAQ sections. However there are several factual inaccuracies that would mislead users.
Critical — Factual Errors
1. unitd --version does NOT output freeunit/X.Y.Z
NXT_NAME is still "Unit" in src/nxt_main.h:14 — the rebrand commit (297d5906) changed Dockerfile labels, README, etc. but never touched the C source. The actual output is Unit/1.35.2. This claim appears 6+ times throughout the PR (Docker verify, source build verify, FAQ, Remi verify).
Either the docs need to match reality (unit/X.Y.Z), or NXT_NAME needs changing in the C source first.
2. ./configure --modules=php:8.4,python:3.13,... does not exist
The source build section shows:
$ ./configure --openssl --otel \
--modules=php:8.4,python:3.13,nodejs:22,go1.25,ruby3.4,perl5.40
--modules=DIR is a deprecated synonym for --modulesdir (sets the module install directory). Language modules are configured via separate subcommands:
$ ./configure --openssl
$ ./configure python --config=python3-config --module=python3
$ ./configure php --module=php
$ make
$ make python3 php
3. Broken internal link: migration-remi-hybrid
Referenced in the introduction and FAQ but the anchor is never defined. Will render as a broken cross-reference in Sphinx.
4. unitd --test-config does not exist
Used in the PHP troubleshooting section but this flag doesn't exist in Unit's CLI parser (src/nxt_main.c). Config validation happens through the control API, not a CLI flag.
5. make install does NOT install unitctl
The build section claims make install installs unitctl to /usr/local/bin/. The Makefile's install target only runs unitd-install + manpage-install. unitctl is built separately with cargo from tools/unitctl/.
6. pkg/systemd/unit.service does not exist
The systemd setup section says cp pkg/systemd/unit.service /etc/systemd/system/ but there is no pkg/systemd/ directory in the repo.
Significant — Incorrect Docker Variant Claims
The variant table lists go1.26 and node24, but only these Dockerfiles actually exist:
go1.24,go1.25(nogo1.26)node20,node22(nonode24)ruby3.4✓,perl5.40✓,php8.5✓ — these are correct
Also: the custom Docker image section uses FROM ...:minimal but earlier the doc states there is "no bare latest tag". Should be latest-minimal or a pinned version.
Minor Issues
docker-compose(v1 syntax) — modern Docker usesdocker compose(v2 plugin, no hyphen). Consider using the v2 form.- Runtime Compatibility table — Lists
/run/unit/control.sockbut the default from./configureis$runstatedir/control.unit.sock(typically/run/unit/control.unit.sock). - Duplicate Docker variant table — Identical table in both
installation.rstandmigration.rst. Consider using a shared include or cross-reference to avoid drift. - Missing newline at EOF —
migration.rstdoesn't end with a newline.
Pedantic reviewReproduced locally with Blockers
Significant
Minor / nits
Not an issue (investigated)
Reproduce locally$ uv venv
$ uv pip install -r requirements.txt
$ .venv/bin/sphinx-build -E -b nxt_html source buildVerdictDon't merge as-is. Blockers 1–7 either break the build (ERROR), drop content silently (meta/emphasis), or produce dangling refs. Blocker 7 in particular will mislead users who copy-paste. Recommended path: fix the |
|
I will try to fix some later today or tomorrow |
Resolve the 1 ERROR and 47 warnings the reviewer flagged in migration.rst, the six factual corrections, and the two requested additive sections. After this commit migration.rst contributes zero warnings; the 9 remaining build warnings are pre-existing envvar targets on master (out of scope). Sphinx fixes: - Fix .. meta:: directive indent (1 space -> 3): clears the "Invalid meta directive" ERROR. - Escape the leading asterisk in \*BSD: clears the "Inline emphasis start-string without end-string" warning. - Drop :ref: wrappers on bare nouns (socket, file, user and group, TLS, regex). These read as descriptive column labels, not cross-references, and no anchors exist. Clears 29 "undefined label" warnings. - Extend short title overlines to match title length (7 titles, 11 overline warnings). - Add migration to contents.rst toctree: clears the "document isn't included in any toctree" warning. - Fix :doc:\`configuration\` -> :doc:\`configuration/index\`; the chapter lives in configuration/index.rst. Factual corrections: - Docker variant matrix: drop go1.26 and node24 (not built); keep the language-versions bullet list in sync. - ./configure --modules=<csv> is not a valid flag. Replace with the real per-language subcommand sequence (./configure php --module=..., ./configure python ..., etc.), matching howto/source.rst. - make install does not install unitctl; the Makefile installs unitd, modules, and manpages. Drop the false line and point at the separate freeunitorg/unitctl repository. - pkg/systemd/unit.service does not ship in the tree. Replace the "copy the provided unit file" step with an inline example unit file. - unitd --test-config does not exist. Replace the troubleshooting step with listing module paths and tailing the log after a reload, since config errors surface only when the daemon applies a config through the control API. - unitd --version output: NXT_NAME still reads "unit" in the fork, so the prefix is "unit version: X.Y.Z", not "freeunit/X.Y.Z". Correct each example and replace the "freeunit/ prefix confirms the fork" note with provenance checks (git remote / rpm -q / docker inspect). - docker-compose v1 (hyphenated binary) is end of life. Use the docker compose v2 subcommand form in deploy/rollback snippets. New sections requested by the reviewer: - migration-remi-hybrid: RHEL/Fedora walk-through for swapping only the unit core for freeunit while keeping Remi's phpXX-unit-php modules in place. Defines the anchor the intro and FAQ already referenced (was 2 of the undefined-label warnings). - migration-plan: pre-migration checklist (inventory current core, back up the live /config/, pick one install path, schedule the restart window, keep rollback trigger in place). - migration-validation: post-install checklist (daemon active, control socket answers, config diffs clean against backup, every configured module loaded, end-to-end request returns 2xx, reload log is clean); any failure points at migration-roll-back.
Resolve the 1 ERROR and 47 warnings the upstream review on freeunitorg#9 flagged. Strictly format-only / cross-reference fixes; no factual rewrites in this commit. After this commit migration.rst contributes zero warnings (the 9 remaining build warnings are pre-existing envvar targets on master, out of scope). - Fix .. meta:: directive indent (1 space -> 3): clears the "Invalid meta directive" ERROR. - Escape leading asterisk in \*BSD: clears the inline-emphasis warning. - Drop :ref: wrappers on bare nouns (socket, file, user and group, TLS, regex). They read as descriptive labels, not cross-references, and no anchors exist. Clears 29 "undefined label" warnings. - Extend short title overlines to match title length (7 titles, 11 "Title overline too short" warnings). - Add migration to contents.rst toctree: clears the "document isn't included in any toctree" warning. - Fix :doc:\`configuration\` -> :doc:\`configuration/index\`; the chapter lives at configuration/index.rst. - Inline the "hybrid approach" prose at both ref sites instead of cross-referencing a non-existent migration-remi-hybrid section. Clears the last 2 undefined-label warnings; the section itself can land in a follow-up.