Skip to content
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

chore: only build _ext and kernels #2813

Merged
merged 4 commits into from
Nov 13, 2023
Merged

Conversation

agoose77
Copy link
Collaborator

@agoose77 agoose77 commented Nov 10, 2023

Right now, we install three libraries: _ext, libawkward, libawkward-cpu-kernels, and build some additional static variants.

We didn't change this when splitting out awkward-cpp, because I didn't want to rock the boat any more. Since becoming more well versed with CMake's nuances, I'm confident that we can remove the leftover baggage.

This PR keeps the separate libawkward and _ext libraries (because it doesn't appear trivial to merge them), but drops the static libraries.

Copy link

codecov bot commented Nov 10, 2023

Codecov Report

Merging #2813 (05ee4a2) into main (9d874a0) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

We now export some more symbols to support this
@agoose77 agoose77 force-pushed the agoose77/chore-build-single-lib branch from b463e10 to 7faabab Compare November 10, 2023 21:30
@agoose77
Copy link
Collaborator Author

@jpivarski this PR fails on Windows. The error message in the CI indicates that the copy-assignment constructor for GrowableBuffer is being called by std::vector, but this constructor is implicitly deleted if one defines a move-assignment constructor.

I'm not sure why it's suddenly an issue. It's possible that the CI toolchain bumped MSVC at the same time that I wrote this PR, but I doubt it; in this PR I also added an EXPORT_SYMBOL for the FromJsonObjectSchema class, which has std::vector<GrowableBuffer<T>> members. That seems like causative.

This raises the following questions for me:

  1. Why does GCC not complain if MSVC does?
  2. Why does the standard not imply that the V in std::vector<V> needs to be copy-assignable (and copy-constructible?)? It used to in C++11, but seems to relax the rule for C++17+ (see "Template parameters" in https://en.cppreference.com/w/cpp/container/vector).
  3. Is it reasonable to add a copy-assignment constructor to GrowableBuffer? It seems to me like we really don't want that.

I'll keep digging.

@agoose77
Copy link
Collaborator Author

Well, that (05ee4a2) worked! I'm not entirely clear on why this became an error once I exported this class, but it seems that the emission of the default copy-assignment constructor leads to the attempted copy-assignment of the GrowableBuffer.

Clearly, we don't want that, so 05ee4a2 deletes the top-level copy-assignment (rather than implementing it)

Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reducing the number of shared libraries to two (one is the Python module and the other is dlopened through ctypes) is ideal.

However, this PR doesn't change awkward-cpp/src/awkward_cpp/cpu_kernels.py to look for kernels in libawkward.so. It's odd that it passes tests: how does the kernel dispatch find the CPU kernels?

Are the tests picking up a cached version of awkward-cpp?

awkward-cpp/include/awkward/io/json.h Show resolved Hide resolved
@agoose77 agoose77 merged commit 3bb4367 into main Nov 13, 2023
66 of 71 checks passed
@agoose77 agoose77 deleted the agoose77/chore-build-single-lib branch November 13, 2023 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants