Skip to content

Document cleanup roadmap: split PR #90 and DUnitX-first #96

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
omonien wants to merge 1 commit into
base: development
Choose a base branch
from
Open

Document cleanup roadmap: split PR #90 and DUnitX-first #96

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
151 changes: 151 additions & 0 deletions Docs/Cleanup-Roadmap.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
# DEC Cleanup Roadmap

Working branch: **`Cleanup_OM`** (PRs against **`development`**).

This document records agreed technical decisions and the planned order of work.
It is intentionally a planning artefact, not a design for individual pull requests.

---

## 1. Context: PR #90 (ChaCha20-Poly1305)

Open contribution: [PR #90](https://github.com/MHumm/DelphiEncryptionCompendium/pull/90)
(`mikerabat` → base branch `development_chacha20poly1305`).

The PR currently mixes **two independent concerns**:

| Concern | What it contains | Risk if merged as-is |
|---|---|---|
| **A. Architecture / AEAD base rewrite** | Slimmer `TAuthenticatedCipherModesBase` lifecycle (`InitAuth`, `UpdateWithEncDecBuf`, `FinalizeMAC` / `FinalizeAEAD`, multi-call encode/decode); large GCM rewrite for stream/chunk support (#87); shared wiring for authenticated modes | Touches core cipher-mode contracts; can break GCM/CCM and existing tests; needs careful review |
| **B. ChaCha features** | `TCipher_ChaCha20`, `TCipher_XChaCha20`, Poly1305 mode (`cmPoly1305`, `DECCipherModesPoly1305`), CPU feature detection, SSE/AVX paths, AES-NI, Wycheproof/RFC tests | Large feature surface, but can be evaluated as an algorithm addition **once** the base API is stable |

Related extras in the same PR (also not “pure ChaCha”): AES-NI assembler, `DECCPUSupport`, `DECOptions.inc` ASM define changes, 64-bit util/ASM fixes.

### Decision (binding)

**The architecture rework must be completely separated from the ChaCha feature work.**

Consequences:

1. **Do not** merge PR #90 as a single unit.
2. Split (or re-land) into at least:
- **PR / package 1 — AEAD architecture:** base class + GCM multi-call/stream behaviour (+ only the minimum CCM adaptations required for the new interface). No ChaCha/XChaCha/Poly1305 algorithm code.
- **PR / package 2 — ChaCha20 / XChaCha20 / Poly1305 AEAD:** algorithms, mode wiring, SIMD options, dedicated tests — built **on top of** the settled architecture from package 1.
3. Optional further splits (if review load requires it): AES-NI, CPU support, options/ASM fixes as separate changes.
4. Until that split exists, treat PR #90 as a **reference implementation / donor branch**, not as the integration path.

Rationale: architecture changes redefine how authenticated modes work for the whole library. Reviewing them together with a new cipher family makes regressions harder to attribute and forces an all-or-nothing review of ~8k LOC.

---

## 2. Prerequisite: complete DUnit → DUnitX migration

**Before** tackling the AEAD architecture split (and before large cipher-mode refactors), the unit-test suite must be **fully migrated to DUnitX**.

### 2.1 Current state (as of `development` / start of `Cleanup_OM`)

| Item | Status |
|---|---|
| Classic DUnit project | `Unit Tests/DECDUnitTestSuite.dpr` (+ `.dproj`) — uses `TestFramework` |
| DUnitX project | `Unit Tests/DECDUnitXTestSuite.dpr` (+ `.dproj`) |
| Shared switch | `Unit Tests/Tests/TestDefines.inc` — `{.$DEFINE DUnitX}` (**off by default**) |
| Dual-stack tests | Most units under `Unit Tests/Tests/*.pas` use `{$IFDEF DUnitX}` for uses / `[TestFixture]` / registration, and still fall back to classic `TestFramework` / DUnit runners |
| Compatibility layer | Several units import `DUnitX.DUnitCompatibility` when DUnitX is on — transitional, not end state |
| Coverage tooling | `Unit Tests/CodeCoverage/` historically oriented around the DUnit-era suite |

The suite is therefore **dual-stack**: one codebase, two frameworks, compile-time selected. That is useful historically, but it:

- doubles project/maintenance surface,
- encourages DUnit-shaped APIs (`TTestCase`, compatibility shims) instead of idiomatic DUnitX,
- complicates CI (“which runner is authoritative?”),
- will fight larger AEAD refactors that need clear, single-runner green/red signals.

### 2.2 Target state

| Item | Target |
|---|---|
| Framework | **DUnitX only** |
| Classic DUnit project | Removed (or left unmaintained only if a temporary deprecation notice is required — prefer removal) |
| `TestDefines.inc` dual mode | Removed or reduced to non-framework defines; no DUnit fallback |
| Test units | Idiomatic DUnitX: `[TestFixture]`, `[Test]`, `Assert.*` / DUnitX asserts, `TDUnitX.RegisterTestFixture` (or attribute discovery as chosen consistently) |
| `DUnitX.DUnitCompatibility` | Eliminated once call sites no longer need it |
| Default / CI runner | `DECDUnitXTestSuite` (console; optional GUI/TestInsight kept only if still useful) |
| Documentation | README / CONTRIBUTING mention DUnitX only |

Success criteria:

1. All existing tests compile and run under DUnitX without `TestFramework` / classic DUnit packages.
2. No remaining `{$IFDEF DUnitX}` dual paths in test units (except temporary WIP on a sub-branch).
3. One documented way to run the full suite from IDE and command line.
4. Code coverage (if kept) wired to the DUnitX suite.

### 2.3 Suggested migration steps (implementation order)

These steps are guidance for a later implementation PR (or series of PRs on `Cleanup_OM`):

1. **Baseline:** enable `DUnitX` in `TestDefines.inc`, run full `DECDUnitXTestSuite`, record pass/fail inventory.
2. **Project cleanup:** make DUnitX the only suite in the group project; stop shipping dual search paths for DUnit.
3. ** mechanize unit-by-unit:** drop `TestFramework` uses and `{$ELSE}` DUnit branches; keep behaviour identical.
4. **Remove compatibility layer:** replace remaining DUnit-compatible assert/test-case patterns with native DUnitX.
5. **Delete** `DECDUnitTestSuite` (and obsolete ModelSupport / DUnit-only assets if unused).
6. **Update docs** (readme “Has it been tested?”, CONTRIBUTING) and any coverage scripts.
7. **Optional:** align layout/naming later (`tests/` etc.) — out of scope for the framework migration itself.

Do **not** mix this migration with AEAD architecture or ChaCha feature work in the same PR.

---

## 3. Agreed overall work order

```
Cleanup_OM (and follow-up PRs → development)
│
├─ 0. Repo hygiene (started)
│ e.g. DelphiStandards .gitignore
│
├─ 1. DUnit → DUnitX migration (complete) ← next major technical step
│ Single test runner, no dual-stack
│
├─ 2. AEAD architecture split (from PR #90 concern A)
│ Base + GCM stream/multi-call only; review as core API change
│
├─ 3. ChaCha / XChaCha / Poly1305 (from PR #90 concern B)
│ On top of settled AEAD base; algorithms + tests
│
└─ 4. Optional: AES-NI / CPU support / options fixes
As separate reviewable packages if not absorbed earlier
```

### Why this order

1. **Tests first:** architecture and cipher merges need a single, trustworthy automated suite. Dual DUnit/DUnitX weakens that signal.
2. **Architecture before features:** ChaCha-as-AEAD depends on (or must not re-introduce) the multi-call authenticated-mode model; shipping ChaCha on the old GCM-only shape and then rewriting the base again is wasted motion.
3. **Separation of review:** maintainers can accept/reject AEAD API changes without blocking or rubber-stamping a large SIMD cipher contribution.

---

## 4. Out of scope for this document

- Detailed AEAD class design (belongs in a design note when step 2 starts).
- Accept/reject decision on individual PR #90 commits.
- Delphi style / layout alignment with external house standards beyond what is already done (e.g. `.gitignore`).
- FPC/Lazarus policy (may interact with ASM defines from the donor PR; track separately).

---

## 5. References

| Item | Location |
|---|---|
| Working branch | `Cleanup_OM` |
| Integration target | `development` |
| Donor PR | https://github.com/MHumm/DelphiEncryptionCompendium/pull/90 |
| Donor branch (origin target) | `development_chacha20poly1305` |
| Donor head (implementation) | `mikerabat/DelphiEncryptionCompendium` branch `development` |
| DUnit project | `Unit Tests/DECDUnitTestSuite.*` |
| DUnitX project | `Unit Tests/DECDUnitXTestSuite.*` |
| Dual-stack switch | `Unit Tests/Tests/TestDefines.inc` |

---

*Document created as part of cleanup planning on `Cleanup_OM`. Update this file when a step completes or a decision changes.*