Skip to content

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

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: master
Choose a base branch
from

Conversation

jayvdb
Copy link
Contributor

@jayvdb jayvdb commented Jul 4, 2022

No description provided.

@jayvdb jayvdb requested a review from a team as a code owner July 4, 2022 02:46
@mhowlett
Copy link
Contributor

mhowlett commented Jul 4, 2022

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

@jayvdb
Copy link
Contributor Author

jayvdb commented Jul 4, 2022

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

@mhowlett
Copy link
Contributor

mhowlett commented Jul 5, 2022

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

@jayvdb
Copy link
Contributor Author

jayvdb commented Jul 5, 2022

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 #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.

@mhowlett
Copy link
Contributor

mhowlett commented Jul 5, 2022

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 ]

@jayvdb
Copy link
Contributor Author

jayvdb commented Jul 5, 2022

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.

@edenhill
Copy link
Contributor

edenhill commented Jul 5, 2022

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.

@jayvdb
Copy link
Contributor Author

jayvdb commented Jul 5, 2022

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.

@edenhill
Copy link
Contributor

edenhill commented Jul 5, 2022

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.

@jayvdb
Copy link
Contributor Author

jayvdb commented Jul 5, 2022

@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.

@ivanklimberg
Copy link

It would be really helpful if this gets merged

Comment on lines +47 to +48
include_dirs=['/usr/local/include/'],
library_dirs=['/usr/local/lib'],

Choose a reason for hiding this comment

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

this isn't portable (especially for M1 where brew's librdkafka will be in /opt and not /usr/local)

you can usually utilize CPATH="$(brew --prefix librdkafka)/include" LDFLAGS="-L$(brew --prefix librdkafka)/lib" ... to build a wheel

@cla-assistant
Copy link

cla-assistant bot commented Aug 15, 2023

CLA assistant check
All committers have signed the CLA.

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.

5 participants