Repository navigation
THRIFT-6490: Build the cross-test client and server without Boost - #4108
Open
slachiewicz wants to merge 1 commit into
Open
slachiewicz wants to merge 1 commit into
slachiewicz wants to merge 1 commit into
Conversation
mergeable
Bot
added
c++
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The parser consumes a following option as a value, so --domain-socket --ssl sets an invalid socket path and leaves SSL unset instead of reporting the missing value.
1 open issue
What changed in this PR
This PR removes Boost dependencies from the C++ cross-test client and server, replacing their utility usage with shared C++11 helpers.
Changes:
- Adds command-line parsing and executable-relative test-key discovery helpers.
- Updates the client and server to use these helpers and report option errors with help text.
- Removes Boost discovery, include flags, and library links from the C++ cross-test build configuration.
| File | Description |
|---|---|
test/cpp/src/TestServer.cpp |
Replaces Boost utilities with shared helpers and standard string operations. |
test/cpp/src/TestOptions.h |
Adds command-line parsing, value conversion, and help formatting. |
test/cpp/src/TestKeysDir.h |
Adds upward directory traversal to locate test certificates. |
test/cpp/src/TestClient.cpp |
Replaces Boost utilities and constants with C++11 equivalents. |
test/cpp/Makefile.am |
Registers the new headers and removes Boost build flags and links. |
test/cpp/CMakeLists.txt |
Removes Boost discovery and links from test targets. |
Review effort: Balanced
Client: cpp TestClient and TestServer were the only code outside lib/cpp/test that needed Boost. Two C++11 headers replace it: - TestOptions.h parses the command line. It accepts what the cross-test harness passes and what boost::program_options accepts by default: --name=value, --name value, -x value, -xvalue and unique prefixes of long names. Unknown options, values given to flags and malformed numbers are errors, as before; the programs now print the error and the help and exit with their error code, where the exception from program_options went uncaught. - TestKeysDir.h finds test/keys by walking up from the directory of the executable, as the boost::filesystem code did. boost::starts_with becomes a small helper, BOOST_CONSTEXPR_OR_CONST becomes constexpr, and the unused boost/random/random_device.hpp include goes. test/cpp no longer links boost_program_options, boost_filesystem or boost_system. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
slachiewicz
force-pushed
the
THRIFT-6490
branch
from
November 5, 2026 08:55
ddcd6d6 to
6f8fc75
Compare
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
6 open issues
ifstream(...).good()as an existence check can mis-detect files as “missing” when they… · New--(end-of-options marker) will currently be treated as a short option named'-'and reported… · NewstartsWith()helper is duplicated in bothTestClient.cppandTestServer.cpp. Consider… · New<map>is included but not used anywhere in this header. Removing it reduces compile time and… · New
1 resolved since last review
Review effort: Lite
Comment on lines
+53
to
+55
| if (std::ifstream(dir + "/keys/" + file).good()) { | ||
| return dir.empty() ? "/" : dir; | ||
| } |
Comment on lines
+70
to
+87
| if (arg.compare(0, 2, "--") == 0 && arg.size() > 2) { | ||
| std::string name = arg.substr(2); | ||
| const std::string::size_type eq = name.find('='); | ||
| if (eq != std::string::npos) { | ||
| value = name.substr(eq + 1); | ||
| hasValue = true; | ||
| name.erase(eq); | ||
| } | ||
| option = findLong(name); | ||
| } else if (arg[0] == '-' && arg.size() > 1) { | ||
| option = findShort(arg[1]); | ||
| if (arg.size() > 2) { | ||
| value = arg.substr(2); | ||
| hasValue = true; | ||
| } | ||
| } else { | ||
| throw std::invalid_argument("unexpected argument '" + arg + "'"); | ||
| } |
Comment on lines
+63
to
+66
| // True if s begins with prefix. | ||
| static bool startsWith(const string& s, const char* prefix) { | ||
| return s.rfind(prefix, 0) == 0; | ||
| } |
|
|
||
| #include <cstddef> | ||
| #include <functional> | ||
| #include <map> |
| } | ||
| } | ||
| if (!match) { | ||
| throw std::invalid_argument("unrecognised option '--" + name + "'"); |
| return &option; | ||
| } | ||
| } | ||
| throw std::invalid_argument(std::string("unrecognised option '-") + shortName + "'"); |
Copilot stopped reviewing on behalf of
slachiewicz due to an error
November 5, 2026 09:21
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TestClient and TestServer were the only code outside lib/cpp/test that needed Boost. Two C++11 headers replace it:
TestOptions.hparses the command line, accepting what the cross-test harness passes and whatboost::program_optionsaccepts by default (--name=value,--name value,-x value, unique prefixes of long names), andTestKeysDir.hfindstest/keysby walking up from the executable, as theboost::filesystemcode did.boost::starts_with,BOOST_CONSTEXPR_OR_CONSTand an unusedrandom_deviceinclude go too, so test/cpp neither includes nor links Boost. An option error now prints the message and the help instead of ending with an uncaught exception.Verified: autotools
make -C test/cpp precross→ an SSL TestServer started from test/ as./cpp/TestServerand a TestClient started from/by absolute path both find test/keys, client exit 0; CMake build → ctest 63 of 63 pass, TestServer and TestClient need no Boost library, and a framed/compact run given as--prot=compact --trans=framed -n 2exits 0.