Skip to content

Conversation

@bnavigator
Copy link
Contributor

This fixes #216, by introducing a USE_SYSTEM_BLOSC environment variable. Custom install locations are supported through CMake's standard settable Blosc_ROOT, Blosc_INCLUDE_DIR, etc. variables for finding modules.

For consistency with the standard CMake way, I decided against retaining the defunct BLOSC_DIR variable.

BTW, the build_clib and build_ext commands seems to do nothing on my installs, everything is performed by scikit-build during the build stage. Might have the same reason as #222.

@sreerajkksd
Copy link

@FrancescAlted @thewtex Can one of you review this PR ? Does this approach look sane to you guys ?

@thewtex
Copy link
Contributor

thewtex commented Jun 25, 2021

Hi @bnavigator @sreerajkksd @FrancescAlted I explained more in #216, but improvements could be made by using a CMake package configuration file.

@FrancescAlted
Copy link
Member

Any possibility that the PR adopts the suggestions by @thewtex in #212? Also, the PR has conflicts and needs an update.

@bnavigator
Copy link
Contributor Author

If I understand correctly, @thewtex suggests to create a CMake package file instead of this PRs FindBlosc.cmake. This would go into c-blosc, not python-blosc. Way out of scope of this PR.

Copy link
Member

@FrancescAlted FrancescAlted left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me (unless @thewtex would propose otherwise).

Copy link
Member

@FrancescAlted FrancescAlted left a comment

Choose a reason for hiding this comment

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

Let's keep this on-hold until/if @thewtex would chime in.

@FrancescAlted FrancescAlted self-requested a review August 3, 2021 18:00
@FrancescAlted FrancescAlted dismissed their stale review August 3, 2021 18:01

I have been convinced of the weird CMake name conventions

Copy link
Member

@FrancescAlted FrancescAlted left a comment

Choose a reason for hiding this comment

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

I have just a trivial comment

@thewtex
Copy link
Contributor

thewtex commented Aug 5, 2021

@thewtex suggests to create a CMake package file instead of this PRs FindBlosc.cmake

Yes, we should definitely be using a CMake package configuration file instead of a Find module. But, if @bnavigator is not willing to make those changes, this is still a step forward. We could merge this and make additional progress later.

@sreerajkksd
Copy link

I would recommend merging this now (as we have approval from the maintainers) and creating a new PR for the concern mentioned. I can see that @bnavigator already created an issue for this. I believe this is going to help some people (including me) to compile with an existing c-blosc library.

@FrancescAlted FrancescAlted merged commit e71322e into Blosc:master Aug 10, 2021
@sreerajkksd
Copy link

@FrancescAlted Can you cut a new release in Pypi ?

@mgorny
Copy link
Contributor

mgorny commented Oct 7, 2021

I'm confused now. So building python-blosc against the system library now requires c-blosc with a CMake package file that's not yet provided by c-blosc?

@bnavigator
Copy link
Contributor Author

python-blosc provides its own FindBlosc.cmake for the time being. All should be there.

@mgorny
Copy link
Contributor

mgorny commented Oct 7, 2021

I'm sorry, my bad. However, the FindBlosc module is missing from the pypi sdist. After using the raw git archive from GitHub, it works.

@eamanu
Copy link

eamanu commented May 7, 2022

HI,

I'm packing python-blosc 1.10.6 in Debian. I wanted to be sure if this PR won't
re-introduce the issue #227 and fixed here https://github.com/Blosc/python-blosc/pull/242/files.

Thanks!
Cheers,

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.

BLOSC_DIR environment variable seems defunct as of v1.8.3

6 participants