Jens-G commented on PR #3826: URL: https://github.com/apache/thrift/pull/3826#issuecomment-5634153168
### Code review Found 2 issues: 1. The Autotools recipe does not work with the dependencies the page lists. `brew install cmake boost libevent openssl bison flex` installs none of autoconf, automake, libtool or pkg-config, and macOS does not ship them. The recipe starts with `./bootstrap.sh`, which exits with "Couldn't find libtoolize!" when libtool is missing. It also requires automake 1.13 or later and runs aclocal/autoconf/automake, and `configure.ac` needs the pkg-config macros. The macOS CI job installs `automake bison flex boost libevent openssl libtool pkg-config` before running the same steps. (bug due to doc/install/os_x.md: `brew install cmake boost libevent openssl bison flex`, followed by `./bootstrap.sh`) https://github.com/apache/thrift/blob/362d95457d960060130f142670a9b3cf794237dd/doc/install/os_x.md#L23-L25 https://github.com/apache/thrift/blob/362d95457d960060130f142670a9b3cf794237dd/.github/workflows/build.yml#L131-L133 2. The commit has no AI attribution trailer, although the PR description says the change was created with AI assistance (AGENTS.md says "**Always** label AI-assisted commits and PRs. Use one or both of: `Co-Authored-By: <AI tool name and version>` / `Generated-by: <AI tool name and version>`"). https://github.com/apache/thrift/blob/362d95457d960060130f142670a9b3cf794237dd/AGENTS.md#L60-L74 Two suggestions, below the bar for the list above but verified: - The page is about installing the compiler, but the CMake recipe runs with the default options. Those also build the libraries, tests and tutorials (`BUILD_LIBRARIES`, `BUILD_TESTING` and `BUILD_TUTORIALS` default to ON), and the tests need compiled Boost libraries. The compiler-only options used in doc/ReleaseManagement.md (`-DBUILD_TESTING=OFF -DBUILD_TUTORIALS=OFF -DWITH_CPP=OFF … -DBUILD_COMPILER=ON`) would match what the page promises. Also, the macOS CI job only runs the Autotools path, so the recipe marked "(Recommended)" is the one that is not tested. https://github.com/apache/thrift/blob/362d95457d960060130f142670a9b3cf794237dd/doc/install/os_x.md#L28-L37 - `export LDFLAGS="-L$(brew --prefix bison)/lib"` is not needed. bison only runs as a program and nothing links against it; CI only adds bison's `bin` directory to `PATH`. The export also replaces any `LDFLAGS` the reader already has set. https://github.com/apache/thrift/blob/362d95457d960060130f142670a9b3cf794237dd/doc/install/os_x.md#L41-L44 🤖 Generated with [Claude Code](https://claude.ai/code) <sub>- If this code review was useful, please react with 👍. Otherwise, react with 👎.</sub> -- 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]
