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 to install the library on systems without compilation toolset #281

Merged
merged 13 commits into from Sep 7, 2018

Conversation

asvetlov
Copy link
Member

@asvetlov asvetlov commented Sep 6, 2018

Fixes #280

setup.py Outdated
@@ -24,6 +26,13 @@
except ImportError:
USE_CYTHON = False

if (here / '.git').exists() and not USE_CYTHON:
Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member

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

@webknjaz webknjaz self-assigned this Sep 6, 2018
@webknjaz
Copy link
Member

webknjaz commented Sep 6, 2018

Hold on merging this. There's a chicken-egg problem to solve ;)

@webknjaz
Copy link
Member

webknjaz commented Sep 6, 2018

Some linters don't need such dependency, but this change this.
Like dist setup check, which only requires docutils==0.14.
I think we'll need to have some switch for this.

@asvetlov
Copy link
Member Author

asvetlov commented Sep 6, 2018

Well, if you want to support the env var in git clone mode -- I'm ok with it.

@webknjaz
Copy link
Member

webknjaz commented Sep 6, 2018

@asvetlov can we reuse MULTIDICT_NO_EXTENSIONS? is it's meaning strictly defined somewhere? Like affecting the build- or run- time.

@asvetlov
Copy link
Member Author

asvetlov commented Sep 6, 2018

I think reusing MULTIDICT_NO_EXTENSIONS is perfectly fine

@codecov
Copy link

codecov bot commented Sep 7, 2018

Codecov Report

Merging #281 into master will decrease coverage by 0.28%.
The diff coverage is n/a.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
multidict/__init__.py 87.5% <0%> (-12.5%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f07daa...a6b5589. Read the comment docs.

@webknjaz
Copy link
Member

webknjaz commented Sep 7, 2018

@asvetlov there's copy-paste from multidict._compat module in place now. Do you think we can just import that? (Of course, this would mean banning _compat from importing anything from project to avoid cyclic dependency hell)

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,
Copy link
Member

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):
    ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Please do it

@asvetlov
Copy link
Member Author

asvetlov commented Sep 7, 2018

I prefer don't import from a package in setup.py.
Copy-paste is fine for the case.

@webknjaz
Copy link
Member

webknjaz commented Sep 7, 2018

@asvetlov ready for review again

@asvetlov
Copy link
Member Author

asvetlov commented Sep 7, 2018

LGTM!

@asvetlov
Copy link
Member Author

asvetlov commented Sep 7, 2018

thank you

@webknjaz webknjaz merged commit 5206f5a into master Sep 7, 2018
@jstriebel
Copy link

Seems like this is not yet published on pypi. Could you please publish it there?

@asvetlov
Copy link
Member Author

Please wait for #284
I'd like to have fixed type hints in a new release

@asvetlov asvetlov deleted the fix-setup branch September 13, 2018 16:51
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.

None yet

3 participants