confluent-kafka-python icon indicating copy to clipboard operation
confluent-kafka-python copied to clipboard

Published 20 hours ago • verified publisher icon confluentinc

setup.py: Add /usr/local for macos brew support

Open jayvdb opened this issue 3 years ago • 12 comments
trafficstars

jayvdb avatar Jul 04 '22 02:07 jayvdb

brew support of what? - can you explain a bit more the purpose

mhowlett avatar Jul 04 '22 22:07 mhowlett

Sure. This allows building with https://formulae.brew.sh/formula/librdkafka , which has headers and libs install into /usr/local

jayvdb avatar Jul 04 '22 23:07 jayvdb

we do binary wheels for python, right. why do you want to use the brew installed librdkafka? is it for M1 support?

mhowlett avatar Jul 05 '22 00:07 mhowlett

We do have one dev on an M1, and we had a requirement to switch to python 3.10 before the recent release which added wheels, so I had this patch so our CI (and me on a non-M1 mac) could easily build on mac before https://github.com/confluentinc/confluent-kafka-python/issues/1219 was resolved recently.

The instructions at https://github.com/confluentinc/confluent-kafka-python/blob/master/INSTALL.md#install-from-source-on-mac-os-x dont work without this patch.

jayvdb avatar Jul 05 '22 01:07 jayvdb

i wonder if this can it be done with env variables?

[ i'm not an expert here as you can tell from my n00b questions :-), so wary of screwing something up ]

mhowlett avatar Jul 05 '22 01:07 mhowlett

It can be done with env variables, but that then depends on whether other tools are all going to play nice. e.g. the package manager like flit/poetry/pipenv/pdm/etc, and pip.

It is also possible to create a custom site.cfg for a venv, e.g. https://github.com/numpy/numpy/blob/main/site.cfg.example

But they both require the user to know what they are doing.

A better approach is to use https://pypi.org/project/pkgconfig/ , which means it will find the correct paths to add for any platform, but then you need to add that as a setup_requires . I've done that a few times for packages using external C deps, so I don't mind doing that if you prefer it.

jayvdb avatar Jul 05 '22 01:07 jayvdb

If include_dirs (et.al) take precedence over standard paths then this PR introduces a problem with promoting /usr/local over system installed packages, which would be unexpected and undesired. I think it would be better to instruct people how to set proper env vars for non-standard source builds instead.

edenhill avatar Jul 05 '22 08:07 edenhill

What about if I adjusted this so it only adds /usr/local on macos, or explicitly adding [/usr, /usr/local] so that the precedence is clearly /usr is used first if possible.

jayvdb avatar Jul 05 '22 08:07 jayvdb

I'd rather we didn't alter these defaults path at all, but instead provide instructions for how to build with non-default include/library paths.

Also note that we'll soon be providing m1 wheels.

edenhill avatar Jul 05 '22 17:07 edenhill

@edenhill , /usr/local is the correct path on macos, based on the instructions at https://github.com/confluentinc/confluent-kafka-python/blob/master/INSTALL.md#install-from-source-on-mac-os-x . brew never installs into /usr , except for a few binary blob casks from non-OSS vendors that they can't control.

jayvdb avatar Jul 05 '22 23:07 jayvdb

It would be really helpful if this gets merged

ivanklimberg avatar Jul 21 '22 15:07 ivanklimberg

CLA assistant check
All committers have signed the CLA.

cla-assistant[bot] avatar Aug 15 '23 18:08 cla-assistant[bot]