Skip to content

Simplify blosc build configuration by turning it into an object library #5507

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rroelke
Copy link
Member

@rroelke rroelke commented Apr 29, 2025

The external blosc library is used in the byte shuffle filter. Our build system contorted itself a bit to compile this into TILEDB_CORE_OBJECTS as well as the unit test. As this pull request shows there is a much simpler path.

While I no longer have need of the reason I implemented this, it is nonetheless a simplification of the build process, so I figured I'd post a PR for it.


TYPE: NO_HISTORY
DESC: Simplify blosc build

@rroelke rroelke force-pushed the rr/blosc-object-library branch from cfa2e58 to bd2cb5f Compare April 29, 2025 15:21
Copy link
Contributor

@davisp davisp left a comment

Choose a reason for hiding this comment

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

Looks good overall except you need to go through and add the blosc object library dependency to all the tests that need it. I'm pretty sure my 65154 branch has those listed out. Alternatively, the way I fixed those was to just run make -j$(nproc) tests && VERBOSE=1 make tests over and over fixing each link error until the build succeeded.

@davisp
Copy link
Contributor

davisp commented Apr 29, 2025

Also, I'll note that given we don't really need to make this change, it might not really be worth the churn. Though I do agree it does simplify things which is always nice.

@rroelke
Copy link
Member Author

rroelke commented Apr 29, 2025

Also, I'll note that given we don't really need to make this change, it might not really be worth the churn. Though I do agree it does simplify things which is always nice.

Given that we have to add it to all the unit test object library lists - which I didn't expect, I though that it would be enough to include it in the filter object library - I'm not sure this is worth doing anymore either. I might close it.

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