Jens-G opened a new pull request, #3910: URL: https://github.com/apache/thrift/pull/3910
`compiler/cpp/CMakeLists.txt` builds `thrift-compiler` from the generator sources, and `compiler/cpp/tests/CMakeLists.txt` builds a separate static library from the same sources, gathered a second time, because the test executable links the generator code rather than running the compiler it built. Counted on a Linux build of `BUILD_COMPILER=ON BUILD_LIBRARIES=OFF BUILD_TESTING=ON`: | target | object files | | --- | --- | | `thrift-compiler` (executable) | 37 | | `thrift_compiler` (static library) | 35 | | **identical in both** | **32** | Unique to the executable: `main.cc` and the kotlin, markdown, mermaid and ocaml generators. Unique to the library: `logging.cc` and the two test support files. Everything else was compiled twice. On AppVeyor that is `thrift-compiler.exe` at 4 min 22 s and `thrift_compiler.lib` at roughly 4 min more, out of a 21 min 27 s build. The shared sources now go into a CMake `OBJECT` library that both targets consume. **The compiler tree drops from 84 object files to 52.** ### Two things worth a reviewer's attention **`t_ocaml_generator.cc` is deliberately excluded from the shared objects.** `tests/ocaml/t_ocaml_generator_tests.cc` includes that translation unit directly, so linking a compiled copy beside it duplicates every definition in it — the executable compiles it on its own. What the executable keeps is selected by searching the assembled source list rather than by re-deriving the option names. That is not cosmetic: `THRIFT_ADD_COMPILER` upper-cases them, so a condition written as `if(THRIFT_COMPILER_ocaml)` reads correctly, is never true, and silently leaves the OCaml generator in the shared set. I wrote that version first and it failed to link. **The object library links `parse`.** Both consumers carried that edge already; it is what guarantees bison has written `thrifty.hh` before any generator source compiles. The new target needs one of its own, or its objects can start compiling before the parser exists — a race, not a deterministic failure. The edge is in the generated graph: `thrift_compiler_core.dir/all: parse.dir/all`. ### Behaviour change In the combined build the test library gains the kotlin, markdown and mermaid generators. `tests/CMakeLists.txt` carries no `THRIFT_ADD_COMPILER` entry for those three while the parent build does, so the library has been missing them. They register themselves like every other generator; the effect is a slightly larger test binary and one less way for the two lists to drift apart. ### Testing Both paths that `.github/workflows/cmake.yml` exercises were built and run on Linux, before and after: | | baseline | after | | --- | --- | --- | | objects, combined build | 84 | 52 | | `t_cpp_generator.cc.o` compiled | 2× | 1× | | compiler tests, combined | 330 assertions in 24 test cases | identical | | `ctest` of that configuration | 18/18 passed | 18/18 passed | | objects, standalone `compiler/cpp/tests` | 24 | 24 | | compiler tests, standalone | 330 assertions in 24 test cases | identical | Plus three clean configure-and-build cycles at `-j16` to shake out the ordering. MSVC was not exercised locally — the AppVeyor run on this pull request covers it. ### Related THRIFT-6324 cuts the AppVeyor matrix and adds `--parallel`; THRIFT-6325 repairs the `/MP` condition. This one removes work rather than spreading it over more cores, so it composes with both. --- - [x] Did you create an [Apache Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket? — THRIFT-6326 - [x] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"? - [x] Did you squash your changes to a single commit? - [x] Did you do your best to avoid breaking changes? Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> -- 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]
