Conversation
staticlibs
force-pushed
the
windows_remove_jemalloc
branch
from
627f1de to
b5a90c2
Compare
The jemalloc extention was recently added to JDBC builds in duckdb#133. The logic for its selection in `vendor.py` does not look fully correct to me, but both Linux and Mac are passing on CI, only Windows is breaking, so this change only tries to unbreak Windows builds, and the jemalloc selection logic can be improved in subsequent PRs.
staticlibs
force-pushed
the
windows_remove_jemalloc
branch
from
b5a90c2 to
d30ec3b
Compare
Mytherin
left a comment
Mytherin
left a comment
There was a problem hiding this comment.
Thanks for the PR! Looks good - one comment
| add_definitions(-DDUCKDB_BUILD_LIBRARY) | ||
|
|
||
| if(MSVC) | ||
| remove_definitions(-DDUCKDB_EXTENSION_JEMALLOC_LINKED) |
There was a problem hiding this comment.
Instead of removing them can we perhaps not add them in the first place? That seems like it would be cleaner. Or is the above list of files auto-generated?
There was a problem hiding this comment.
Yes, these definitions and the sources list is auto-generated by vendor.py, regarding jemalloc it contains the following logic. I cannot make sense of it, because these platform checks are done when the vendor.py itself is being run (how is it supposed to check for Android and WASM at that time, why checking for WASM for JDBC sources etc - it was added in #133, I asked Laurens in Discord about it). I assume these checks for jemalloc need to be run at CMake configure time instead. And the list of sources needs to be added by vendor.py into a separate CMake variable. But for now, without touching vendor.py this "filter out on Windows" logic looks the easiest.
There was a problem hiding this comment.
That makes sense. It looks like these checks were copied from the Python build, which indeed doesn't work because the vendor script does not get run per configuration. The correct checks should be in the CMake. I would copy over the checks from here.
|
We can clean up the logic in a subsequent PR - thanks for the fix! |
|
Thanks for merging this! CI is passing now in main, I've rebased other PRs onto the latest main. |
This change is a follow-up to duckdb#162, it moves platform-specific jemalloc selection logic from `vendor.py` to CMake. Additionally CMake file is restructured and cleaned up. Testing: no functional changes, no new tests.
2 participants
The jemalloc extention was recently added to JDBC builds in #133. The logic for its selection in
vendor.pydoes not look fully correct to me, but both Linux and Mac are passing on CI, only Windows is breaking, so this change only tries to unbreak Windows builds, and the jemalloc selection logic can be improved in subsequent PRs.