You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "CJCombrink (via GitHub)" <gi...@apache.org> on 2025/06/06 06:51:25 UTC
[PR] Try to add cpp cross tests [thrift]
CJCombrink opened a new pull request, #3164:
URL: https://github.com/apache/thrift/pull/3164
<!-- Explain the changes in the pull request below: -->
This PR tries to enable cpp cross tests
I am not sure if all of this is needed but I need help getting past the ssl issues, I have tried [many, many times (Build#25 already) ](https://github.com/CJCombrink/thrift/actions/runs/15484246919) and SSL is outside of my comfort zone.
See some of my comments on my internal PR: https://github.com/CJCombrink/thrift/pull/3
It appears to be something about either expired or self signed certificates or both :shrug:
Any help will be appreciated
--
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: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Try to add cpp cross tests [thrift]
Posted by "CJCombrink (via GitHub)" <gi...@apache.org>.
CJCombrink commented on code in PR #3164:
URL: https://github.com/apache/thrift/pull/3164#discussion_r2142390570
##########
lib/cpp/src/thrift/protocol/THeaderProtocol.cpp:
##########
@@ -146,6 +146,10 @@ uint32_t THeaderProtocol::writeBinary(const std::string& str) {
return proto_->writeBinary(str);
}
+uint32_t THeaderProtocol::writeUUID(const TUuid& uuid) {
Review Comment:
@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?
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Try to add cpp cross tests [thrift]
Posted by "CJCombrink (via GitHub)" <gi...@apache.org>.
CJCombrink commented on code in PR #3164:
URL: https://github.com/apache/thrift/pull/3164#discussion_r2142395247
##########
lib/cpp/src/thrift/protocol/TProtocolTap.h:
##########
@@ -166,6 +166,12 @@ class TProtocolTap : public TVirtualProtocol<TProtocolTap> {
return rv;
}
+ uint32_t readUUID(TUuid& uuid) {
Review Comment:
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
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Try to add cpp cross tests [thrift]
Posted by "jimexist (via GitHub)" <gi...@apache.org>.
jimexist commented on code in PR #3164:
URL: https://github.com/apache/thrift/pull/3164#discussion_r2132569225
##########
.github/workflows/build.yml:
##########
@@ -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
Review Comment:
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?
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Try to add cpp cross tests [thrift]
Posted by "CJCombrink (via GitHub)" <gi...@apache.org>.
CJCombrink commented on code in PR #3164:
URL: https://github.com/apache/thrift/pull/3164#discussion_r2136916973
##########
.github/workflows/build.yml:
##########
@@ -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
Review Comment:
@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?
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Try to add cpp cross tests [thrift]
Posted by "CJCombrink (via GitHub)" <gi...@apache.org>.
CJCombrink commented on PR #3164:
URL: https://github.com/apache/thrift/pull/3164#issuecomment-2961479190
Specifically the part that I expect to work that is not is the java Server and C++ Client for SSL.
For example in the cross tests logs (re-ordered them a bit to match pairs)
```
...
java-cpp binary zlib-ip success
java-cpp binary zlib-ip-ssl failure(-6)
...
java-cpp binary framed-ip success
java-cpp binary framed-ip-ssl failure(-6)
...
```
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Try to add cpp cross tests [thrift]
Posted by "CJCombrink (via GitHub)" <gi...@apache.org>.
CJCombrink commented on code in PR #3164:
URL: https://github.com/apache/thrift/pull/3164#discussion_r2131622637
##########
lib/cpp/test/OneWayHTTPTest.cpp:
##########
@@ -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));
Review Comment:
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.
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Try to add cpp cross tests [thrift]
Posted by "CJCombrink (via GitHub)" <gi...@apache.org>.
CJCombrink commented on PR #3164:
URL: https://github.com/apache/thrift/pull/3164#issuecomment-2957694290
> lgtm modulo the java version change
@fishy Thanks for you review thus far.
As mentioned, I want to drop the java change but something needs to be fixed to fix the java/kotlin cross tests.
The C++ SSL cross tests all still fail and I need some help to get past that. The worst case would be to mark them as known failures but ideally I would like to solve the issue.
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Try to add cpp cross tests [thrift]
Posted by "fishy (via GitHub)" <gi...@apache.org>.
fishy commented on PR #3164:
URL: https://github.com/apache/thrift/pull/3164#issuecomment-2963306362
There's probably some known issue with java cross test ssl handling. If you look at other cross tests, or history or cross tests of prior runs, it seems to only work between java-java and java-kotlin, and never work with other languages.
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Try to add cpp cross tests [thrift]
Posted by "CJCombrink (via GitHub)" <gi...@apache.org>.
CJCombrink commented on code in PR #3164:
URL: https://github.com/apache/thrift/pull/3164#discussion_r2131628894
##########
test/cpp/src/TestClient.cpp:
##########
@@ -321,9 +322,15 @@ int main(int argc, char** argv) {
std::shared_ptr<TProtocol> protocol2; // SecondService for multiplexed
if (ssl) {
- cout << "Client Certificate File: " << certPath << '\n';
- cout << "Client Key File: " << keyPath << '\n';
- cout << "CA File: " << caPath << '\n';
+ auto fileExists = [](const std::string& path) {
+ std::ifstream f(path.c_str());
+ return f.good();
+ };
+
+ cout << "Client Path : " << testDir << '\n';
+ cout << "Client Certificate File: " << certPath << " (" << std::boolalpha << fileExists(certPath) << ")"<< '\n';
Review Comment:
I see there are v3 certs in the repo but the client above does not use them, not sure if it can be related to my issues
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Try to add cpp cross tests [thrift]
Posted by "CJCombrink (via GitHub)" <gi...@apache.org>.
CJCombrink commented on PR #3164:
URL: https://github.com/apache/thrift/pull/3164#issuecomment-2967907746
Hoping for some answers but closing this PR for the more cleaned up https://github.com/apache/thrift/pull/3167
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Try to add cpp cross tests [thrift]
Posted by "CJCombrink (via GitHub)" <gi...@apache.org>.
CJCombrink commented on PR #3164:
URL: https://github.com/apache/thrift/pull/3164#issuecomment-2967859522
I am starting to clean this up and split out the relevant parts to relevant PRs.
@jimexist I still need some guidance on what is the correct fix for the kotlin error (https://github.com/apache/thrift/pull/3164#discussion_r2131618892)
@Jens-G Can I bundle the two fixes into one PR that also adds the cross tests (https://github.com/apache/thrift/pull/3164#discussion_r2142395247, https://github.com/apache/thrift/pull/3164#discussion_r2142390570)
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Try to add cpp cross tests [thrift]
Posted by "CJCombrink (via GitHub)" <gi...@apache.org>.
CJCombrink commented on code in PR #3164:
URL: https://github.com/apache/thrift/pull/3164#discussion_r2131622637
##########
lib/cpp/test/OneWayHTTPTest.cpp:
##########
@@ -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));
Review Comment:
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_
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Try to add cpp cross tests [thrift]
Posted by "CJCombrink (via GitHub)" <gi...@apache.org>.
CJCombrink commented on code in PR #3164:
URL: https://github.com/apache/thrift/pull/3164#discussion_r2131618892
##########
.github/workflows/build.yml:
##########
@@ -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
Review Comment:
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:
```
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Try to add cpp cross tests [thrift]
Posted by "CJCombrink (via GitHub)" <gi...@apache.org>.
CJCombrink closed pull request #3164: Try to add cpp cross tests
URL: https://github.com/apache/thrift/pull/3164
--
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: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Try to add cpp cross tests [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on code in PR #3164:
URL: https://github.com/apache/thrift/pull/3164#discussion_r2143639652
##########
lib/cpp/src/thrift/protocol/THeaderProtocol.cpp:
##########
@@ -146,6 +146,10 @@ uint32_t THeaderProtocol::writeBinary(const std::string& str) {
return proto_->writeBinary(str);
}
+uint32_t THeaderProtocol::writeUUID(const TUuid& uuid) {
Review Comment:
> Can I just create a Jira Ticket for it and we close it part of this PR
I think either way is fine.
##########
lib/cpp/src/thrift/protocol/THeaderProtocol.cpp:
##########
@@ -146,6 +146,10 @@ uint32_t THeaderProtocol::writeBinary(const std::string& str) {
return proto_->writeBinary(str);
}
+uint32_t THeaderProtocol::writeUUID(const TUuid& uuid) {
Review Comment:
> Can I just create a Jira Ticket for it and we close it part of this PR
I think either way is fine.
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Try to add cpp cross tests [thrift]
Posted by "CJCombrink (via GitHub)" <gi...@apache.org>.
CJCombrink commented on code in PR #3164:
URL: https://github.com/apache/thrift/pull/3164#discussion_r2143660313
##########
.github/workflows/build.yml:
##########
@@ -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
Review Comment:
I created a ticket: [THRIFT-5879: java and kotlin cross tests fail in the GitHub action](https://issues.apache.org/jira/browse/THRIFT-5879)
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Try to add cpp cross tests [thrift]
Posted by "CJCombrink (via GitHub)" <gi...@apache.org>.
CJCombrink commented on code in PR #3164:
URL: https://github.com/apache/thrift/pull/3164#discussion_r2131618892
##########
.github/workflows/build.yml:
##########
@@ -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
Review Comment:
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
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Try to add cpp cross tests [thrift]
Posted by "fishy (via GitHub)" <gi...@apache.org>.
fishy commented on code in PR #3164:
URL: https://github.com/apache/thrift/pull/3164#discussion_r2132458766
##########
.github/workflows/build.yml:
##########
@@ -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
Review Comment:
cc @jimexist to chime in.
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Try to add cpp cross tests [thrift]
Posted by "CJCombrink (via GitHub)" <gi...@apache.org>.
CJCombrink commented on code in PR #3164:
URL: https://github.com/apache/thrift/pull/3164#discussion_r2131626677
##########
test/cpp/src/TestClient.cpp:
##########
@@ -321,9 +322,15 @@ int main(int argc, char** argv) {
std::shared_ptr<TProtocol> protocol2; // SecondService for multiplexed
if (ssl) {
- cout << "Client Certificate File: " << certPath << '\n';
- cout << "Client Key File: " << keyPath << '\n';
- cout << "CA File: " << caPath << '\n';
+ auto fileExists = [](const std::string& path) {
+ std::ifstream f(path.c_str());
+ return f.good();
+ };
+
+ cout << "Client Path : " << testDir << '\n';
+ cout << "Client Certificate File: " << certPath << " (" << std::boolalpha << fileExists(certPath) << ")"<< '\n';
Review Comment:
Trying to debug SSL and cert issues I though the certs are missing in the pipeline.
Concluded through this code that it was not the issue and decided to keep this here since it can be useful in future
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Try to add cpp cross tests [thrift]
Posted by "jimexist (via GitHub)" <gi...@apache.org>.
jimexist commented on code in PR #3164:
URL: https://github.com/apache/thrift/pull/3164#discussion_r2132569225
##########
.github/workflows/build.yml:
##########
@@ -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
Review Comment:
it was not a typo.
that `UnsupportedClassVersionError` likely from kotlin which isn't configured with the same version as the java lib project?
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Try to add cpp cross tests [thrift]
Posted by "CJCombrink (via GitHub)" <gi...@apache.org>.
CJCombrink commented on code in PR #3164:
URL: https://github.com/apache/thrift/pull/3164#discussion_r2136915830
##########
.github/workflows/build.yml:
##########
@@ -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
Review Comment:
There is a test in ToStringTest.cpp that is part of the lib tests that requires this, else the tests fails.
See [ToStringTest.cpp#L57](https://github.com/apache/thrift/blob/master/lib/cpp/test/ToStringTest.cpp#L57)
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Try to add cpp cross tests [thrift]
Posted by "CJCombrink (via GitHub)" <gi...@apache.org>.
CJCombrink commented on PR #3164:
URL: https://github.com/apache/thrift/pull/3164#issuecomment-2965151052
> There's probably some known issue with java cross test ssl handling
Thanks for the info, it does in deed look like a known issue.
I am trying to spend some time to figure out why does cpp <-> cpp not all work and then perhaps see where I am at and maybe mark the ssl ones as know issues.
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Try to add cpp cross tests [thrift]
Posted by "CJCombrink (via GitHub)" <gi...@apache.org>.
CJCombrink commented on code in PR #3164:
URL: https://github.com/apache/thrift/pull/3164#discussion_r2142898366
##########
test/cpp/src/TestClient.cpp:
##########
@@ -321,9 +322,15 @@ int main(int argc, char** argv) {
std::shared_ptr<TProtocol> protocol2; // SecondService for multiplexed
if (ssl) {
- cout << "Client Certificate File: " << certPath << '\n';
- cout << "Client Key File: " << keyPath << '\n';
- cout << "CA File: " << caPath << '\n';
+ auto fileExists = [](const std::string& path) {
+ std::ifstream f(path.c_str());
+ return f.good();
+ };
+
+ cout << "Client Path : " << testDir << '\n';
+ cout << "Client Certificate File: " << certPath << " (" << std::boolalpha << fileExists(certPath) << ")"<< '\n';
Review Comment:
Appears to not be related
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Try to add cpp cross tests [thrift]
Posted by "fishy (via GitHub)" <gi...@apache.org>.
fishy commented on code in PR #3164:
URL: https://github.com/apache/thrift/pull/3164#discussion_r2132415318
##########
.github/workflows/build.yml:
##########
@@ -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
Review Comment:
is this one needed?
##########
.github/workflows/build.yml:
##########
@@ -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
Review Comment:
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.
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org