morningman commented on PR #66712: URL: https://github.com/apache/doris/pull/66712#issuecomment-5279076862
Pushed fixes for all four findings from the review above (each answered inline): | # | finding | fix | |---|---|---| | 1 | `run-be-ut.sh` drops the switch | 0dcc67c8e37 — forwards `-DENABLE_UNITY_BUILD` and logs the effective value, mirroring `ENABLE_PCH` | | 2 | `if (ENABLE_UNITY_BUILD)` is a one-way gate | e1299ba40f4 — option normalized to strict ON/OFF, all three pilot targets set `UNITY_BUILD` unconditionally from it | | 3 | `cut_impact.py` silently discards unity blocks | 90d33da6061 — refuses a unity build dir with the rebuild instruction | | 4 | `report.py` files unity time under the build dir | 90d33da6061 — unity TUs attributed to their target's directory | Plus 6bb52390d17, which was not in the review: `rebuild_radius.py` reads the same `ninja -t deps` database at per-object granularity, so under unity a header included by all 51 InformationSchema scanners reports 1 dependent instead of 51. Same class of silent meaning-change as #3, so it now says when its counts are batch-granular. Verification: - **CMake gate**, real configure of this tree: `-DENABLE_UNITY_BUILD=OFF -DCMAKE_UNITY_BUILD=ON` gave 28 unity TUs across the pilot targets before, **0** after; `-DENABLE_UNITY_BUILD=ON` still gives exactly the 6 advertised batches (1 + 1 + 4). An empty `-DENABLE_UNITY_BUILD=` now normalizes to OFF instead of failing the configure with an argument-count error. - **Tooling**, `build-support/tests/test-compile-bench-unity.sh` (new, 6 checks, passing): a canned `ninja -t deps` database in both shapes — the standalone block is still loaded, the unity block is refused with the `ENABLE_UNITY_BUILD=OFF` instruction — plus unity / ordinary / non-unity-build-tree path grouping. Two notes where the fix deliberately differs from the suggestion: - For #3 the review offered "expand unity blocks **or** fail explicitly". Expanding is not sound: a unity block carries the union of its members' closures, so attributing it back per member would make every source look like it reaches every header a sibling pulls in, inflating the affected counts and the seeding advice. Silent under-coverage would become silent over-estimation, so the tool refuses. - For #4, attribution is only as fine as the target's own directory: the Service batch merges just `service/http` sources but rolls up under `be/src/service`, and the Storage batches under `be/src/storage`. A unity TU is one timing for all its members by construction — finer attribution is only recoverable from an `ENABLE_UNITY_BUILD=OFF` run. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
