GH-47483: [C++] Bump vendored xxhash to 0.8.3#47476
Conversation
|
|
|
Current failures are related to LLVM, I will rebase once the PR fixing those jobs is merged. |
|
@github-actions crossbow submit -g cpp |
|
Revision: 29e3ab6 Submitted crossbow builds: ursacomputing/crossbow @ actions-9f25a8e327
|
|
It seems that the xxhash bump doesn't fix the test-conda-cpp-valgrind issues: Still, seems worth updating it, @pitrou what do you think? |
|
Yes, it's worth bumping xxhash anyway. |
raulcd
changed the title
|
|
|
It appears this was reported and discussed upstream: Cyan4973/xxHash#943 (comment) We should probably apply the stated workaround:
|
|
@github-actions crossbow submit test-conda-cpp-valgrind |
|
Revision: eb76ca8 Submitted crossbow builds: ursacomputing/crossbow @ actions-d2be094bc2
|
|
|
||
| include(SetupCxxFlags) | ||
|
|
||
| if(${CMAKE_CXX_FLAGS_DEBUG} MATCHES "-Og") |
There was a problem hiding this comment.
No.
In general, we should not use add_definitions() because its scope is global. We should create a target and use it instead:
add_library(arrow::xxhash INTERFACE IMPORTED)
target_compile_definitions(arrow::xxhash INTERFACE "$<$<CONFIG:Debug>:XXH_NO_INLINE_HINTS>")(I think that we don't need -Og check. We can always disable inline hints in debug build.)
There was a problem hiding this comment.
FYI: #47495 is an approach that doesn't use add_definitions().
There was a problem hiding this comment.
Thanks @kou , should we create targets for the different vendored libraries and only link when necessary as a future task too? I am thinking about datetime, double-conversion, etc
There was a problem hiding this comment.
Yes when they need additional build flags. If they don't need additional build flags, we don't need to create targets for them. Because we have src/ in include paths:
Lines 559 to 560 in 743d0af
BTW, we should not use include_directories() because its scope is global (directory) too...
|
The CI failure is now unrelated to building but to a leak on azure-test. |
|
Cool, I suspect we can merge but let's just first wait for AppVeyor to finish? |
|
I restarted the Valgrind job. If the Azure-related failure occurs again, we'll have to file a separate issue for it. |
It has failed again, I've opened a new issue to track the Azure leak: |
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 2284234. There weren't enough matching historic benchmark results to make a call on whether there were regressions. The full Conbench report has more details. |
3 participants
Rationale for this change
We are using an older version, this might fix the current failure for test-conda-cpp-valgrind failures. The upgrade might also come with some improvements / fixes.
What changes are included in this PR?
Update vendored xxhash to newer version:
Are these changes tested?
Yes, via CI.
Are there any user-facing changes?
No