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

Allow disabling of dependency installation in setup.py #1135

Merged
merged 17 commits into from
Mar 30, 2021

Conversation

vvilpas
Copy link
Contributor

@vvilpas vvilpas commented Feb 15, 2021

Summary

  • Some Python dependencies are installed outside regular setuptools installation because are required
    by compilation of C++ code. Now this installation can be disabled setting the environment variable
    _DISABLE_DEPENDENCY_INSTALL to 1 or "On".

  • This commit also adds version checks on those packages when setup.py checks if they are already available.

Details and comments

Fixes #1128

- Some Python dependencies are installed outside regular setuptools installation because are required
  by compilation of C++ code. Now this installation can be disabled setting the environment variable
  _DISABLE_DEPENDENCY_INSTALL to 1 or "On".

- This commit also adds version checks when setup.py checks if those packages are available.
@vvilpas vvilpas changed the title [WIP] Allow disabling of dependency installation in setup.py Allow disabling of dependency installation in setup.py Feb 23, 2021
mtreinish
mtreinish previously approved these changes Mar 9, 2021
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

This LGTM, it might be worth pointing out that newer pip versions shouldn't need this because of the pyproject.toml build requirements outlining these. But it comes up enough that people are calling setup.py directly or using an older pip version that this doesn't work.

My only hesitation here is the additional use of distutils which will be deprecated in python 3.10: https://www.python.org/dev/peps/pep-0632/ they specifically call out strtobool as something we need to implement ourselves (https://www.python.org/dev/peps/pep-0632/#migration-advice ). Maybe for future proofing we do that in a follow up and write our own strtobool

@vvilpas
Copy link
Contributor Author

vvilpas commented Mar 10, 2021

I've added our won strtobool function, mimicking distutils one.

Copy link
Member

@chriseclectic chriseclectic left a comment

Choose a reason for hiding this comment

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

Some suggestions for making the code a little easier to read

setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@chriseclectic chriseclectic added this to the Aer 0.8 milestone Mar 30, 2021
@chriseclectic chriseclectic merged commit 9450103 into Qiskit:master Mar 30, 2021
@mtreinish mtreinish added the Changelog: New Feature Include in the Added section of the changelog label Apr 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the Added section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow disabling of pip installations in setup.py
3 participants