Skip to content
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

Better setup script #222

Closed
rodluger opened this issue Dec 9, 2019 · 6 comments
Closed

Better setup script #222

rodluger opened this issue Dec 9, 2019 · 6 comments

Comments

@rodluger
Copy link
Owner

rodluger commented Dec 9, 2019

We need a better way of ensuring pybind11 is installed before any of the C code is compiled. For some reason placing pybind11 in setup_requires is not sufficient, and the code reaches the import pybind11 line (when resolving the include paths) before it's actually installed. The suggested workaround here is to use subprocess to pip install pybind11, which is horribly hacky. @dfm, any ideas?

@dfm
Copy link
Collaborator

dfm commented Dec 11, 2019

You definitely don't want to do that!!!!!11!!!1!!1!

There are a few issues with the current setup (e.g. setuptools_scm version 3.4 doesn't exist yet so that dep can't be satisfied) but otherwise the pip install works just fine for me even in a fresh environment without pybind11 preinstalled. Here's what I did:

conda create -p ./env python=3.7 pip
conda activate ./env
which python
python -m pip install -e .

This runs fine after applying the following diff:

diff --git a/pyproject.toml b/pyproject.toml
index 4280939a..f8db24fe 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -1,5 +1,5 @@
 [build-system]
-requires = ["setuptools>=42", "wheel", "numpy", "pybind11", "setuptools_scm[toml]>=3.4"]
+requires = ["setuptools>=42", "wheel", "numpy", "pybind11", "setuptools_scm[toml]>=3.3"]
 build-backend = "setuptools.build_meta"

 [tool.black]
diff --git a/setup.py b/setup.py
index 43945e7b..a1882897 100644
--- a/setup.py
+++ b/setup.py
@@ -132,7 +132,7 @@ class BuildExt(build_ext):
     l_opts = {"msvc": [], "unix": []}

     if sys.platform == "darwin":
-        darwin_opts = ["-stdlib=libc++", "-mmacosx-version-min=10.7"]
+        darwin_opts = ["-stdlib=libc++", "-mmacosx-version-min=10.14"]
         c_opts["unix"] += darwin_opts
         l_opts["unix"] += darwin_opts

@rodluger
Copy link
Owner Author

Haha, I figured you'd cringe a little at that. OK, I confirm that it works. Why did you bump mmacosx-version-min to Mojave?

@dfm
Copy link
Collaborator

dfm commented Dec 11, 2019

The conda compiler crashes if you only have 10.7. I'm not sure what number is required though!

@dfm
Copy link
Collaborator

dfm commented Dec 11, 2019

We could use platform.mac_ver to detect the version. My machine seems to require -mmacosx-version-min=10.14 (it has something to do with the C++ headers that get used).

@dfm
Copy link
Collaborator

dfm commented Dec 11, 2019

See also: pybind/python_example#44

@rodluger
Copy link
Owner Author

Cool, thanks!

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

No branches or pull requests

2 participants