-
-
Notifications
You must be signed in to change notification settings - Fork 16
Windows shared builds #25
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
Conversation
…nda-forge-pinning 2022.02.19.23.26.53
|
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 ( |
|
Hi! This is the friendly automated conda-forge-linting service. I wanted to let you know that I linted all conda-recipes in your PR ( Here's what I've got... For recipe:
|
072dcba to
cb48ebf
Compare
|
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 ( |
|
@conda-forge/abseil-cpp The windows packaging in particular seems to be slicing and dicing things differently, and most importantly is still also building static libs despite the |
| - cmake | ||
| - ninja | ||
|
|
||
| {% set absl_libs = [ |
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.
The bot will probably choke on this, I've seen several feedstock stop autoupdate because they had such a pattern. Could you put this into a test script?
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.
Hm, I'd almost be tempted to rather fix the bot. I like this idiom for testing, keeps things nicely visible in meta.yaml.
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.
The bot will probably choke on this, [...]
So it's mostly about the version updates? I've looked here (and other places in that repo), and it sounds like it'd just need some extra handling for {% ... %} cases.
| # absence of static libs | ||
| - test ! -f $PREFIX/lib/libabsl_{{ each_lib }}.a # [unix] | ||
| # TODO: do we want to get rid of the static windows libs? | ||
| # - if exist %LIBRARY_LIB%\\absl_{{ each_lib }}.lib exit 1 # [win] |
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.
On Windows, you will always have .lib files. In the case of shared DLLs, these are small import libraries needed that only contain "glue" to lazy load the function definitions from the DLLs at runtime.
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.
I had to learn already that .lib is both for static libs as well as the "import libraries" that wrap DLLs, but the logs here look pretty unambiguous to me:
[142/154] Linking CXX static library absl\flags\absl_flags_parse.lib
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.
It seems there's only one DLL (plus its import lib) being created on windows with BUILD_SHARED_LIBS
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.
It's been some time since I last looked at the abseil build system, but my recollection was that on Windows only static builds were supported. Has this changed in the last year or so?
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.
Seems so. abseil_dll.dll is pretty unambiguous as well IMO. ;-)
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.
I am probably being paranoid, and I understand that this would be additional work for you, but would it be possible to have a small tests that checks (on Windows) that the DLL actually works (i.e., compile and run a small executable which calls functions that have been compiled in the DLL)?
I am asking because I am a bit scared by some of the statements in the abseil docs regarding dynamics libraries:
https://abseil.io/about/compatibility
But in any case, thanks a lot for working on this! Dynamic libraries are definitely the way to go for a distribution like conda, it would be great to have this PR merged.
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.
Yeah, AFAICT those warnings are for systems that cannot track ABI as comprehensively as conda(-forge) can.
I have an open PR that directly inspired this need. Not sure if a 5-line #include "absl/xxx" / int main() {} would help much to determine the proper operations, to be honest. What I could think of is publishing this PR to a separate channel (say absl_dev) first, and then I can test it in the sentencepiece PR (though, admittedly, a negative outcome can also just mean that the sentencepiece side isn't working yet).
How does that sound?
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.
I would be personally more in favour of pulling the trigger and merge this PR directly, and perhaps roll it back later in case it turns out it breaks existing packages.
But certainly @xhochy has a better feeling than I do about whether this course of action is appropriate for conda-forge as a whole, so I'll leave the decision to him.
|
Ping @xhochy, would appreciate your thoughts on this (e.g. if you'd agree with the libs as jinja variables if the bot were fixed (resp. which migrators are of concern...); an whether we should be including/excluding the windows static libs despite switching to shared builds) |
|
Gentle ping @xhochy :) |
|
Thank you! BTW, I'd be happy to try fixing the bot so it can update feedstocks with such a test idiom; if you can tell me which kind of migrator I should look at primarily from your POV, it'll probably be faster (but not strictly necessary). |
|
I'm seeing several of Arrow's AppVeyor builds failing to link, and it seems the difference is that it uses the updated version of abseil. These builds all use abseil-cpp build Though, maybe we just need to get gRPC rebuilt against this abseil or something? Example passing: https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/42731674 Example failing (unrelated PRs):
|
|
Apologies for the breakage @lidavidm @kszucs! I'm going to mark the windows builds as broken, which should unblock you hopefully. I think we could test binaries of an eventual re-attempt by publishing them to an
I'm surprised that switching to shared libs causes an issue, because if anything, with only static libs before, those symbols should have been embedded in the consuming binaries. We can try rebuilding grpc too of course. |
|
No worries! Yeah, we can try to work things out on a separate channel. I haven't seriously dug into this yet, either. |
|
Just to confirm, the latest build indeed gets past the C++ build step now that it's back on the |
|
Happy to hear it @lidavidm! I opened an issue upstream and tried a first attempt at a fix in #27 which is pushing into the Would you be willing to open a PR to arrow that tries this out? You'd need to add (the equivalent of) |
|
Thanks for putting this up! I kicked off a build here: (EDIT: it flaked, new build: https://ci.appveyor.com/project/lidavidm/arrow/builds/42761375) |
|
Thanks for testing! I saw that there are now many more missing symbols in your build. Could you confirm that you're compiling with C++17? Abseil's API/ABI depends on the standard that it's been compiled with, and we're at C++17 here (cf. #29). A lot of the symbols on the sentencepiece side resolved themselves once I enforced the C++ standard, but some missing symbols remain - I'm still struggling to understand where these symbols go missing in the upstream CMake infra, though the patterns seems to be that they are all constants; in the case I'm hitting for sentencepiece, e.g. |
|
Ah…no, Arrow is compiling targeting C++11 still: https://github.com/apache/arrow/blob/85f6f7c8fce2746cfde9be7f4dc870e2c48cff2a/cpp/cmake_modules/SetupCxxFlags.cmake#L122-L124 Hmm, this may be a problem then. |
|
PS. How did you trigger the build? I don't see a corresponding PR in the arrow repo... |
|
That build is just because I have AppVeyor set up against my personal fork, but you can trigger a build by opening a PR as well, yes. |
|
Could you try and see what happens if you set it to C++17? |
|
Uh, hmm. I'll have to check in the morning but I think we just didn't notice the bump, then. We do still want to target C++11 because there are supported platforms still on old compilers (I believe down to GCC 4.8 or 4.9), and we want to move to C++14 in the medium term…but jumping wholesale to C++17 seems difficult. Though, for conda, that may be a different question. |
|
Well, as long as you're not getting compile time errors when using C++11 against abseil compiled with C++17, you should be fine 😅 But strictly speaking, I don't think it'll work necessarily (and on windows it certainly doesn't). |
|
Tried kicking off a new build: https://ci.appveyor.com/project/lidavidm/arrow/build/job/8sbrahsh07hx2wet Yeah, it is curious that it works on the other platforms. |
|
Still no dice: https://ci.appveyor.com/project/lidavidm/arrow/builds/42790698/job/fwn13p399k6jmjsj#L1455 The missing symbols are *requested by gRPC. Maybe we do need to rebuild gRPC? |
Ah yes, this certainly looks like gRPC hasn't been compiled with C++17 (which is where |
|
A grpc compiled with C++17 against the shared builds here still runs into some missing symbols, but many less than in the arrow CI: The |
Hopefully helps with the sentencepiece situation...