-
-
Notifications
You must be signed in to change notification settings - Fork 10
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
WIP: Shared builds on windows #108
base: main
Are you sure you want to change the base?
Conversation
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the changes LGTM, once the builds pass.
cc9d4be
to
2811a8d
Compare
@coryan @hmaarrfk **
|
SGTM |
Ayayay, google explicitly sets abseil (and a bunch of others) as a public link interface for this package... I'm now tempted to rebuild grpc & this against shared abseil, but I'm kinda reluctant to do this because it would force C++17 on any consuming feedstock. The alternative is to add it as a host-dep here and patch out the abseil parts of the link interface. |
NVM, the alternative is to just switch to the corresponding abseil static lib (though we still need to patch it out of the link interface) |
@coryan, just for context here: a lot of the woes here (and the changes in the abseil-feedstock) come from the ABI-sensitivity of abseil to the C++ standard used to compile it. Upstream abseil just supports the case for one uniform C++ version across the entire stack, but we cannot enforce a consistent C++ version in the entire ecosystem (some still require C++11, some need C++17 features), much less cover all through a single shared library (which obviously has to choose a C++-version and hence ABI). The escape hatch are static builds of abseil against C++11/14, while the shared lib uses C++17 (and is to be preferred for those who can use C++17). The problem here (and similarly for grpc-cpp) is that by having Right now I'm just trying how I can get this to work (so don't take the changes here, much less the patches) as final. |
I am well aware.
A different escape hatch would have been to pin Abseil to its C++11 ABI. You compile abseil with C++11 and then change Then Abseil will always use the C++11 type, regardless of what C++ version is in effect at the time it is used.
I think that needs to be
|
That's very relevant/interesting to know, thanks! Though given the issues with that I'm really surprised that exposing abseil types in the public API was a good trade-off.
Also very good to know! I'm not really sure I like that approach TBH, but certainly a thing to keep in mind if I can't get the current approach to work. |
Scratch that, I do like the idea. But unless we only build for the C++11 ABI (which I don't think is great for indirection/performance for those who could also use the C++17 stdlib), we'd still get a virality problem where shared builds of The static build approach seems to be much more work than hoped for, but I liked the idea of separating the internal ABI concerns of a package from those of the ecosystem. Now that |
... unless there'd be a way to do "fat" abseil builds that contain the symbols for both ABIs (C++11/17)?! |
I fear that would create ODR violations. Any function Sometimes you can get that to work (e.g. if one version of the function is only used in a |
Yeah, you're right... 😑 What are your thoughts on the impact of using the C++11 ABI unversally on performance / binary footprint? The issue to me is that most packages can use C++17 shared builds just fine, so we'd be pessimizing everyone just for some corner cases. For now, I think the least bad option might be to move forward with C++17 builds of google-cloud-cpp (against shared C++17 abseil), and potentially having C++14 builds if necessary (like for abseil). |
b5753e3
to
c12bcef
Compare
Ah well, damned if you do (shared), damned if you don't (static): abseil/abseil-cpp#1265 |
I suspect the cognitive load of dealing with
Another solution is to compile https://github.com/abseil/abseil-cpp/blob/43d3c7a4e290ee96684735daf7b1d528c30a7943/CMake/AbseilHelpers.cmake#L282 I have argued that this should be the default behavior in Abseil, i.e., if it was compiled with C++17 it should require C++17 downstream because .. it does. |
Good point. I've slowly come around to that as well, see conda-forge/abseil-cpp-feedstock#45 (feel free to comment if you like).
I agree with this. I even had experimental patches to that effect lined up, but they never got published in one of our GA builds.
Also agreed; that is, unless one does the thing with |
Windows errors should be fixed by abseil/abseil-cpp#1269 |
So I backported this patch, and indeed the missing symbols are now being found 🥳 OTOH, the build is still stuck because
My first guess from (painful) experience is that this happens because no symbols are being exported ( |
e5b2b55
to
1c3af3a
Compare
FWIW, I just looked at the logs. These errors look like a bug in
We should change that to a function. I just created googleapis/google-cloud-cpp#10205 to track that work. |
Great, thanks a lot! |
FWIW. The fixes for googleapis/google-cloud-cpp#10205 are already available in Conda. |
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
Hm, running into missing symbols, more precisely virtual tables. I remembered this from somewhere else, where I had left myself a link to a related SO. Not sure if it applies here. I was hoping we might get further this time, but I don't have time to start playing with patches... 🙈
|
You may be running into this:
https://cmake.org/cmake/help/latest/prop_tgt/WINDOWS_EXPORT_ALL_SYMBOLS.html The virtual table seems like a data symbol. I have tried to fix this before, but gave up as gRPC does not support DLLs for generated code. More details in this bug |
Yeah, this is likely it. Btw, I managed to get grpc to build as a shared library on windows, but the effort to make it work (precisely around that dllimport & export) is substantial, and there's still something going wrong. |
That is really cool work. I wonder if you would be willing to upstream these changes? Yes, they still need work, but I think it is great progress. I don't work in gRPC, but I know some folks that do. I will ask if they are interested in supporting Windows DLLs and ping you with their response. Separately: You probably realized this, even with that work, |
Would be happy to of course! Though for now I didn't feel ready to "bother" upstream grpc with what I was doing. I had opened grpc/grpc#30838 at least, which is the other part of that feedstock PR (now only CMake after upstream grpc already fixed the pkgconfig metadata). But if people are interested, I can open a PR for discussion.
Haha, I already battled with this when trying to build sentencepiece as a shared lib... 🙃 |
Thought I'd give this another whirl. Not much different from last time:
|
So I started looking at this again, here's the list of missing symbols (from the last build, as of 2.11.0):
Normally, I know roughly how to tackle this - add a switch for I believe that a lot of those come through The key issue however is how to pass the required information through the protobuf-generation step. I don't have any experience there, but I doubt that protobuf has capabilities for that? Overall, this still looks like a very tall order... |
As a little update, I've managed to build a shared library for sentencepiece on windows now. Still quite cumbersome though (needs a hand-written protoc plugin), but I've filed an upstream issue for an improvement that would hopefully ease supporting this. (of course, that's still only protobuf, not yet grpc, but one step at a time...) |
Try building shared lib for windows
Continued from #107, also adds myself as maintainer (if desired; otherwise happy to remove)Superseded by #108