-
-
Notifications
You must be signed in to change notification settings - Fork 101
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 to install the library on systems without compilation toolset #281
Conversation
setup.py
Outdated
@@ -24,6 +26,13 @@ | |||
except ImportError: | |||
USE_CYTHON = False | |||
|
|||
if (here / '.git').exists() and not USE_CYTHON: |
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.
maybe we should also rely on MULTIDICT_NO_EXTENSIONS
?
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.
My strategy is:
- git clone is for development mode. Compilation toolset is required
- Users should install released versions from PyPI.
- Binary wheels are preferred, they don't execute
setup.py
code - If a binary wheel for the target platform is not available source tarball is used regardless to
MULTIDICT_NO_EXTENSIONS
- If the toolset is not available pure python installation is executed with a warning to stdout. Better to change stdout -> stderr as a minor improvement
Later I want to consider switching to pure python only if MULTIDICT_NO_EXTENSIONS
is set to prevent silent performance lose. It enforces something like MULTIDICT_NO_EXTENSIONS=1 pip install multidict
on machines without compilation toolset when a binary wheel is not available.
Thoughts?
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.
Sounds fine, I think. I've pushed minor adjustments. Plz review
Hold on merging this. There's a chicken-egg problem to solve ;) |
Some linters don't need such dependency, but this change this. |
Well, if you want to support the env var in |
@asvetlov can we reuse |
I think reusing |
Codecov Report
@@ Coverage Diff @@
## master #281 +/- ##
==========================================
- Coverage 99.71% 99.43% -0.29%
==========================================
Files 4 4
Lines 355 355
==========================================
- Hits 354 353 -1
- Misses 1 2 +1
Continue to review full report at Codecov.
|
@asvetlov there's copy-paste from |
setup.py
Outdated
@@ -74,7 +86,7 @@ def run(self): | |||
def build_extension(self, ext): | |||
try: | |||
build_ext.build_extension(self, ext) | |||
except (DistutilsExecError, | |||
except (CompilerError, DistutilsExecError, |
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.
Maybe:
extra_ignore_exc = () if USE_CYTHON_EXTENSIONS else (CompilerError, )
...
# and then
except (*extra_ignore_exc, DistutilsExecError, DistutilsPlatformError, ValueError):
...
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.
Please do it
I prefer don't import from a package in |
@asvetlov ready for review again |
LGTM! |
thank you |
Seems like this is not yet published on pypi. Could you please publish it there? |
Please wait for #284 |
Fixes #280