Skip to content

Try to add cpp cross tests #3164

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
CJCombrink wants to merge 13 commits into from
Closed

Try to add cpp cross tests #3164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
5329796
Use latest thrift file for cpp
CJCombrink Jun 5, 2025
5ae0e1a
Remove lisp from building by default
CJCombrink Jun 5, 2025
63a37e7
Add cpp cross tests
CJCombrink Jun 5, 2025
c29b4ec
Be explicit about localhost ip
CJCombrink Jun 5, 2025
9521348
Missing locales for the cpp tests
CJCombrink Jun 5, 2025
f673f4d
Try to upload everything that is needed for the cpp tests
CJCombrink Jun 5, 2025
f4cf10e
Missing libraries for cpp tests
CJCombrink Jun 5, 2025
54a2210
Update java version for cross test
CJCombrink Jun 5, 2025
764e213
Trying to debug the cpp ssl issues
CJCombrink Jun 6, 2025
f8e6ec8
UUID support for THeaderProtocol
CJCombrink Jun 12, 2025
a4aee68
UUID support for TProtocolTap
CJCombrink Jun 12, 2025
dbb2c52
Just sort the known failures
CJCombrink Jun 12, 2025
5b067a7
Mark cpp and java ssl tests as known failures
CJCombrink Jun 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 84 additions & 5 deletions .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ env:
--without-lua
--without-rs
--without-swift
--without-cl

permissions:
contents: read
Expand Down Expand Up @@ -552,20 +553,86 @@ jobs:
- name: Run ts tests
run: make -C lib/nodets check

lib-cpp:
needs: compiler
runs-on: ubuntu-24.04
steps:
- uses: actions/checkout@v4

- name: Install dependencies
run: |
sudo apt-get update -yq
sudo apt-get install -y --no-install-recommends g++ $BUILD_DEPS locales
sudo locale-gen en_US.UTF-8
sudo locale-gen de_DE.UTF-8
sudo update-locale

- name: Run bootstrap
run: ./bootstrap.sh

- name: Run configure
run: |
./configure $(echo $CONFIG_ARGS_FOR_LIBS | sed -E 's/without-cpp/with-cpp/g')

- uses: actions/download-artifact@v4
with:
name: thrift-compiler
path: compiler/cpp

- name: Run thrift-compiler
run: |
chmod a+x compiler/cpp/thrift
compiler/cpp/thrift -version

- name: Run make for cpp
run: make -j$(nproc) -C lib/cpp

- name: Run make check for lib/cpp
run: make -j$(nproc) -C lib/cpp check

- name: Run make check for test/cpp
run: make -j$(nproc) -C test/cpp check

- name: Run make precross for cpp test
run: make -j$(nproc) -C test/cpp precross

- name: Upload cpp precross artifacts
uses: actions/upload-artifact@v4
with:
name: cpp-precross
if-no-files-found: error
include-hidden-files: true
path: |
test/cpp/TestClient
test/cpp/TestServer
test/cpp/.libs/TestClient
test/cpp/.libs/TestServer
lib/cpp/.libs/*.so
retention-days: 3

- name: Upload log files from failed test runs
uses: actions/upload-artifact@v4
if: failure()
with:
name: lib-cpp-test-log
path: lib/cpp/test/*.xml
retention-days: 3

cross-test:
needs:
- lib-java-kotlin
#- lib-swift # swift is currently broken and no maintainers around -> see THRIFT-5864
- lib-rust
- lib-go
- lib-python
- lib-cpp
runs-on: ubuntu-24.04
strategy:
matrix:
# swift is currently broken and no maintainers around -> see THRIFT-5864
server_lang: ['java', 'kotlin', 'go', 'rs'] # ['java', 'kotlin', 'go', 'rs', 'swift']
server_lang: ['java', 'kotlin', 'go', 'rs', 'cpp'] # ['java', 'kotlin', 'go', 'rs', 'swift']
# we always use comma join as many client langs as possible, to reduce the number of jobs
client_lang: ['java,kotlin', 'go,rs'] # ['java,kotlin', 'go,rs', 'swift']
client_lang: ['java,kotlin', 'go,rs,cpp'] # ['java,kotlin', 'go,rs', 'swift']
fail-fast: false
steps:
- uses: actions/checkout@v4
Expand All @@ -577,14 +644,17 @@ jobs:
- uses: actions/setup-java@v4
with:
distribution: temurin
# here we intentionally use an older version so that we also verify Java 17 compiles to it
Copy link
Contributor Author

@CJCombrink CJCombrink Jun 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment actually true?
I know Java compiled on an old version should work with newer versions: This is not the first time I run into the issue with a old version trying something compiled with a new version.
The logs print this:

Exception in thread "main" java.lang.UnsupportedClassVersionError: org/apache/thrift/test/TestClientKt has been compiled by a more recent version of the Java Runtime (class file version 61.0), this version of the Java Runtime only recognizes class file versions up to 52.0

Which proves the statement wrong

Copy link
Member

@fishy fishy Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the comment about java 17 is probably a typo? the one we want to make sure is probably java 7 not 17?

I can't remember the exact details but I think we do want to maintain compatibility with older lts java versions.

Copy link
Member

@fishy fishy Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @jimexist to chime in.

Copy link
Member

@jimexist jimexist Jun 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was not a typo.

that UnsupportedClassVersionError likely from kotlin lib which isn't configured with the same compiler flags as the java lib project?

Copy link
Contributor Author

@CJCombrink CJCombrink Jun 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jimexist Thanks for dropping in and the update: I want to drop this change from my PR since it does not belong, but what is the correct fix for this that should go into the repo to ensure that java/kotlin tests pass without this issue?

Copy link
Contributor Author

@CJCombrink CJCombrink Jun 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a ticket: THRIFT-5879: java and kotlin cross tests fail in the GitHub action

java-version: 8
java-version: 17
cache: "gradle"

- name: Install openssl and certificates (for SSL tests)
run: |
sudo apt-get update -yq
sudo apt-get install -y --no-install-recommends openssl ca-certificates
sudo apt-get install -y --no-install-recommends \
openssl \
ca-certificates \
libboost-all-dev \
libevent-dev

- name: Download java precross artifacts
uses: actions/download-artifact@v4
Expand Down Expand Up @@ -617,6 +687,12 @@ jobs:
name: go-precross
path: test/go/bin

- name: Download cpp precross artifacts
uses: actions/download-artifact@v4
with:
name: cpp-precross
path: .

- name: Set back executable flags
run: |
chmod a+x lib/java/build/run*
Expand All @@ -625,6 +701,9 @@ jobs:
# THRIFT-5864 chmod a+x test/swift/CrossTests/.build/x86_64-unknown-linux-gnu/debug/*
chmod a+x test/rs/bin/*
chmod a+x test/go/bin/*
chmod a+x test/cpp/*
chmod a+x test/cpp/.libs/*
chmod a+x lib/cpp/.libs/*.so

- name: Create tmp domain socket folder
run: mkdir /tmp/v0.16
Expand Down
8 changes: 8 additions & 0 deletions lib/cpp/src/thrift/protocol/THeaderProtocol.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,10 @@ uint32_t THeaderProtocol::writeBinary(const std::string& str) {
return proto_->writeBinary(str);
}

uint32_t THeaderProtocol::writeUUID(const TUuid& uuid) {
Copy link
Contributor Author

@CJCombrink CJCombrink Jun 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Jens-G This is a bugfix for the THeaderProtocol.
Can I just create a Jira Ticket for it and we close it part of this PR (once I get there) or do I need to pull this out into a new PR for the issue?

Copy link
Member

@Jens-G Jens-G Jun 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I just create a Jira Ticket for it and we close it part of this PR

I think either way is fine.

return proto_->writeUUID(uuid);
}

/**
* Reading functions
*/
Expand Down Expand Up @@ -246,6 +250,10 @@ uint32_t THeaderProtocol::readString(std::string& str) {
uint32_t THeaderProtocol::readBinary(std::string& binary) {
return proto_->readBinary(binary);
}

uint32_t THeaderProtocol::readUUID(TUuid& uuid) {
return proto_->readUUID(uuid);
}
}
}
} // apache::thrift::protocol
Expand Down
4 changes: 4 additions & 0 deletions lib/cpp/src/thrift/protocol/THeaderProtocol.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,8 @@ class THeaderProtocol : public TVirtualProtocol<THeaderProtocol> {

uint32_t writeBinary(const std::string& str);

uint32_t writeUUID(const TUuid& uuid);

/**
* Reading functions
*/
Expand Down Expand Up @@ -181,6 +183,8 @@ class THeaderProtocol : public TVirtualProtocol<THeaderProtocol> {

uint32_t readBinary(std::string& binary);

uint32_t readUUID(TUuid& uuid);

protected:
std::shared_ptr<THeaderTransport> trans_;

Expand Down
6 changes: 6 additions & 0 deletions lib/cpp/src/thrift/protocol/TProtocolTap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,12 @@ class TProtocolTap : public TVirtualProtocol<TProtocolTap> {
return rv;
}

uint32_t readUUID(TUuid& uuid) {
Copy link
Contributor Author

@CJCombrink CJCombrink Jun 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just saw this as missing, I will see if it makes sense to add a test for this protocol since I see there aren't any

uint32_t rv = source_->readUUID(uuid);
sink_->writeUUid(uuid);
return rv;
}

private:
std::shared_ptr<TProtocol> source_;
std::shared_ptr<TProtocol> sink_;
Expand Down
4 changes: 2 additions & 2 deletions lib/cpp/test/OneWayHTTPTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,10 +202,10 @@ BOOST_AUTO_TEST_CASE( JSON_BufferedHTTP )
#endif

{
std::shared_ptr<TSocket> socket(new TSocket("localhost", port));
std::shared_ptr<TSocket> socket(new TSocket("127.0.0.1", port));
Copy link
Contributor Author

@CJCombrink CJCombrink Jun 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to be an issue with localhost getting resolved to the IPv6 address ::1 and stuff breaks.
I am actually reconsidering this commit and instead for the pipeline purge the IPv6 ::1 localhost from the /etc/hosts file in the pipeline.

Or one that knows more about IPv6 and the socket libs used can perhaps see if this can be fixed in the library; I don't know enough of IPv6 and the libraries used to know if this is even an option

socket->setRecvTimeout(10000) ; // 1000msec should be enough
std::shared_ptr<TBlockableBufferedTransport> blockable_transport(new TBlockableBufferedTransport(socket));
std::shared_ptr<TTransport> transport(new THttpClient(blockable_transport, "localhost", "/service"));
std::shared_ptr<TTransport> transport(new THttpClient(blockable_transport, "127.0.0.1", "/service"));
std::shared_ptr<TProtocol> protocol(new TJSONProtocol(transport));
onewaytest::OneWayServiceClient client(protocol);

Expand Down
4 changes: 2 additions & 2 deletions lib/cpp/test/SecurityFromBufferTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ struct SecurityFromBufferFixture {
pServerSocketFactory->loadCertificateFromBuffer(certString("server.crt").c_str());
pServerSocketFactory->loadPrivateKeyFromBuffer(certString("server.key").c_str());
pServerSocketFactory->server(true);
pServerSocket.reset(new TSSLServerSocket("localhost", 0, pServerSocketFactory));
pServerSocket.reset(new TSSLServerSocket("127.0.0.1", 0, pServerSocketFactory));
shared_ptr<TTransport> connectedClient;

try {
Expand Down Expand Up @@ -169,7 +169,7 @@ struct SecurityFromBufferFixture {
pClientSocketFactory->loadCertificateFromBuffer(certString("client.crt").c_str());
pClientSocketFactory->loadPrivateKeyFromBuffer(certString("client.key").c_str());
pClientSocketFactory->loadTrustedCertificatesFromBuffer(certString("CA.pem").c_str());
pClientSocket = pClientSocketFactory->createSocket("localhost", mPort);
pClientSocket = pClientSocketFactory->createSocket("127.0.0.1", mPort);
pClientSocket->open();

uint8_t buf[3];
Expand Down
4 changes: 2 additions & 2 deletions lib/cpp/test/SecurityTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ struct SecurityFixture
pServerSocketFactory->loadCertificate(certFile("server.crt").string().c_str());
pServerSocketFactory->loadPrivateKey(certFile("server.key").string().c_str());
pServerSocketFactory->server(true);
pServerSocket.reset(new TSSLServerSocket("localhost", 0, pServerSocketFactory));
pServerSocket.reset(new TSSLServerSocket("127.0.0.1", 0, pServerSocketFactory));
shared_ptr<TTransport> connectedClient;

try
Expand Down Expand Up @@ -176,7 +176,7 @@ struct SecurityFixture
pClientSocketFactory->loadCertificate(certFile("client.crt").string().c_str());
pClientSocketFactory->loadPrivateKey(certFile("client.key").string().c_str());
pClientSocketFactory->loadTrustedCertificates(certFile("CA.pem").string().c_str());
pClientSocket = pClientSocketFactory->createSocket("localhost", mPort);
pClientSocket = pClientSocketFactory->createSocket("127.0.0.1", mPort);
pClientSocket->open();

uint8_t buf[3];
Expand Down
2 changes: 1 addition & 1 deletion lib/cpp/test/TNonblockingSSLServerTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ class Fixture {

bool canCommunicate(int serverPort) {
std::shared_ptr<TSSLSocketFactory> pClientSocketFactory = createClientSocketFactory();
std::shared_ptr<TSSLSocket> socket = pClientSocketFactory->createSocket("localhost", serverPort);
std::shared_ptr<TSSLSocket> socket = pClientSocketFactory->createSocket("127.0.0.1", serverPort);
socket->open();
test::ParentServiceClient client(std::make_shared<protocol::TBinaryProtocol>(
std::make_shared<transport::TFramedTransport>(socket)));
Expand Down
2 changes: 1 addition & 1 deletion lib/cpp/test/TNonblockingServerTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ class Fixture {
}

bool canCommunicate(int serverPort) {
shared_ptr<transport::TSocket> socket(new transport::TSocket("localhost", serverPort));
shared_ptr<transport::TSocket> socket(new transport::TSocket("127.0.0.1", serverPort));
socket->open();
test::ParentServiceClient client(make_shared<protocol::TBinaryProtocol>(
make_shared<transport::TFramedTransport>(socket)));
Expand Down
22 changes: 11 additions & 11 deletions lib/cpp/test/TSSLSocketInterruptTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,11 +139,11 @@ shared_ptr<TSSLSocketFactory> createClientSocketFactory() {

BOOST_AUTO_TEST_CASE(test_ssl_interruptable_child_read_while_handshaking) {
shared_ptr<TSSLSocketFactory> pServerSocketFactory = createServerSocketFactory();
TSSLServerSocket sock1("localhost", 0, pServerSocketFactory);
TSSLServerSocket sock1("127.0.0.1", 0, pServerSocketFactory);
sock1.listen();
int port = sock1.getPort();
shared_ptr<TSSLSocketFactory> pClientSocketFactory = createClientSocketFactory();
shared_ptr<TSSLSocket> clientSock = pClientSocketFactory->createSocket("localhost", port);
shared_ptr<TSSLSocket> clientSock = pClientSocketFactory->createSocket("127.0.0.1", port);
clientSock->open();
shared_ptr<TTransport> accepted = sock1.accept();
boost::thread readThread(std::bind(readerWorkerMustThrow, accepted));
Expand All @@ -159,11 +159,11 @@ BOOST_AUTO_TEST_CASE(test_ssl_interruptable_child_read_while_handshaking) {

BOOST_AUTO_TEST_CASE(test_ssl_interruptable_child_read) {
shared_ptr<TSSLSocketFactory> pServerSocketFactory = createServerSocketFactory();
TSSLServerSocket sock1("localhost", 0, pServerSocketFactory);
TSSLServerSocket sock1("127.0.0.1", 0, pServerSocketFactory);
sock1.listen();
int port = sock1.getPort();
shared_ptr<TSSLSocketFactory> pClientSocketFactory = createClientSocketFactory();
shared_ptr<TSSLSocket> clientSock = pClientSocketFactory->createSocket("localhost", port);
shared_ptr<TSSLSocket> clientSock = pClientSocketFactory->createSocket("127.0.0.1", port);
clientSock->open();
shared_ptr<TTransport> accepted = sock1.accept();
boost::thread readThread(std::bind(readerWorkerMustThrow, accepted));
Expand All @@ -180,12 +180,12 @@ BOOST_AUTO_TEST_CASE(test_ssl_interruptable_child_read) {

BOOST_AUTO_TEST_CASE(test_ssl_non_interruptable_child_read) {
shared_ptr<TSSLSocketFactory> pServerSocketFactory = createServerSocketFactory();
TSSLServerSocket sock1("localhost", 0, pServerSocketFactory);
TSSLServerSocket sock1("127.0.0.1", 0, pServerSocketFactory);
sock1.setInterruptableChildren(false); // returns to pre-THRIFT-2441 behavior
sock1.listen();
int port = sock1.getPort();
shared_ptr<TSSLSocketFactory> pClientSocketFactory = createClientSocketFactory();
shared_ptr<TSSLSocket> clientSock = pClientSocketFactory->createSocket("localhost", port);
shared_ptr<TSSLSocket> clientSock = pClientSocketFactory->createSocket("127.0.0.1", port);
clientSock->open();
shared_ptr<TTransport> accepted = sock1.accept();
static_pointer_cast<TSSLSocket>(accepted)->setRecvTimeout(1000);
Expand All @@ -206,7 +206,7 @@ BOOST_AUTO_TEST_CASE(test_ssl_non_interruptable_child_read) {

BOOST_AUTO_TEST_CASE(test_ssl_cannot_change_after_listen) {
shared_ptr<TSSLSocketFactory> pServerSocketFactory = createServerSocketFactory();
TSSLServerSocket sock1("localhost", 0, pServerSocketFactory);
TSSLServerSocket sock1("127.0.0.1", 0, pServerSocketFactory);
sock1.listen();
BOOST_CHECK_THROW(sock1.setInterruptableChildren(false), std::logic_error);
sock1.close();
Expand Down Expand Up @@ -234,11 +234,11 @@ void peekerWorkerInterrupt(shared_ptr<TTransport> tt) {

BOOST_AUTO_TEST_CASE(test_ssl_interruptable_child_peek) {
shared_ptr<TSSLSocketFactory> pServerSocketFactory = createServerSocketFactory();
TSSLServerSocket sock1("localhost", 0, pServerSocketFactory);
TSSLServerSocket sock1("127.0.0.1", 0, pServerSocketFactory);
sock1.listen();
int port = sock1.getPort();
shared_ptr<TSSLSocketFactory> pClientSocketFactory = createClientSocketFactory();
shared_ptr<TSSLSocket> clientSock = pClientSocketFactory->createSocket("localhost", port);
shared_ptr<TSSLSocket> clientSock = pClientSocketFactory->createSocket("127.0.0.1", port);
clientSock->open();
shared_ptr<TTransport> accepted = sock1.accept();
boost::thread peekThread(std::bind(peekerWorkerInterrupt, accepted));
Expand All @@ -255,12 +255,12 @@ BOOST_AUTO_TEST_CASE(test_ssl_interruptable_child_peek) {

BOOST_AUTO_TEST_CASE(test_ssl_non_interruptable_child_peek) {
shared_ptr<TSSLSocketFactory> pServerSocketFactory = createServerSocketFactory();
TSSLServerSocket sock1("localhost", 0, pServerSocketFactory);
TSSLServerSocket sock1("127.0.0.1", 0, pServerSocketFactory);
sock1.setInterruptableChildren(false); // returns to pre-THRIFT-2441 behavior
sock1.listen();
int port = sock1.getPort();
shared_ptr<TSSLSocketFactory> pClientSocketFactory = createClientSocketFactory();
shared_ptr<TSSLSocket> clientSock = pClientSocketFactory->createSocket("localhost", port);
shared_ptr<TSSLSocket> clientSock = pClientSocketFactory->createSocket("127.0.0.1", port);
clientSock->open();
shared_ptr<TTransport> accepted = sock1.accept();
static_pointer_cast<TSSLSocket>(accepted)->setRecvTimeout(1000);
Expand Down
Loading