Jens-G opened a new pull request, #3843: URL: https://github.com/apache/thrift/pull/3843
Implements [THRIFT-6235](https://issues.apache.org/jira/browse/THRIFT-6235). ## The defect `thrift_compiler_tests` does not link when the Go generator is disabled: ``` undefined reference to `t_go_generator::get_publicized_name_and_def_value(t_field*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, t_const_value**) const' undefined reference to `t_go_generator::is_pointer_field(t_field*, bool)' undefined reference to `t_go_generator::is_container_keyed_map(t_type*)' ``` Two configurations disable it: - Configuring `compiler/cpp/tests` on its own. That is how `compiler/cpp/tests/README.md` says to build and run the compiler tests, and the Go generator defaults to off there. - A top-level CMake build with `-DTHRIFT_COMPILER_GO=OFF`. The `thrift` executable still links, but the test binary does not. 0.23.0 and 0.24.0 fail the same way, with the first two symbols. CI did not notice. The *Build with CMake* workflow and the MSVC build both configure from the top level and leave Go enabled. ## The change `compiler/cpp/CMakeLists.txt` compiles `go_validator_generator.cc` only when `THRIFT_COMPILER_GO` is on. The `THRIFT_ADD_VALIDATOR_COMPILER` macro in `compiler/cpp/tests/CMakeLists.txt` used a separate `THRIFT_VALIDATOR_COMPILER_GO` option instead, and that option defaults to on. So the validator could be compiled without the generator it calls into. The test binary links its library as a whole archive, which means the validator's unresolved references broke the link. The tests' macro now keys on `THRIFT_COMPILER_<LANG>` too, and its comment says why. In both files, the validator is now compiled only when the Go generator is: | `THRIFT_COMPILER_GO` | validator in `thrift` | validator in `thrift_compiler_tests`, before | after | |---|---|---|---| | `ON` | yes | yes | yes | | `OFF` | no | yes, and the link fails | no | An existing build directory may still have `THRIFT_VALIDATOR_COMPILER_GO` in its cache. Nothing reads it any more. ## Test The *Build with CMake* workflow gets a new step. It runs the README's commands in `compiler/cpp/tests`, then `ctest --no-tests=error`, so this configuration stays covered. `--no-tests=error` makes the step fail if no test is registered, so it cannot pass with nothing run. Results in `thrift:jammy`, with the new step's commands taken from the workflow file: | | master | this change | |---|---|---| | new workflow step: `compiler/cpp/tests` on its own | link fails with the three symbols above | 1 of 1 test passes (326 assertions in 22 test cases) | | top-level build with `-DTHRIFT_COMPILER_GO=OFF` | link fails | 18 of 18 `ctest` tests pass | | top-level build with defaults (the existing workflow steps) | 18 of 18 pass (also in CI at `b0b31cfa0`) | 18 of 18 pass | I also checked the build files after configuring. With `THRIFT_COMPILER_GO=ON`, both targets compile the validator; with `OFF`, neither does. `zizmor` reports no findings for the changed workflow, the same as on master. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
