-
-
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
Changes from all commits
a0dbbd0
217e53c
336f231
fdfb046
ccb1beb
b9017cc
e3572a1
4dd236a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
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.`@gaborbernat <https://github.com/gaborbernat>`_ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
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.`@gaborbernat <https://github.com/gaborbernat>`_ |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,13 +7,15 @@ | |
from __future__ import print_function | ||
|
||
import os | ||
import pipes | ||
import re | ||
import shutil | ||
import subprocess | ||
import sys | ||
import time | ||
|
||
import py | ||
from pkg_resources import get_distribution | ||
|
||
import tox | ||
from tox._verlib import IrrationalVersionError, NormalizedVersion | ||
|
@@ -136,9 +138,16 @@ def _initlogpath(self, actionid): | |
def popen(self, args, cwd=None, env=None, redirect=True, returnout=False, ignore_ret=False): | ||
stdout = outpath = None | ||
resultjson = self.session.config.option.resultjson | ||
|
||
cmd_args = [str(x) for x in args] | ||
cmd_args_shell = " ".join(pipes.quote(i) for i in cmd_args) | ||
if resultjson or redirect: | ||
fout = self._initlogpath(self.id) | ||
fout.write("actionid: {}\nmsg: {}\ncmdargs: {!r}\n\n".format(self.id, self.msg, args)) | ||
fout.write( | ||
"actionid: {}\nmsg: {}\ncmdargs: {!r}\n\n".format( | ||
self.id, self.msg, cmd_args_shell | ||
) | ||
) | ||
fout.flush() | ||
outpath = py.path.local(fout.name) | ||
fin = outpath.open("rb") | ||
|
@@ -153,11 +162,13 @@ def popen(self, args, cwd=None, env=None, redirect=True, returnout=False, ignore | |
popen = self._popen(args, cwd, env=env, stdout=stdout, stderr=subprocess.STDOUT) | ||
except OSError as e: | ||
self.report.error( | ||
"invocation failed (errno {:d}), args: {}, cwd: {}".format(e.errno, args, cwd) | ||
"invocation failed (errno {:d}), args: {}, cwd: {}".format( | ||
e.errno, cmd_args_shell, cwd | ||
) | ||
) | ||
raise | ||
popen.outpath = outpath | ||
popen.args = [str(x) for x in args] | ||
popen.args = cmd_args | ||
popen.cwd = cwd | ||
popen.action = self | ||
self._popenlist.append(popen) | ||
|
@@ -431,7 +442,7 @@ def _copyfiles(self, srcdir, pathlist, destdir): | |
target.dirpath().ensure(dir=1) | ||
src.copy(target) | ||
|
||
def _makesdist(self): | ||
def _get_package(self): | ||
setup = self.config.setupdir.join("setup.py") | ||
if not setup.check(): | ||
self.report.error( | ||
|
@@ -446,19 +457,8 @@ def _makesdist(self): | |
) | ||
raise SystemExit(1) | ||
with self.newaction(None, "packaging") as action: | ||
action.setactivity("sdist-make", setup) | ||
self.make_emptydir(self.config.distdir) | ||
action.popen( | ||
[ | ||
sys.executable, | ||
setup, | ||
"sdist", | ||
"--formats=zip", | ||
"--dist-dir", | ||
self.config.distdir, | ||
], | ||
cwd=self.config.setupdir, | ||
) | ||
action.setactivity("{}-make".format(self.config.build), setup) | ||
self._make_package(action, setup) | ||
try: | ||
return self.config.distdir.listdir()[0] | ||
except py.error.ENOENT: | ||
|
@@ -478,6 +478,46 @@ def _makesdist(self): | |
) | ||
raise SystemExit(1) | ||
|
||
def _make_package(self, action, setup): | ||
self.make_emptydir(self.config.distdir) | ||
if self.config.build == "sdist": | ||
action.popen( | ||
[ | ||
sys.executable, | ||
setup, | ||
"sdist", | ||
"--formats=zip", | ||
"--dist-dir", | ||
self.config.distdir, | ||
], | ||
cwd=self.config.setupdir, | ||
) | ||
elif self.config.build == "wheel": | ||
if NormalizedVersion(get_distribution("pip").version).parts[0][0] > 10: | ||
raise RuntimeError("wheel support requires pip 10 or later") | ||
py_project_toml = self.config.setupdir.join("pyproject.toml") | ||
if not py_project_toml.exists(): | ||
raise RuntimeError( | ||
"wheel support requires creating and setting build-requires in {}".format( | ||
py_project_toml | ||
) | ||
) | ||
action.popen( | ||
[ | ||
sys.executable, | ||
"-m", | ||
"pip", | ||
"wheel", | ||
"-w", | ||
self.config.distdir, # target folder | ||
"--no-deps", # don't build wheels for package dependencies | ||
self.config.setupdir, # what to build - ourselves | ||
], | ||
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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense, thanks for the clarification |
||
|
||
def make_emptydir(self, path): | ||
if path.check(): | ||
self.report.info(" removing {}".format(path)) | ||
|
@@ -576,23 +616,23 @@ def get_installpkg_path(self): | |
if not path: | ||
path = self.config.sdistsrc | ||
path = self._resolve_pkg(path) | ||
self.report.info("using package {!r}, skipping 'sdist' activity ".format(str(path))) | ||
self.report.info( | ||
"using package {!r}, skipping '{}' activity ".format(str(path), self.config.build) | ||
) | ||
else: | ||
try: | ||
path = self._makesdist() | ||
path = self._get_package() | ||
except tox.exception.InvocationError: | ||
v = sys.exc_info()[1] | ||
self.report.error("FAIL could not package project - v = {!r}".format(v)) | ||
return | ||
sdistfile = self.config.distshare.join(path.basename) | ||
if sdistfile != path: | ||
self.report.info("copying new sdistfile to {!r}".format(str(sdistfile))) | ||
self.report.info("copying new package to {!r}".format(str(sdistfile))) | ||
try: | ||
sdistfile.dirpath().ensure(dir=1) | ||
except py.error.Error: | ||
self.report.warning( | ||
"could not copy distfile to {}".format(sdistfile.dirpath()) | ||
) | ||
self.report.warning("could not copy package to {}".format(sdistfile.dirpath())) | ||
else: | ||
path.copy(sdistfile) | ||
return path | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -403,7 +403,15 @@ def test(self, redirect=False): | |
raise | ||
|
||
def _pcall( | ||
self, args, cwd, venv=True, testcommand=False, action=None, redirect=True, ignore_ret=False | ||
self, | ||
args, | ||
cwd, | ||
venv=True, | ||
testcommand=False, | ||
action=None, | ||
redirect=True, | ||
ignore_ret=False, | ||
no_python_path=False, | ||
): | ||
os.environ.pop("VIRTUALENV_PYTHON", None) | ||
|
||
|
@@ -414,6 +422,8 @@ def _pcall( | |
env = self._getenv(testcommand=testcommand) | ||
bindir = str(self.envconfig.envbindir) | ||
env["PATH"] = p = os.pathsep.join([bindir, os.environ["PATH"]]) | ||
if no_python_path: | ||
env.pop("PYTHONPATH", None) | ||
self.session.report.verbosity2("setting PATH={}".format(p)) | ||
return action.popen(args, cwd=cwd, env=env, redirect=redirect, ignore_ret=ignore_ret) | ||
|
||
|
@@ -487,7 +497,10 @@ def tox_runtest(venv, redirect): | |
def tox_runenvreport(venv, action): | ||
# write out version dependency information | ||
args = venv.envconfig.list_dependencies_command | ||
output = venv._pcall(args, cwd=venv.envconfig.config.toxinidir, action=action) | ||
# 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 commentThe 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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. |
||
# the output contains a mime-header, skip it | ||
output = output.split("\n\n")[-1] | ||
packages = output.strip().split("\n") | ||
|
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 PEP518There 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.