-
-
Notifications
You must be signed in to change notification settings - Fork 525
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
[WIP] wheel support (and transitively PEP-518), plus pipe display failed commands #852
Conversation
Codecov Report
@@ Coverage Diff @@
## master #852 +/- ##
======================================
- Coverage 93% 92% -<1%
======================================
Files 12 12
Lines 2326 2344 +18
Branches 408 413 +5
======================================
+ Hits 2153 2164 +11
- Misses 107 110 +3
- Partials 66 70 +4 |
9eae7f8
to
b19f7c1
Compare
To be still added is deprecation messages for using the |
doc/config.rst
Outdated
tox will do for the projects package: | ||
|
||
- if it's ``none`` tox will not try to build the package (implies no install happens later), | ||
- if it's ``sdist`` it will create a source distribution by using ``distutils``: |
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'd just say "using setup.py sdist
", since it's way more likely to go through setuptools
anyway
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.
👍
doc/config.rst
Outdated
|
||
.. code-block:: ini | ||
|
||
pip build wheel --no-dep |
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 believe you mean pip wheel . --no-deps
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.
another note, it might be better (even in the wheel case) to:
- build an sdist
- build a wheel from the sdist
part of the benefit of tox using the
- build sdist
- install from sdist
ensures that the package is installable (and not depending on workdir state) -- would be good to keep that assertion going forward even in the "install from wheel" case
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.
@asottile what you're describing there is what pip plans to do part of pip wheel
command; so we already have that ensured. I would prefer for us to not re-implement the build infrastructure and leave that to the build fronted, e.g. pip
.
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.
this would be a step backward until pip 10 has majority adoption and pep518 is actually implemented
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.
We don't have wheel support at the moment. So that's a step ahead. If we implement pep-518 in tox that would be totally redundant over implementing in pip. So I argue if we add this support let's do it in pip so everyone benefits. Inside tox you can upgrade pip, so I don't understand why you would need pip 10 majority adoption. You already get it with latest virtualenv which is installed even with tox 2.x automatically.
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.
The main point I'm making here is:
- tox is a tool to prevent incorrect packaging
- unless you're using pip 10 and pep518, building a wheel from the working directory can bypass some checks that tox currently enforces via the "make sdist then install from it" process (due to lack of workdir isolation)
imo we should either:
- require pip10 + pep518 support to enable
build=wheel
sdist => wheel => install
Here's an example project which tox would prevent today, but this PR would incorrecty allow with build=wheel
:
status quo, correctly failing
$ tail -n999 setup.py tox.ini requirements.txt
==> setup.py <==
from setuptools import setup
# oops I goofed, should have made a MANIFEST.in
with open('requirements.txt') as f:
contents = list(f)
setup(
name='mylib',
install_requires=contents,
)
==> tox.ini <==
[testenv]
commands =
pip freeze --all
==> requirements.txt <==
six
$ tox -e py27
GLOB sdist-make: /private/tmp/g/setup.py
py27 create: /private/tmp/g/.tox/py27
py27 inst: /private/tmp/g/.tox/dist/mylib-0.0.0.zip
ERROR: invocation failed (exit code 1), logfile: /private/tmp/g/.tox/py27/log/py27-1.log
ERROR: actionid: py27
msg: installpkg
cmdargs: ['/private/tmp/g/.tox/py27/bin/pip', 'install', '/private/tmp/g/.tox/dist/mylib-0.0.0.zip']
Processing ./.tox/dist/mylib-0.0.0.zip
Complete output from command python setup.py egg_info:
Traceback (most recent call last):
File "<string>", line 1, in <module>
File "/private/tmp/pip-req-build-CCk4wP/setup.py", line 3, in <module>
with open('requirements.txt') as f:
IOError: [Errno 2] No such file or directory: 'requirements.txt'
----------------------------------------
Command "python setup.py egg_info" failed with error code 1 in /private/tmp/pip-req-build-CCk4wP/
___________________________________ summary ____________________________________
ERROR: py27: InvocationError for command /private/tmp/g/.tox/py27/bin/pip install /private/tmp/g/.tox/dist/mylib-0.0.0.zip (see /private/tmp/g/.tox/py27/log/py27-1.log) (exited with code 1)
your branch, incorrectly succeeding
[tox]
build = wheel
[testenv]
commands =
pip freeze --all
$ tox -e py36
GLOB wheel-make: /private/tmp/g/setup.py
py36 create: /private/tmp/g/.tox/py36
py36 inst: /private/tmp/g/.tox/dist/mylib-0.0.0-py3-none-any.whl
py36 installed: mylib==0.0.0,six==1.11.0
py36 runtests: PYTHONHASHSEED='1061640697'
py36 runtests: commands[0] | pip freeze --all
mylib==0.0.0
pip==10.0.1
setuptools==39.2.0
six==1.11.0
wheel==0.31.1
___________________________________ summary ____________________________________
py36: commands succeeded
congratulations :)
your branch, unrelated also broken thing
(noticed this while making a reproduction) -- tox
's python is python3, but venvs is python2 so there's a wheel mismatch
(note that this is covered by my comment below about the open issue to use the virtualenv python instead of tox
's python)
$ tox -e py27
GLOB wheel-make: /private/tmp/g/setup.py
py27 inst-nodeps: /private/tmp/g/.tox/dist/mylib-0.0.0-py3-none-any.whl
ERROR: invocation failed (exit code 1), logfile: /private/tmp/g/.tox/py27/log/py27-5.log
ERROR: actionid: py27
msg: installpkg
cmdargs: '/private/tmp/g/.tox/py27/bin/pip install -U --no-deps /private/tmp/g/.tox/dist/mylib-0.0.0-py3-none-any.whl'
mylib-0.0.0-py3-none-any.whl is not a supported wheel on this platform.
___________________________________ summary ____________________________________
ERROR: py27: InvocationError for command /private/tmp/g/.tox/py27/bin/pip install -U --no-deps /private/tmp/g/.tox/dist/mylib-0.0.0-py3-none-any.whl (see /private/tmp/g/.tox/py27/log/py27-5.log) (exited with code 1)
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.
This branch wants to implement require pip10 + pep518 support to enable. Will check how come those succeed and alter the code/create pr for pip to fail. thanks!
cwd=self.config.setupdir, | ||
) | ||
else: | ||
raise RuntimeError("invalid build type {}".format(self.config.build)) |
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.
there's an open issue to change these to instead use the virtualenv python (instead of tox
's python) to run these builds -- maybe worth changing in this review since it's already being touched?
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.
Again the creation of a build venv is the responsibility of pip, and in case of the wheel it already does that. For doing the same for sdist
the solution will be to move to pip build sdist
and we'll support implementing PEP-517 inside pip. tox will not implement what you're describing as it's outside the scope of the tool.
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'm sorry for breaking into your conversation but I have one argument to invoke pip wheel
with virtualenv's python - packages with C extensions. They depend on Python version with which they are built and afaiu pip will build wheels only for interpeter it's being run by, so such wheels built by tox's interpreter will not suit environment with another one.
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.
That’s a valid point. Maybe tox should run pip wheel multiple times for each python version, when it isn’t a universal pure python wheel.
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.
For the first phase we decided to focus on universal wheel. Hence the way this works now. it's still a wip though and likely will wait for pip 517 to merge to adopt this because of some sanity checks not applied by pip at the moment.
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.
Makes sense, thanks for the clarification
sys.executable, | ||
"-m", | ||
"pip", | ||
"wheel", |
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.
we could also invoke setup.py bdist_wheel
instead of involving pip -- though that drifts further from PEP518
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.
We plan to come closer to PEP-517/518 not move away from it. Once pip has that support we'll be able to provide flint, poetry etc support natively, without magical tox plugins.
# we clear the PYTHONPATH to allow reporting packages outside of this environment | ||
output = venv._pcall( | ||
args, cwd=venv.envconfig.config.toxinidir, action=action, no_python_path=True | ||
) |
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.
is this related to this change?
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.
It is. During testing this change I've encountered the situation of a faulty PYTHONPATH
can report packages outside the venv as installed during the build. This defends against such.
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.
hmmm, maybe split this into a separate PR so it's more clear why this changed?
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.
Felt linked against this pr to close as without it the tests would fail, so thought not worth the extra effort.
tox.ini
Outdated
@@ -102,7 +102,7 @@ commands = python -m pip list --format=columns | |||
[flake8] | |||
max-complexity = 22 | |||
max-line-length = 99 | |||
ignore = E203, W503 | |||
ignore = E203, W503, E501 |
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.
hmmm, is black not respecting the max line length?
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.
it does however flake8 falsely signals multiline strings as running long, see https://github.com/gaborbernat/tox/blob/pep-518/tests/test_package_build.py#L148
there is no way to break that, and given black already ensures source code does not flow over 100 I decided to skip it
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'd rather # noqa
the specific literal instead of blanket turning this off
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.
The problem is that flake complains not at assignment but middle of the string and one cannot add disable there. Will look to disable for that section then.
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.
if you put """ # noqa
(on the end of the tqs) that should work -- it's not super intuitive but 🤷♂️
…mmands PEP-518 support: provide a tox configuration flag ``build`` which can be either ``sdist`` or ``wheel``. For ``sdist`` (default) we build the package as before by using ``python setup.py sdist``. However, when ``wheel`` is enabled now we'll use ``pip wheel`` to build it, and we'll also install wheels in these case into the environments. Note: ``pip`` 10 supports specifying project dependencies (such as ``setuptools-scm``, or a given ``setuptools`` version) via ``pyproject.toml``. Once ``pip`` supports building ``sdist`` to we'll migrate over the ``sdist`` build too. While running tox invokes various commands (such as building the package, pip installing dependencies and so on), these were printed in case they failed as Python arrays. Changed the representation to a shell command, allowing the users to quickly replicate/debug the failure on their own.
c375365
to
83c1a03
Compare
did we actually want to merge this? I haven't had a chance to look at it since last time and I don't think anyone else has looked at it either :S |
Yeah, my bad. Removed it, will open it as a new PR. Sorry! |
Hello. I found this PR looking for ways to build wheel and test it with tox. This PR is shown as merged but I cannot find the documentation added here in https://tox.readthedocs.io/en/latest/config.html . I am wondering what happened to this option since. Thanks. |
Sadly we ran into issues with this and had to revert. tox 4 first alpha came out last week, and that contains this feature, though first public release not expected before April. |
#850 PEP-518 support: provide a tox configuration flag
build
which can be eithersdist
orwheel
. Forsdist
(default) we build the package as before by usingpython setup.py sdist
. However, whenwheel
is enabled now we'll usepip wheel
to build it, and we'll also install wheels in these case into the environments. Note:pip
10 supports specifying project dependencies (such assetuptools-scm
, or a givensetuptools
version) viapyproject.toml
. Oncepip
supports buildingsdist
to we'll migrate over thesdist
build too.#851 While running tox invokes various commands (such as building the package, pip installing dependencies and so on), these were printed in case they failed as Python arrays. Changed the representation to a shell command, allowing the users to quickly replicate/debug the failure on their own.