Skip to content

Repo hygiene: AGENTS.md, no CHANGELOG, module lists, submodule JSON - #4

Merged
kirre-bylund merged 3 commits into
devfrom
chore/repo-hygiene
Nov 5, 2026
Merged

kirre-bylund merged 3 commits into
devfrom
chore/repo-hygiene

Conversation

@kirre-bylund

Copy link
Copy Markdown
Contributor

Five changes from the owner's review of main.

1. AGENTS.md replaces .github/copilot-instructions.md

Matching the rest of the org. The old path stays as a one-line pointer because some tooling looks for it specifically, and it says explicitly not to add rules there so the two cannot drift.

2. CHANGELOG.md removed

GitHub releases are the changelog: the release notes are written at release time and are what users read, so a second file is a second thing to keep in step and a second place to be wrong. None of unity-sdk, unreal-sdk or godot-sdk keeps one either, so this removes a divergence rather than creating one.

3. LOOTLOCKER_MODULE_ONLY takes a list

This was a real bug. The option was compared against each module name with STREQUAL, so -DLOOTLOCKER_MODULE_ONLY=auth;broadcasts matched nothing and the configure succeeded with zero feature targets declared — a silent failure that reads as "the SDK is broken" rather than "the option is wrong".

  • The option is now validated against the known names, and a typo is a FATAL_ERROR.
  • core is accepted as an input even though it is not a feature module, because naming it means "core and no feature modules", which is how CI proves core builds alone.
  • CI now builds a subset and asserts that a typo fails the configure.

Verified all four cases locally: core, auth, broadcasts, auth;broadcasts.

4. LOOTLOCKER_REGENERATE_TIMEZONE

Regenerates the timezone table at build time, off by default. The generator is Python, and requiring it in every adopter's pipeline — including consoles, where the toolchain is already the hard part — would be a worse trade than a checked-in generated file. Packaging turns it on.

It is a custom target rather than a configure-time step because the generator writes into the source tree, which would make a read-only checkout fail to configure at all.

CI now runs both paths — the script directly and the CMake option — and fails on any diff.

5. nlohmann/json is a submodule

Pinned to v3.11.3 (9cca280) instead of a copied header. The copied file had no recorded version and no way to tell whether it had been modified, which is a supply-chain question with no answer in the repository.

The cost is real and I want it on the record: a fresh clone needs --recursive, and a studio that vendors dependencies itself may refuse submodules. That is a worse experience for exactly the adopter D2 was written for. The mitigation is that the pin is a tag, so the version is legible without reading a commit hash, and NOTICE records both.

Verification

  • Clean build, zero warnings, all 5 suites pass.
  • All four module combinations declare the right targets.
  • A typo fails the configure with a clear message.
  • Regeneration is a no-op on a current table, and the CMake path produces the same bytes as the script.
  • The include-boundary check still catches an injected #include "../src/...".
  • A fresh git clone --recursive builds and passes all tests, which is the only way to prove the submodule path actually works.

Spec

lootlocker/index#1879 records all five as revisions to D4, D15, D17 and D26.

Five changes from the owner's review of main.

AGENTS.md replaces .github/copilot-instructions.md as the single source of
truth, matching the rest of the org. The old path stays as a one-line pointer
because some tooling looks for it specifically, and it says explicitly not to
add rules there so the two cannot drift.

CHANGELOG.md is removed. GitHub releases are the changelog: the release notes
are written at release time and are what users read, so a second file is a
second thing to keep in step. None of unity-sdk, unreal-sdk or godot-sdk keeps
one either.

LOOTLOCKER_MODULE_ONLY now takes a list. It compared the option against each
module name with STREQUAL, so -DLOOTLOCKER_MODULE_ONLY=auth;broadcasts matched
nothing and the configure succeeded with zero feature targets declared - a
silent failure that reads as "the SDK is broken". The option is now validated
against the known names and a typo is a FATAL_ERROR. core is accepted as an
input because naming it means "core and no feature modules", which is how CI
proves core builds alone. CI builds a subset and asserts that a typo fails.

LOOTLOCKER_REGENERATE_TIMEZONE regenerates the timezone table at build time,
off by default. The generator is Python, and requiring it in every adopter's
pipeline - including consoles - would be worse than a checked-in generated
file. Packaging turns it on. It is a custom target rather than a configure-time
step because the generator writes into the source tree, which would make a
read-only checkout fail to configure.

nlohmann/json is a submodule pinned to v3.11.3 instead of a copied header. The
copied file had no recorded version and no way to tell whether it had been
modified. The cost is that a fresh clone needs --recursive, which is a worse
experience for exactly the adopter D2 was written for; the mitigation is that
the pin is a tag, so the version is legible without reading a commit hash.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings November 5, 2026 11:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Active critical, moderate, and documentation issues remain, including a missing pinned submodule that prevents compilation and CMake regeneration paths that are not actually exercised.

7 open issues
What changed in this PR

This pull request centralizes repository guidance, removes the redundant changelog, improves CMake module selection and timezone-table regeneration, and switches nlohmann/json to a documented submodule dependency.

Changes:

  • Adds AGENTS.md, removes CHANGELOG.md, and retains the legacy guidance pointer.
  • Validates module lists, supports core-only configurations, and adds opt-in timezone regeneration.
  • Updates CI, documentation, licensing metadata, and recursive submodule checkout handling.
File Summary Review comments
README.md Documents recursive cloning, module selection, and repository guidance. Nit (4 votes): Quote the semicolon-separated LOOTLOCKER_MODULE_ONLY value so the documented command works in POSIX shells.
NOTICE Records the nlohmann/json dependency and pinned revision. No review comments.
CMakeLists.txt Adds module validation, the JSON include path, and timezone regeneration support. Critical (1 vote): Add the missing submodule gitlink at the pinned commit. Moderate (3 votes): Ensure core-only configurations do not build tests that require the unavailable auth target. Moderate (2 votes): Ensure the timezone regeneration target actually runs rather than treating the checked-in output as up to date. Nits (4 votes): Correct the comment describing regeneration as configure-time.
CHANGELOG.md Removes the repository-maintained changelog. No review comments.
AGENTS.md Adds centralized repository and development conventions. Nit (4 votes): Quote the semicolon-separated module list in the inline command.
.gitmodules Registers the nlohmann/json submodule. No review comments.
.github/​workflows/​build.yml Adds recursive checkout and validates module selection and generated files. Moderate (2 votes): Remove or invalidate the generated timezone table before the CMake build so the CMake regeneration path is actually exercised.
.github/​copilot-instructions.md Points legacy tooling to AGENTS.md. No review comments.

This pull request requires changes.

Review effort: Lite

Comment thread CMakeLists.txt
# nlohmann/json is a submodule pinned to a tag (see .gitmodules). Only the
# single-header include root is on the path, so `#include <nlohmann/json.hpp>`
# resolves without exposing the rest of the checkout.
${CMAKE_CURRENT_SOURCE_DIR}/third_party/nlohmann-json/single_include

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked this one carefully and I believe it is not correct. The gitlink is present: git ls-tree origin/chore/repo-hygiene third_party/ returns 160000 commit 9cca280a4d0ccf0c08f47a99aa71d1b0e52f8d03 third_party/nlohmann-json, and .gitmodules is committed alongside it. To be sure, I cloned the pushed branch from GitHub with --recursive, and the submodule checked out at the pinned commit, single_include/nlohmann/json.hpp was present, and the clone built clean and passed all five tests. So the pin is real and config_file.cpp compiles. My guess is the review saw the patch rather than the resulting tree, since a submodule shows up in a diff as a single gitlink line with no file contents.

Comment thread .github/workflows/build.yml Resolved
Comment thread CMakeLists.txt Resolved
Comment thread CMakeLists.txt Resolved
Comment thread AGENTS.md Resolved
Comment thread CMakeLists.txt Resolved
Comment thread README.md Resolved
kirre-bylund and others added 2 commits November 5, 2026 12:43
Updated CMake configuration to regenerate timezone table at build time instead of configure time. Adjusted documentation and build scripts accordingly.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
The CMake regeneration target did not regenerate. Declaring the checked-in
.inc as an OUTPUT let the build system satisfy the rule from the file that was
already there, so a stale or hand-edited table survived a build meant to fix
it. It is now a command-bearing target, which is always out of date and always
runs. Verified by corrupting the table and watching the build repair it.

The CI step that claimed to test that path tested nothing, for the same reason:
it built an intact tree and reported "no work to do". It now corrupts the file
first, asserts the corruption is gone, and then asserts the result matches the
committed table.

-DLOOTLOCKER_MODULE_ONLY=core did not work with the default options. The
consumer test links LootLocker::auth unconditionally, so configuring core-only
failed with a target-not-found error even though core is an accepted input. The
consumer is now gated on the target existing. This matters because it is the
path the module-boundary CI uses.

Two documentation commands were not executable as shown: an unquoted semicolon
ends the cmake command on a POSIX shell, so "-DLOOTLOCKER_MODULE_ONLY=auth;
broadcasts" ran broadcasts as a separate command. Quoted in AGENTS.md and
README.md, with a note saying why.

The option comment said regeneration happens at configure time. It happens at
build time, and Python is only needed when the option is on.

Not changed: the claim that the submodule gitlink is missing. It is present at
the PR head, and a recursive clone of the pushed branch fetches it, builds and
passes all five tests. Replied with the evidence rather than adding a redundant
fix.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@kirre-bylund
kirre-bylund merged commit a76a023 into dev Nov 5, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants