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

Require cython when building from git clone only #3189

Merged
merged 16 commits into from
Aug 20, 2018
Merged

Require cython when building from git clone only #3189

merged 16 commits into from
Aug 20, 2018

Conversation

asvetlov
Copy link
Member

Fixes #3162

Supersedes #3164

setup.py Outdated
if (here / '.git').exists() and not (here / 'vendor/http-parser/README.md'):
print("Install submodules when building from git clone")
print("git submodule init")
print("git submodule update")
Copy link
Member

Choose a reason for hiding this comment

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

It's simpler to do git submodule update --init.

Copy link
Member

Choose a reason for hiding this comment

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

Also, how about to do that automagically if condition is met? Since we know which commands are need to run. Suddenly, it wouldn't be useful for Cython - setup_requires can't handle wheels /:

Copy link
Member Author

Choose a reason for hiding this comment

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

It is for dev module only.
I prefer don't hide git commands from user here.
There is a rare possibility to have no git command in PATH, better to give a hint only

Copy link
Member

Choose a reason for hiding this comment

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

Ok, that's fine for me.

setup.py Outdated

try:
from Cython.Build import cythonize
USE_CYTHON = True
except ImportError:
USE_CYTHON = False

if (here / '.git').exists() and not USE_CYTHON:
print("Install cython when building from git clone")
print("pip install cython")
Copy link
Member

Choose a reason for hiding this comment

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

Add this to pyproject.toml and it'll be autoinstalled for pip 10+ users.

setup.py Outdated

try:
from Cython.Build import cythonize
USE_CYTHON = True
except ImportError:
USE_CYTHON = False

if (here / '.git').exists() and not USE_CYTHON:
print("Install cython when building from git clone")
Copy link
Member

Choose a reason for hiding this comment

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

Please use stderr

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch

setup.py Outdated

if (here / '.git').exists() and not (here / 'vendor/http-parser/README.md'):
print("Install submodules when building from git clone")
print("git submodule update --init")
Copy link
Member

Choose a reason for hiding this comment

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

Try calling this from here it'd be smooth for users then.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't want to call/install anything, just generate a better error message

Copy link
Member

Choose a reason for hiding this comment

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

Ok, improve it by adding Please do so by running: prefix.

.travis.yml Outdated
@@ -152,12 +152,16 @@ jobs:
- <<: *_doc_base
env:
- TARGET=towncrier
install:
- pip install -r requirements/towncrier.txt
Copy link
Member

Choose a reason for hiding this comment

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

You're also overriding - *upgrade_python_toolset here.

@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3189      +/-   ##
==========================================
- Coverage   98.11%   98.07%   -0.04%     
==========================================
  Files          43       43              
  Lines        7853     7853              
  Branches     1354     1354              
==========================================
- Hits         7705     7702       -3     
- Misses         57       59       +2     
- Partials       91       92       +1
Impacted Files Coverage Δ
aiohttp/tcp_helpers.py 90% <0%> (-6.67%) ⬇️
aiohttp/client_reqrep.py 97.47% <0%> (-0.17%) ⬇️

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 d036e38...86479c3. Read the comment docs.

@asvetlov asvetlov merged commit 168e3f0 into master Aug 20, 2018
@asvetlov asvetlov deleted the relax-build branch August 20, 2018 20:08
@asvetlov asvetlov mentioned this pull request Aug 26, 2018
3 tasks
@Suida Suida mentioned this pull request Aug 16, 2019
5 tasks
@lock
Copy link

lock bot commented Oct 28, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build/install broken without cython
4 participants