chore(build): include benches/ in insert_apache_header.py target roots - #1728
Conversation
Add "benches" to TARGET_ROOTS so the Apache-header insertion script covers the benches/ directory. Insert the standard header into benches/audio_fft.rs, the only Rust source file under a top-level source root that was missing it. Extend the Python test by one assertion to pin the new root. Closes lablup#1707
|
|
inureyes
added
status:review
Issue lablup#1707 found `benches/audio_fft.rs` shipping unlicensed and attributed it to `benches` missing from `insert_apache_header.py`'s `TARGET_ROOTS`, calling it "the only Rust file under a top-level source root missing it". Running the script proves that premise wrong: 95 further files under `src`, `examples` and `tests` were unlicensed too, and those three roots were already listed. The roots list was never the binding constraint; nothing ever ran the script. Every file here is a pure 14-line prepend, +14/-0 with no source line touched. The two files under `src/lib/mlx-cpp/patches/` that receive a header are the ones marked "Added by mlxcel. Not an upstream MLX file." The rest of that directory keeps Apple's copyright and is skipped by `has_existing_header`, which must stay true: this must never stamp a Lablup header onto upstream-derived code. `build.rs` is included because it is the crate's own build script (`build = "build.rs"` in Cargo.toml) and ships in the published package. `spike/` stays out: those are scratch crates outside the workspace `members` list. Refs lablup#1707, lablup#1728
PR lablup#1728 adds `benches` to `TARGET_ROOTS`, which is correct but cannot hold on its own: nothing in CI, the Makefile, or any script ever invoked `insert_apache_header.py`, and `tests/test_insert_apache_header.py` was run by nothing at all. It sits outside `python/`, and python.yml's pytest step is scoped to `python/tests` behind a `python/**` path filter, so the assertion this PR adds there never executed. That is why 95 files under roots the script already covered shipped unlicensed. `--check` reports the files the default insert mode would write to and exits 1, writing nothing. It reuses `has_existing_header`, the same predicate `insert_header_in_file` branches on, so the gate can never demand an edit that running the script does not produce; a test asserts the two agree on the same tree. `TARGET_FILES` reaches top-level shipped files the root walk cannot: `Path("build.rs").parts[0]` is the file itself and matches no root, so `build.rs` needed an explicit entry rather than a new root. The new `license-headers` job runs both the check and the unit suite, following the `kernel-dtype-keys` pattern: ubuntu-latest, no toolchain, outside the `changes` filter. Verified by reverting: stripping the header off one backfilled file fails the check, and dropping `build.rs` from `TARGET_FILES` fails three tests. `from __future__ import annotations` keeps the PEP 604 unions in the new signatures parseable on Python 3.9, which python.yml still tests. Refs lablup#1707, lablup#1728
Hardening pass on this PRReviewed this PR against #1707. The The issue's premise was wrong, and understated the gap#1707 called Nothing enforced any of this
Validation11/11 unit tests pass; Security and performance: no runtime code paths touched. The one risk considered was stamping Lablup copyright onto upstream-derived files, which Refs #1707 |
## Summary - Normalize project-owned source headers and the header generator around Lablup Inc. - Replace individual NOTICE attribution with collective `Lablup Inc. and contributors.` wording. - Retain authorship provenance in the core crate's package metadata. - Follow up on PR #1728's repository-wide header coverage. ## Test plan - `python3 tests/test_insert_apache_header.py -v` (11 tests passed) - `python3 scripts/insert_apache_header.py --check` - `cargo metadata --no-deps --format-version 1` - `git diff --check`
|
Thanks for this one, @saitejagirada. Your diagnosis was right and your commit went in unchanged. Apologies for how much the PR grew under your branch. The hardening comment above has the detail, but the short version is that your fix was correct and the problem was simply bigger than #1707 described: the roots list was never what was holding it back, since nothing had ever run the script. That was not visible from the issue, and finding it took someone actually pulling on the thread. The |
|
good to see that my contribution helped you , and thanks for the merge |
2 participants
Summary
Add
"benches"toTARGET_ROOTSinscripts/insert_apache_header.pyso the Apache-header insertion script covers thebenches/directory. Insert the standard header intobenches/audio_fft.rs, the only Rust source file under a top-level source root that was missing it. Extend the Python test by one assertion to pin the new root.Closes #1707
Changes
scripts/insert_apache_header.py"benches"toTARGET_ROOTSbenches/audio_fft.rstests/test_insert_apache_header.pyshould_process_path("benches/audio_fft.rs")assertionVerification
python3 -m pytest tests/test_insert_apache_header.py -v # 5 passedRunning the script itself confirms
benches/audio_fft.rsis now recognized and skipped (header already present):Acceptance Criteria (from #1707)
benches/audio_fft.rscarries the standard headerbenches/