-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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.
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
[C++] cross-compilation issues with 18.0.0rc0: inclusion of <nmmintrin.h>
resp. mis-detection of grpc_cpp_plugin
#44448
Comments
Could you share the actual CMake options used for the cross-compilation? |
Sure. This is the CMake call as extracted (and formatted for legibility) from the logs:
|
Hi @h-vetinari , thanks for letting us know. I wonder if this is a regression? (That is, the same build used to pass before?) Thanks. |
Yes, definitely a regression. Conda-forge has been cross-compiling osx-arm64 from osx-64 since arrow v0.17, and never ran into this (to my knowledge; some of it was before my time). |
I think here is a possible temporary workaround: adding an extra CMake option But I can't promise this would be the final solution, which I'll have to talk to community about and come up with. We'll keep you posted of course. Thanks. |
OK. By looking at the log more carefully I seem to get to the root cause. From the log, there is a preset
This is well expected for a cross-compiling config. Then from the output of Arrow CMake:
This is printed by:
For some unknown reason the preset -DCMAKE_SYSTEM_PROCESSOR=arm64 didn't take effect. And it's not surprising that the subsequent SIMD options are set for x86 platform and eventually lead to the erroring header.
UPDATE: The above has been confirmed to be the root cause but the following guess I original posted is NOT the cure.
|
So while we solved the CMake bits for not picking up I noted on the conda-forge side:
We have correct CMake metadata for grpc (we're testing this as part of the feedstock), and so I suspect there's something going wrong with the custom processing in FindgRPCAlt. I've tried to read that file (also |
Could you share the build log URL that includes the error message? |
Does https://github.com/conda-forge/grpc-cpp-feedstock provide |
https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=1060564&view=results |
The plugin definitely gets installed; we haven't modified the CMake installation targets, so it's going to be whatever grpc puts in there by default (so I assume |
It seems that https://anaconda.org/conda-forge/libgrpc/1.67.0/download/osx-64/libgrpc-1.67.0-h0fbbd33_0.conda provides |
Apache Arrow C++ uses only |
Yeah, that makes sense. Though again, we're using the default CMake install of upstream grpc, that split does not come from conda-forge. So in that sense, it would be good for arrow to be(come) compatible with it. |
grpc/grpc@831d2a6 |
I did a quick check on our package content, and also found that this was introduced in 1.52... :) We've been using grpc newer than that in conda-forge for a long time already, so it seems to have worked by accident somehow (or perhaps there were some compatibility targets in the old CMake module that eventually got removed). |
https://packages.ubuntu.com/search?keywords=libgrpc |
Wow, that was already 14 months old when 24.04 got released 😱 Edit: Actually, it seems ubuntu stopped updating libgrc. Even in oracular it's still on 1.51 |
Ah, wait. $ cat libgrpc-1.67.0-h0fbbd33_0/lib/cmake/grpc/gRPCConfig.cmake
# Module path
list(APPEND CMAKE_MODULE_PATH ${CMAKE_CURRENT_LIST_DIR}/modules)
# Depend packages
if(NOT ZLIB_FOUND)
find_package(ZLIB)
endif()
include(CMakeFindDependencyMacro)
find_dependency(Protobuf CONFIG)
if(NOT OPENSSL_FOUND)
find_package(OpenSSL)
endif()
if(NOT c-ares_FOUND)
find_package(c-ares)
endif()
if(NOT TARGET absl::strings)
find_package(absl CONFIG)
endif()
if(NOT re2_FOUND)
find_package(re2)
endif()
# Targets
include(${CMAKE_CURRENT_LIST_DIR}/gRPCTargets.cmake)
if(NOT CMAKE_CROSSCOMPILING)
include(${CMAKE_CURRENT_LIST_DIR}/gRPCPluginTargets.cmake)
endif() |
I was wrong. There is only |
Does conda really need to use cross-compiling? |
Yeah, that makes sense... It's not possible to use the plugins for the target architecture. But in our case we actually supply both, i.e. there's a libgrpc in the build environment (where we can execute the plugins), and then one in the host environment (where we don't need the plugins but need to match the target architecture).
Yes, and this is impossible to change. We don't have native runners for It would be easy on our end to provide some CMake variable, for example |
Hmm. Why can we cross-compile 17.0.0...? We don't have any changes around it... Anyway, could you try GH-44448? |
<nmmintrin.h>
<nmmintrin.h>
with 18.0.0rc0
I think our CMake got confused and didn't actually enter into the |
<nmmintrin.h>
with 18.0.0rc0<nmmintrin.h>
resp. mis-detection of grpc_cpp_plugin
…cross-compiling (#44507) ### Rationale for this change We can't use `find_package(gRPC)` and `gRPC::grpc_cpp_plugin` for cross-compiling because it's for host. We need `grpc_cpp_plugin` for target in cross-compiling. ### What changes are included in this PR? Add `ARROW_GRPC_CPP_PLUGIN` CMake option that overwrites `gRPC::grpc_cpp_plugin` path found by `find_package(gRPC)`. ### Are these changes tested? Yes. conda-forge/arrow-cpp-feedstock#1432 ### Are there any user-facing changes? Yes. * GitHub Issue: #44448 Authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
Issue resolved by pull request 44507 |
Accident or not, it worked for years. When a major distribution channel ends up being broken, that should be the definition of a release blocker, even though we'll manage to backport the patch (again, thanks for that). It's not about the effort to backport, it's about what role conda-forge has. With several million downloads, the arrow feedstock should IMO be important enough to be a release consideration (that's originally why the conda-forge CI was integrated into this repo...) |
It seems that we need to define support level or something like other projects. We may not be able to support all environments because we have limited maintenance resources. How about discuss this on the mailing list? https://arrow.apache.org/community/#mailing-lists |
If there would be no patch for this, I would consider the issue a release blocker. The release is blocked until we have a patch. Mainly because breaking conda-forge being such a big distribution channel is a release blocker for me. In my opinion on how to proceed now that we have a patch and due to the current effort on creating a new Release Candidate with the current limited maintenance resources we have, I feel it's reasonable to port the patch on conda and release RC0. |
As a side note, it may be nice to have at least one CI build to exercise cross-compiling (perhaps as a Crossbow build?). That needs someone to set up, though. |
I've opened an issue to track that #44531 |
The in-repo conda(-forge) CI does cross compilation already. Not sure what's the state of that though, presumably unmaintained? |
Yes, it's unmaintained because it comes with the entire cognitive overhead of conda-forge that none of us is familiar with (except you ;-)). |
People like Uwe and Keith would equally know how to do it, and I would mentor anyone who invests time on the feedstock. 😉 The offer to maintain it myself also still stands, but I cannot do that out of my free time. Perhaps the situation may change/improve now that QuantStack has jumped into the fray (I'm already collaborating with them). 🙃 |
Yes, hopefully. We might get to work together :-) |
Describe the bug, including details regarding any error messages, version, and platform.
I'm testing 18.0.0rc0 in conda-forge, and getting a compilation error on osx-arm64 (which for us is cross-compiled from osx-64). It seems something is going wrong in the following code
arrow/cpp/src/arrow/util/simd.h
Lines 34 to 39 in 5d7987b
in the sense that this tries to include
<nmmintrin.h>
in cross-compilation (probably theHAVE_RUNTIME_X
is based on the architecture of the agent?), despite the target architecture having no support for that.Last relevant-looking change in that area is 87d6477 CC @zanmato1984 @pitrou
Component(s)
C++, Packaging
The text was updated successfully, but these errors were encountered: