-
Notifications
You must be signed in to change notification settings - Fork 85
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
libawkward export tuning #316
libawkward export tuning #316
Conversation
Hi! Thanks for looking into this again. Since I work on Linux, I've been seeing the warnings for a while now, thinking I should look into it and figure out how to fix it. Maybe that's still true because the warnings are still present in the Linux build (below is from Azure, the Linux + Python 3.8 build). I also tried compiling it on my computer, and see the same warnings.
|
@jpivarski Yes, there is a third change coming to fix those. |
Hi, @veprbl! I'm checking in on your PR because it has been a long time. Is this something you intend to finish? If not, then I'll close it. Meanwhile, I'm going to have to do something about the warnings when building on Linux. Doing so may affect the downstream linking on MacOS that you got working. Do you have a minimal example that I'd be able to include as a test, so that when I fix the compile-time warnings on Linux I don't break downstream linking on MacOS? Would the dependent project, as it is currently defined, be an appropriate test? If so, I'll include this or whatever test you give me into CI so that I don't break things for you. Since I don't have a Mac, CI is the only way I have to know that I'm not breaking your use-case. Thanks! |
@jpivarski There was a delay, but I'm back at it now. This was already ready, all that was missing was some extra explanations as for what I'm doing and why. I will rebase and update this now. |
In that case, now is a better time than an hour ago—I just put in a big change. I think that nothing has touched the linking process, and maybe even CMakeLists.txt is unchanged since you were last working on this, but there have been some substantial rearrangements of the code. (Moved header files and several search-and-replacements, but I don't think the lines you changed—the first line of each class declaration—have changed.) |
@jpivarski I see. I will look into the next rebase tomorrow. |
MRE https://godbolt.org/z/imCwCW : template <typename T> class V c {}; extern template class c<int>; gives warning C4910: 'c<int>': '__declspec(dllexport)' and 'extern' are incompatible on an explicit instantiation
The consumers of libawkward's headers are not supposed to export the symbols of libawkward. Those should be either instantiated privately or imported from libawkward.
… is already defined
No warnings in Linux; that's great!
|
This PR is mostly aimed at silencing warnings in a reasonable way. The only visible change should be that the dependent libraries should not re-export any libawkward symbols, before it was pointless to set the default visibility to hidden. |
Okay, thanks for all of your help with this. The differences look good to me. I do need to get the dependent project into the unit tests; something I'll tackle soon. The thing that was preventing it was not substantial—it was something like setting the PATH on Azure (I couldn't find where it had installed into). |
@jpivarski Sorry, I don't quite follow you on the dependent project thread. I haven't looked at this yet. On another note, I must admit I still haven't tried applying awkward1 for my analysis, so I had some questions for you. Is it somewhat ready for use? Is parquet file reading supported? |
https://github.com/scikit-hep/awkward-1.0/tree/master/dependent-project is an example of a C++ project that links to (depends on) Awkward. It used to be part of CI, then I had to drop it when something broke and I didn't know exactly what, now I'd like to reinstate it, since I think you've fixed linking issues, at least on MacOS. Other thread: it is ready to use, and some analysis groups have been using it. Generally speaking, it's more complete and self-consistent than Awkward 0 was, and is better documented, too. Most of my work recently has been responding to bug reports and feature requests of users applying it to data analyses (not all of which are in HEP). Parquet reading is supported. There are issues like not supporting partitioned datasets (#368), but I'm working through them. In fact, the thing I'm working on right now (#403) is adding a categorical type because generic IndexedArrays (in which "categories" are not necessarily unique) is hitting an upstream bug in Arrow (which assumes that they are unique). Declaring categorical types explicitly will make it clear which things I should translate to and from Parquet dictionaries—so this is second-order stuff. To first-order, it works. |
@jpivarski This sounds very encouraging. Thank you! |
Would like to test these two changes.