rodluger / starry

Tools for mapping stars and planets.
https://starry.readthedocs.io
MIT License
144 stars 33 forks source link

Better setup script #222

Closed rodluger closed 4 years ago

rodluger commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

dfm commented 4 years ago

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 commented 4 years ago

See also: https://github.com/pybind/python_example/issues/44

rodluger commented 4 years ago

Cool, thanks!