You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "kou (via GitHub)" <gi...@apache.org> on 2024/12/23 04:18:20 UTC
[PR] GH-45096: [C++] Apply a cstdint patch to bundled Thrift for GCC 15 [arrow]
kou opened a new pull request, #45097:
URL: https://github.com/apache/arrow/pull/45097
### Rationale for this change
Apache Thrift misses `#include <cstdint>` for `int64_t`: https://github.com/apache/thrift/pull/3078
GCC 15 requires it.
### What changes are included in this PR?
Apply https://github.com/apache/thrift/pull/3078 only with GCC 15.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
No.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45096: [C++] Apply a cstdint patch to bundled Thrift for GCC 15 [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #45097:
URL: https://github.com/apache/arrow/pull/45097#discussion_r1906603637
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -1777,13 +1777,28 @@ macro(build_thrift)
set(THRIFT_DEPENDENCIES ${THRIFT_DEPENDENCIES} boost_ep)
endif()
+ set(THRIFT_PATCH_COMMAND)
+ if(CMAKE_COMPILER_IS_GNUCC AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 15.0)
Review Comment:
I was just surprised, no specific concern. Especially if Thrift has incorporated the patch anyway.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45096: [C++] Apply a cstdint patch to bundled Thrift for GCC 15 [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #45097:
URL: https://github.com/apache/arrow/pull/45097#issuecomment-2585650680
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 76fa19e61af25d124ec0af5e543110a4672088db.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/35486958825) has more details. It also includes information about 16 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45096: [C++] Apply a cstdint patch to bundled Thrift for GCC 15 [arrow]
Posted by "jonkeane (via GitHub)" <gi...@apache.org>.
jonkeane merged PR #45097:
URL: https://github.com/apache/arrow/pull/45097
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45096: [C++] Apply a cstdint patch to bundled Thrift for GCC 15 [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #45097:
URL: https://github.com/apache/arrow/pull/45097#discussion_r1906079385
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -1777,13 +1777,28 @@ macro(build_thrift)
set(THRIFT_DEPENDENCIES ${THRIFT_DEPENDENCIES} boost_ep)
endif()
+ set(THRIFT_PATCH_COMMAND)
+ if(CMAKE_COMPILER_IS_GNUCC AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 15.0)
Review Comment:
Ah, right. But the condition seems overly specific in any case.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45096: [C++] Apply a cstdint patch to bundled Thrift for GCC 15 [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on code in PR #45097:
URL: https://github.com/apache/arrow/pull/45097#discussion_r1905599681
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -1777,13 +1777,28 @@ macro(build_thrift)
set(THRIFT_DEPENDENCIES ${THRIFT_DEPENDENCIES} boost_ep)
endif()
+ set(THRIFT_PATCH_COMMAND)
+ if(CMAKE_COMPILER_IS_GNUCC AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 15.0)
Review Comment:
that is fair, this has been merged without any conditional: https://github.com/apache/thrift/commit/947ad66940cfbadd9b24ba31d892dfc1142dd330
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45096: [C++] Apply a cstdint patch to bundled Thrift for GCC 15 [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #45097:
URL: https://github.com/apache/arrow/pull/45097#discussion_r1905424646
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -1777,13 +1777,28 @@ macro(build_thrift)
set(THRIFT_DEPENDENCIES ${THRIFT_DEPENDENCIES} boost_ep)
endif()
+ set(THRIFT_PATCH_COMMAND)
+ if(CMAKE_COMPILER_IS_GNUCC AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 15.0)
Review Comment:
Why make this conditional? The patch should always be valid anyway.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45096: [C++] Apply a cstdint patch to bundled Thrift for GCC 15 [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #45097:
URL: https://github.com/apache/arrow/pull/45097#discussion_r1906191426
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -1777,13 +1777,28 @@ macro(build_thrift)
set(THRIFT_DEPENDENCIES ${THRIFT_DEPENDENCIES} boost_ep)
endif()
+ set(THRIFT_PATCH_COMMAND)
+ if(CMAKE_COMPILER_IS_GNUCC AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 15.0)
Review Comment:
Hmm. I don't think that the current condition has a problem because GCC 15 is the only known problematic compiler for now. And this is a temporary condition. We will remove this patch when a new Apache Thrift is released.
What is your real concern?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45096: [C++] Apply a cstdint patch to bundled Thrift for GCC 15 [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #45097:
URL: https://github.com/apache/arrow/pull/45097#issuecomment-2558877128
:warning: GitHub issue #45096 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45096: [C++] Apply a cstdint patch to bundled Thrift for GCC 15 [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #45097:
URL: https://github.com/apache/arrow/pull/45097#issuecomment-2564518830
> Thanks for this, yeah this looks right, adding https://gcc.gnu.org/gcc-15/porting_to.html in case it is helpful for others.
Thanks. I didn't notice this page. I've added the information to the Thrift PR.
> I can pick up trying to add this to CI — @kou do you have thoughts on getting `gcc-snapshot` installed reliably? I can keep on trying on #44970 but you'll know what I've been missing there.
I've added some comments to #44970.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45096: [C++] Apply a cstdint patch to bundled Thrift for GCC 15 [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #45097:
URL: https://github.com/apache/arrow/pull/45097#discussion_r1906072528
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -1777,13 +1777,28 @@ macro(build_thrift)
set(THRIFT_DEPENDENCIES ${THRIFT_DEPENDENCIES} boost_ep)
endif()
+ set(THRIFT_PATCH_COMMAND)
+ if(CMAKE_COMPILER_IS_GNUCC AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 15.0)
Review Comment:
If we always use `PATCH_COMMAND`, `patch` command is required in a build machine. We want to avoid it. For example, `patch` command doesn't exist in most Windows machine.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org