From 207cf7a304763616e6ba260eab49a975d5f14759 Mon Sep 17 00:00:00 2001 From: andrewstarks Date: Tue, 26 May 2026 13:04:21 -0500 Subject: [PATCH 1/2] docs: require topic branches + PRs for non-trivial changes Lightweight solo process: branch with an intent-shaped name, open a PR, merge when CI is green. Trivial typo / whitespace fixes can still go directly to main. Adds a "Branching & Pull Requests" section to CLAUDE.md and the corresponding step + checklist entry to Development Workflow / Commit Gates. Co-Authored-By: Claude Opus 4.7 (1M context) --- CLAUDE.md | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 24b6a27..7afebd8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -320,14 +320,30 @@ reference symmetry, dynamic-PT-requires-rtpmap, RFC 4570 §3.1 ## Development Workflow -1. Write failing tests first. -2. `busted spec/` — confirm they fail for the right reason. -3. Implement until tests pass. -4. Update GUIDE.md, README.md, CHANGELOG.md, PLAN.md as needed. -5. Commit (see gates). +1. Branch off `main` with an intent-shaped name (`feat/...`, `fix/...`, + `docs/...`, `chore/...`). +2. Write failing tests first. +3. `busted spec/` — confirm they fail for the right reason. +4. Implement until tests pass. +5. Update GUIDE.md, README.md, CHANGELOG.md, PLAN.md as needed. +6. Commit (see gates), push the branch, open a PR (see Branching & PRs). + +## Branching & Pull Requests + +All non-trivial changes go through a topic branch + PR — including +documentation and planning updates. Lightweight: solo developer, no +required reviewers, no required approvals, merge when CI is green. +Multiple commits per PR are fine when they're logically related +(e.g. a rule change plus the work that first applies it). + +The exception is trivial single-line typo / whitespace / formatting +fixes where the branch-and-PR overhead exceeds the value — those may +still go directly on `main`. Anything that touches behavior, public +API, or planning docs goes via PR. ## Commit Gates +- [ ] Work is on a topic branch, not `main` - [ ] Relevant tests added or updated in `spec/` - [ ] `busted spec/` passes with no failures - [ ] `GUIDE.md` updated for any API or behavior change From 0ef26cc142fb6cbc9fce86c00a9b17878ae17996 Mon Sep 17 00:00:00 2001 From: andrewstarks Date: Tue, 26 May 2026 13:05:58 -0500 Subject: [PATCH 2/2] docs(plan): Lua 5.1/5.2 compatibility via opt-in bitops shim Today the floor is Lua 5.3 because addresses.lua uses native `&` / `>>` / `<<` in int_to_ipv4 and ipv6_add (lines 124-198). Everything else in the library is portable. Plan: isolate the bitwise code behind a three-file shim (bitops.lua dispatcher + bitops_53.lua native backend + bitops_compat.lua wrapping bit32), so 5.3+ keeps native operators and 5.1/5.2 never parses the native-syntax file. Rockspec pulls in the bit32 rock only when _VERSION == "Lua 5.1" (5.2 has it in stdlib). CI grows a leafo/gh-actions-lua matrix across 5.1-5.5. Sliced into four ordered PRs: verification, shim refactor (still 5.3+ floor), compat slice (relax to 5.1, matrix CI, 1.3.0 release docs), then publish. Risks and verify-before-commit steps called out. Co-Authored-By: Claude Opus 4.7 (1M context) --- PLAN.md | 192 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 192 insertions(+) diff --git a/PLAN.md b/PLAN.md index 41328dd..19c9ffe 100644 --- a/PLAN.md +++ b/PLAN.md @@ -142,6 +142,198 @@ it(" [ §
]", function() Coverage target: 100% of `it` blocks in the three standards-tied files. +## Next phase — Lua 5.1 / 5.2 compatibility (opt-in shim) + +Today the rockspec declares `lua >= 5.3, < 5.6`. The only thing +binding that floor is `parse_sdp/grammar/addresses.lua` lines 124–198 +(`int_to_ipv4`, `ipv4_to_int`, `ipv6_add`), which use `&` / `>>` / +`<<`. Those operators are a parse error on Lua 5.1/5.2 — the file +cannot even be loaded. Everything else in the library is portable +(no `goto`, no ``/``, no `//` integer division, no +`string.pack`/`unpack`, no `\u{}`/`\z` escapes, no `bit32.*`, no +`_ENV`). + +The fix is to isolate the bitwise code into a tiny dispatcher module +so the 5.3+ syntax never reaches a 5.1/5.2 parser, and to install the +`bit32` rock only on 5.1 (Lua 5.2 ships `bit32` in stdlib). + +### Goal & non-goals + +- **Goal.** Ship `parse_sdp` on Lua 5.1, 5.2, 5.3, 5.4, 5.5 with no + behavioral difference visible to library callers and no compromise + to the 5.3+ code path (no global compat shim, no compat53 import, + no arithmetic rewrites of the bitwise ops). +- **Non-goals.** LuaJIT-specific work beyond what falls out for free. + Backporting Lua 5.4 stdlib features (none are used). Compat shims + for unrelated 5.3+ syntax (none exists outside addresses.lua). + +### Approach — three-file shim + +```text +parse_sdp/grammar/ + bitops.lua -- dispatcher (~6 lines, parses on any Lua ≥ 5.1) + bitops_53.lua -- native `&` / `>>` / `<<` (5.3+ only — never required on 5.1/5.2) + bitops_compat.lua -- wraps require("bit32") (5.1 needs the rock; 5.2 stdlib) + addresses.lua -- calls bitops.band / bitops.rshift / bitops.lshift; no `&` or `>>` in source +``` + +`bitops.lua` dispatches by `_VERSION`: + +```lua +if _VERSION == "Lua 5.1" or _VERSION == "Lua 5.2" then + return require("parse_sdp.grammar.bitops_compat") +end +return require("parse_sdp.grammar.bitops_53") +``` + +Each backend exports the same three functions: `band(a, b)`, +`rshift(a, n)`, `lshift(a, n)`. That's the entire surface +`addresses.lua` needs — no `bor`, `bxor`, `bnot`. `bitops_53.lua` +is shipped to all installs but only `require`'d on 5.3+, so its +`&` / `>>` syntax is invisible to a 5.1/5.2 parser. + +### Rockspec changes + +Relax: `"lua >= 5.1, < 5.6"`. Add the three new modules to +`build.modules`: + +```lua +["parse_sdp.grammar.bitops"] = "parse_sdp/grammar/bitops.lua", +["parse_sdp.grammar.bitops_53"] = "parse_sdp/grammar/bitops_53.lua", +["parse_sdp.grammar.bitops_compat"] = "parse_sdp/grammar/bitops_compat.lua", +``` + +Conditional `bit32` dep — rockspecs are Lua source and `_VERSION` +reflects the target Lua when LuaRocks is invoked under that +interpreter (`luarocks-5.1`, `luarocks-5.2`, …): + +```lua +dependencies = { + "lua >= 5.1, < 5.6", + "lpeg", + "dkjson", + "argparse", +} +if _VERSION == "Lua 5.1" then + table.insert(dependencies, "bit32") +end +``` + +Lua 5.2 has `bit32` in stdlib; Lua 5.3+ uses native operators. So +only 5.1 pulls in the extra rock. + +**Verify before commit (do not skip):** + +1. `luarocks-5.1 install ./parse_sdp-1.3.0-1.rockspec` actually + installs `bit32` as a transitive dep. +2. `luarocks-5.5 install ./parse_sdp-1.3.0-1.rockspec` does *not* + install `bit32`. +3. The conditional eval semantics in LuaRocks haven't changed under + us — if `_VERSION` does not reflect the target interpreter in some + LuaRocks configuration we care about, fall back to publishing two + rockspecs (`parse_sdp-1.3.0-1.rockspec` and + `parse_sdp-1.3.0-1-lua51.rockspec`) and document the choice in + GUIDE.md "Installation". + +### addresses.lua edits + +- Replace `(n >> 24) & 0xff` etc. in `int_to_ipv4` with `bitops.band` / + `bitops.rshift` calls. Same for `ipv6_add` (the inner `g[i] & 0xffff` + and `carry = v >> 16`). +- `ipv4_to_int` already uses arithmetic only — no change. +- Module-top `local bitops = require("parse_sdp.grammar.bitops")`. + +### Tests + +- **New file** `spec/grammar_bitops_spec.lua` — exercises the three + exported functions against the cases addresses.lua actually hits: + `band(0x12345678, 0xff) == 0x78`, `rshift(0x12345678, 24) == 0x12`, + `lshift(0xab, 8) == 0xab00`, plus an `ipv6_add` carry case + (e.g. `ipv6_add({0,0,0,0,0,0,0,0xffff}, 1)` returns + `{0,0,0,0,0,0,1,0}`). Slots topically — bitops is grammar-internal, + near `grammar_addresses_spec.lua`. (See [[test-ordering]].) +- **In-place** `spec/grammar_base_spec.lua:1349`: + `table.unpack(media_block)` → `(table.unpack or unpack)(media_block)`. + This is the only 5.2+-only call in the spec tree. +- Hermetic count: today 1197. After this work: 1197 + whatever + bitops cases land (single-digit). No grammar tests change. + +### CI + +Today `.github/workflows/test.yml` is a single `docker compose run --rm +test` job pinned to Lua 5.5 (via `Dockerfile`'s `LUA_VERSION=5.5.0` +build-arg). The Lua 5.5 path is the one to keep mirroring local dev. +For the matrix, add a separate job using `leafo/gh-actions-lua@v10` +(or `luarocks/gh-actions-lua@v10`) — faster than Docker per-version +and avoids cross-building Lua 5.1/5.2 in the existing Dockerfile. + +```yaml +matrix-test: + strategy: + matrix: + lua: ["5.1", "5.2", "5.3", "5.4", "5.5"] + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: leafo/gh-actions-lua@v10 + with: { luaVersion: ${{ matrix.lua }} } + - uses: leafo/gh-actions-luarocks@v4 + - run: luarocks install --deps-only ./parse_sdp-*.rockspec + - run: luarocks install busted + - run: busted spec/ +``` + +Keep the existing `test` (Lua 5.5 via Docker) and `conformance` jobs +unchanged — they exercise the same parser code, no value in running +the AMWA conformance fixtures across the matrix. + +### Migration & version + +- Ships as **1.3.0** (minor: new supported platforms, no API change). +- CHANGELOG.md entry under `[Unreleased]` calling out the supported + Lua range. +- GUIDE.md "Installation" gains a "Supported Lua versions" line: + "Lua 5.1 through 5.5. On 5.1 the `bit32` rock is installed + automatically; 5.2 uses its stdlib `bit32`; 5.3+ uses native bitwise + operators." +- README.md tech-stack table row: `Language | Lua 5.1+ (tested on + 5.1–5.5)`. +- CLAUDE.md tech-stack table gets the same edit. + +### Risks & open questions + +- **LuaRocks conditional eval.** Highest-priority verification step + before any code change ships. The plan stands or falls on this. + Fall-back is two published rockspecs. +- **`bit32` rock on 5.1.** Last known good: `luarocks install bit32` + works on 5.1. Check at implementation time. Source link: + `https://luarocks.org/modules/siffiejoe/bit32`. +- **LuaJIT (`_VERSION == "Lua 5.1"`).** LuaJIT ships its own `bit` + module, not `bit32`. The `bit32` rock builds on LuaJIT and provides + the bit32 API; if that turns out to be broken, the shim grows a + `pcall(require, "bit32")`-then-`require("bit").band32`-or-equivalent + branch. Flag, don't pre-solve. +- **Round-trip invariant.** No change — bitops live below the + parser/serializer interface, and `addresses.lua` IPv6 canonicalization + is byte-exact regardless of which backend computed the math. + +### Task checklist (execute in order; one PR per major slice) + +1. [ ] **Verification slice** (no code, no docs change): + smoke-test `_VERSION`-conditional dependency eval on a throwaway + rockspec under `luarocks-5.1`, `luarocks-5.2`, `luarocks-5.5`. + Record findings in a comment on the implementation PR. +2. [ ] **Shim slice**: add `bitops.lua` / `bitops_53.lua` / + `bitops_compat.lua` + `spec/grammar_bitops_spec.lua`. Switch + `addresses.lua` to call the shim. Keep rockspec at `lua >= 5.3` + for this PR — pure refactor, suite count grows by the bitops cases. +3. [ ] **Compat slice**: relax rockspec to `lua >= 5.1`, add + conditional `bit32` dep, register the three new modules in + `build.modules`, fix `table.unpack` in the one spec file, + add the matrix CI job. Bump version to 1.3.0. Update CHANGELOG / + GUIDE / README / CLAUDE per Migration & version above. +4. [ ] **Release slice**: tag `v1.3.0`, upload rockspec, publish. + ## Known Deferred Items These were explicitly evaluated and set aside. Do not re-raise them in