|
| 1 | +# Suggested PR split: current uncommitted work (`more-nanobind-migration`) |
| 2 | + |
| 3 | +Maps the branch’s uncommitted work into **a few** reviewable merges. Adjust if |
| 4 | +your maintainer wants one nanobind mega-PR instead. |
| 5 | + |
| 6 | +--- |
| 7 | + |
| 8 | +## 1. Inventory (what the branch touches) |
| 9 | + |
| 10 | +### Modified (tracked) |
| 11 | + |
| 12 | +| Area | Paths | |
| 13 | +|------|--------| |
| 14 | +| Pybind buffer `TypeDesc` | `src/python/py_oiio.cpp` | |
| 15 | +| Nanobind core + module glue | `src/python-nanobind/py_oiio.cpp`, `src/python-nanobind/py_oiio.h` | |
| 16 | +| Nanobind build | `src/python-nanobind/CMakeLists.txt` | |
| 17 | +| Nanobind migration note | `src/python-nanobind/MIGRATION_STATUS.md` | |
| 18 | +| CTest / Python paths | `src/cmake/testing.cmake` | |
| 19 | +| Libutil tests | `src/libutil/CMakeLists.txt` | |
| 20 | +| Shared tests + refs | `testsuite/python-imagebuf/…`, `testsuite/python-imagespec/…`, `testsuite/python-paramlist/…` | |
| 21 | + |
| 22 | +### Untracked (new) |
| 23 | + |
| 24 | +| Area | Paths | |
| 25 | +|------|--------| |
| 26 | +| C++ unit test (PEP 3118 `=` / format mirror) | `src/libutil/python_array_format_test.cpp` | |
| 27 | +| Coverage helper script | `src/build-scripts/report_nanobind_python_coverage.bash` | |
| 28 | +| New nanobind translation units | `src/python-nanobind/py_{argparse,deepdata,imagebuf,imagebufalgo,imageinput,imageoutput}.cpp` | |
| 29 | +| New testsuites | `testsuite/python-oiio/`, `testsuite/python-argparse-nanobind/` | |
| 30 | +| Fixture | `testsuite/python-imageoutput/multipart.exr` | |
| 31 | + |
| 32 | +--- |
| 33 | + |
| 34 | +## 2. Build constraint (nanobind) |
| 35 | + |
| 36 | +`PyOpenImageIONanobind` is **one** CMake target: `nanobind_srcs` lists every |
| 37 | +`py_*.cpp`, and `py_oiio.cpp` calls every `declare_*`. So **all new nanobind |
| 38 | +`.cpp` files and glue normally land in the same PR** unless you restack the |
| 39 | +branch so each merge trims `nanobind_srcs` / init (high effort). |
| 40 | + |
| 41 | +--- |
| 42 | + |
| 43 | +## 3. Clamped PR sequence (recommended: **3 PRs**) |
| 44 | + |
| 45 | +### PR 1 — Pybind only: buffer format → `TypeDesc` |
| 46 | + |
| 47 | +**Files:** `src/python/py_oiio.cpp` only. |
| 48 | + |
| 49 | +**Contents:** `typedesc_from_python_array_code` parity (e.g. leading `=`, `q` / |
| 50 | +`Q`, formatting). |
| 51 | + |
| 52 | +**Why first:** Tiny, isolated, easy revert; aligns pybind with what nanobind |
| 53 | +already does before you land the rest of the migration. (Impact on typical |
| 54 | +NumPy paths is small; this is mostly hygiene / edge-case correctness.) |
| 55 | + |
| 56 | +**Tests:** Optional local `ctest` for existing pybind Python tests; no new |
| 57 | +tests required. |
| 58 | + |
| 59 | +--- |
| 60 | + |
| 61 | +### PR 2 — Nanobind migration (one “feature” merge) |
| 62 | + |
| 63 | +**Single merge** for everything that makes the nanobind extension complete and |
| 64 | +CI-meaningful: |
| 65 | + |
| 66 | +- All **new / changed** under `src/python-nanobind/` (including every new |
| 67 | + `py_*.cpp`, `py_oiio.cpp` / `py_oiio.h`, `CMakeLists.txt`, `MIGRATION_STATUS.md`) |
| 68 | +- **`src/cmake/testing.cmake`** — nanobind test lists, `python-oiio` / |
| 69 | + `python-argparse-nanobind`, image I/O / IBA groups, etc., as on your branch |
| 70 | +- **Tests + refs** that validate nanobind against pybind for this work: |
| 71 | + - `testsuite/python-imagebuf/…` (e.g. HALF `get_pixels` parity) |
| 72 | + - `testsuite/python-imagespec/…`, `testsuite/python-paramlist/…` |
| 73 | + - `testsuite/python-argparse-nanobind/` (full tree) |
| 74 | + - `testsuite/python-imageoutput/multipart.exr` if required |
| 75 | + |
| 76 | +**Why one PR:** Matches the **single shared library** layout; avoids half-wired |
| 77 | +`nanobind_srcs` between merges. Use **commits inside the PR** (per file or per |
| 78 | +subsystem) if reviewers want smaller logical chunks without separate merges. |
| 79 | + |
| 80 | +--- |
| 81 | + |
| 82 | +### PR 3 — Optional “extras” (drop or fold into PR 2) |
| 83 | + |
| 84 | +Bundle non-core or skippable items so they do not block the migration: |
| 85 | + |
| 86 | +- `src/libutil/python_array_format_test.cpp` + `src/libutil/CMakeLists.txt` |
| 87 | + (`unit_python_array_format`) — C++ mirror for `=` / PEP 3118 edge cases |
| 88 | +- `testsuite/python-oiio/` + `src/cmake/testing.cmake` entries for it **if** |
| 89 | + you did not already merge those lines in **PR 2** |
| 90 | +- `src/build-scripts/report_nanobind_python_coverage.bash` |
| 91 | + |
| 92 | +If you prefer **fewer files to track**, fold all of **PR 3** into **PR 2** and |
| 93 | +delete this third bucket. |
| 94 | + |
| 95 | +--- |
| 96 | + |
| 97 | +## 4. Aggressive option: **2 PRs** |
| 98 | + |
| 99 | +1. **PR 1** — same as §3 PR 1 (`src/python/py_oiio.cpp` only). |
| 100 | +2. **PR 2** — **everything else** from the inventory (full nanobind migration + |
| 101 | + all tests + `testing.cmake` + libutil test + `python-oiio` + coverage script). |
| 102 | + |
| 103 | +Same review cost as “one big branch,” but **bisect** can still isolate pybind |
| 104 | +buffer fixes. |
| 105 | + |
| 106 | +--- |
| 107 | + |
| 108 | +## 5. What not to mix in without an explicit goal |
| 109 | + |
| 110 | +| Item | Suggestion | |
| 111 | +|------|------------| |
| 112 | +| Public `OpenImageIO.typedesc_from_python_array_code(...)` | Avoid unless you want a **stable** Python API + stubs. | |
| 113 | +| Shared header in `libutil` + bindings | Separate refactor PR. | |
| 114 | +| Unrelated `testing.cmake` / plugin toggles | Keep migration PR focused. | |
| 115 | + |
| 116 | +--- |
| 117 | + |
| 118 | +## 6. Checklist |
| 119 | + |
| 120 | +- [ ] `./compile.sh` (or CI-equivalent) for backends you enable. |
| 121 | +- [ ] `ctest` for every testsuite you register (`python-*`, `*.nanobind`). |
| 122 | +- [ ] Regenerate `ref/out.txt` only for tests you touch. |
| 123 | + |
| 124 | +This file is **developer guidance**; trim or delete before upstreaming if |
| 125 | +`docs/dev/` should stay minimal. |
0 commit comments