-
Notifications
You must be signed in to change notification settings - Fork 44
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
setuptools min version #91
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Tested by first making a wheel in a python3 venv with setuptools 40
(env) $ cd colcon-core
(env) $ python setup.py bdist_wheel
...
(env) $ deactivate
Then in a new venv with setuptools 20.7.0
(renv) $ pip install src/colcon-core/dist/colcon_core-0.3.5-py3-none-any.whl
Processing ./src/colcon-core/dist/colcon_core-0.3.5-py3-none-any.whl
...
Collecting setuptools>=30.3.0 (from colcon-core==0.3.5)
Using cached https://files.pythonhosted.org/packages/...
...
(renv) $ python -c "import setuptools; print(setuptools.__version__)"
40.0.0
setuptools_version = get_distribution('setuptools').version | ||
minimum_version = '30.3.0' | ||
if parse_version(setuptools_version) < parse_version(minimum_version): | ||
sys.exit( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sys.exit()
is kind of harsh for an ImportError
, but it does make the message easy to read when trying to bootstrap without installing setuptools.
$ ./src/colcon-core/bin/colcon
'setuptools' needs to be at least version 30.3.0, if a newer version is not available from the package manager use 'pip3 install -U setuptools' to update to the latest version
I'm ok with it, but would prefer a raised ImportError
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I modified the patch to extend the raised ModuleNotFoundError
exception with additional information where applicable: 8a71269#diff-9fc4dc6d196841f3b396afba788b9246R18
The "downside" is that the build will continue, likely falling back to the python_setup_py
extension which fails for every package using a setup.cfg
file rather than declaring the package name in the setup.py
file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ModuleNotFoundError
is actually only available as of Pytohn 3.6. Therefore I updated the patch to catch the more generic exception ImportError
. That should make the CI with Python 3.5 happy too.
444e88e
to
8a71269
Compare
8a71269
to
fd683b5
Compare
Codecov Report
@@ Coverage Diff @@
## master #91 +/- ##
==========================================
- Coverage 78.25% 78.03% -0.22%
==========================================
Files 50 50
Lines 2768 2777 +9
Branches 462 463 +1
==========================================
+ Hits 2166 2167 +1
- Misses 580 588 +8
Partials 22 22
Continue to review full report at Codecov.
|
Fixes #90.
The first commit declares the minimum version of setuptools required. This only works when using
pip
to install the package. The version dependency can't be specified in thestdeb.cfg
file since some supported platforms like Ubuntu Xenial don't provide a new enough Debian package ofsetuptools
.The second commit will catch the case where the available
setuptools
version is too old and provide a custom error message describing the problem as well as a way to update the package viapip
.