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

Install setup requirements on-the-fly #101

Merged
merged 14 commits into from
Mar 11, 2021
Merged

Install setup requirements on-the-fly #101

merged 14 commits into from
Mar 11, 2021

Conversation

yannikschaelte
Copy link
Contributor

@yannikschaelte yannikschaelte commented Mar 11, 2021

Automatically install setup requirements during setup.py based installation. Concerns cython+numpy, i.e. pip dependencies. Apt dependencies will need to be installed separately.

Solves #100

.travis.yml Outdated
@@ -36,7 +36,7 @@ install:
# https://github.com/conda-forge/blas-feedstock/issues/58
- conda create -q -n test-env python=$TRAVIS_PYTHON_VERSION
- conda activate test-env
- conda install -y -q lapack "libblas=*=*netlib" cython>=0.26 future>=0.15 "ipopt=$IPOPT_VERSION" numpy>=1.15 pkg-config>=0.29.2 setuptools>=39.0 six>=1.11
- conda install -y -q lapack "libblas=*=*netlib" "ipopt=$IPOPT_VERSION" pkg-config>=0.29.2 setuptools>=39.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer leaving this as is so we pre-install all dependencies from conda. We could have a separate test that attempts to install using pip installed dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted it!

setup.py Outdated
from setuptools import dist
SETUP_REQUIRES = [
"setuptools >= 39.0",
"wheel >= 0.26.2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure why wheel should be a requirement for building the library. Is that only required if you want to create a wheel?

We have #41, but I believe that requires building a wheel for ipopt first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

afaik binary wheel is python's preferred way of installing packages (not sure how much faster though), therefore it warns that it uses e.g. source fallback when wheel is not installed. but also works without, removed it here.

"cython>=0.26",
"future>=0.15",
"setuptools>=39.0",
"six>=1.11",
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is likely that cython and setuptools are not needed to run the library. We'd need to double check.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think Cython could be removed here, but not necessarily numpy. Shouldn't setuptools be a build dep then too? I don't see it used anywhere but setup.py.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true ... I did not want to meddle too much with the build, but certainly possible that cython/numpy can be removed under install_requires (here I did not change those dependencies at all).

setuptools should technically be a build dep too, however I guess that would only work via pyproject.toml or setup.cfg, as setup.py is completely based on setuptools. afaik, it is usually pre-installed on python distros.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Understood. The file requires an import of setuptools itself, so it has to be pre-installed. We can leave this as is.

@moorepants
Copy link
Collaborator

Ok, thanks. Hopefully that helps a little downstream.

@moorepants moorepants merged commit b7fb1c9 into mechmotum:master Mar 11, 2021
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.

2 participants