Open The-Compiler opened 1 year ago
Hi @The-Compiler,
Thanks a lot for the extensive research and detailed proposal!
Let me play devils advocate though: why the need to do this at all? I mean, another option is to just wait until we drop py.path
from pytest, then officially archive the repository, without doing a new major release with removed modules, deprecation, etc.
I see a couple of reasons:
py.path
from pytest will still take a while, with how entangled it is in the entire API. We've done a lot of progress, but sadly we aren't quite there yet. It now happened 2 or 3 times that people spent a lot of time worrying about code nobody should worry about. I have no doubts it's going to happen again until we actually manage to drop py.path
. By removing code nobody is using, we reduce the likelyhood of that happening.pytest-legacy-path
plugin or so. Given the ubiquity of tmpdir
, I suspect it won't be too unpopular. It will need to either depend on pylib still, or vendor py.path
, or try to provide a pathlib-based compatibility layer, but with py.path
's API. If it depends on pylib, I'm not entirely sure if we want to archive it. If it vendors it, why not do that now?All in all, I feel like we need to have a proper deprecation plan for pylib before we get fully rid of it, whether we want to or not. I think dropping some ~unused code and deprecating some lightly used one would be a big first step towards that.
Hmm, I'd rather just put pylib in unmaintained state
@RonnyPfannschmidt I don't realistically see that happen before we drop it from pytest (as until then, it likely won't prevent such situations from repeating themselves again, thus declaring pytest as "unsafe", whether we like it or not). It also seems like a questionable move without a good way to make projects like tox or devpi aware of it. One of the best tools we have to do that is deprecation warnings.
In that case, apipkg has a own project, iniconfig as well, and time to drom/alias /deprecate
As for py. Io, I'd like to eventually separate it together with terminalwriter and pytests io capture, but the time horizon for that is too large
I see, thanks @The-Compiler. Those are compelling arguments.
I think the way to go is to vendor py.path
into pytest as you suggest, which allows pytest to drop the dependency to py
entirely. We cannot deprecate py.path.local
anytime soon, as old versions of pytest still use it.
When the time comes for pytest to drop py.local
support, the vendored code can be moved to the pytest-legacy-path
repository as well.
About the other libraries, I don't see much point in dropping any of them, we should just deprecate everything except py.path.local
and py.error
: dropping would make sense if we ever intend for py
to move forward without those libraries, but that is not the case, we plan to drop py
entirely (archiving the repository) in the future.
@nicoddemus based on the cost of py.code vendoring and the to expect breakages of the api location changes i'm strictly opposed to vendoring py.path.local , id much rather see us move the support code into a temporaryly my plugin that will eventually turn a extra dependency and finally be phased out
my progression recommendation would be to
we could probably get away with vendoring and a little sys.modules
trickery to preserve the module paths
i'd prefer to keep it outside the pytest codebase if feasible
@nicoddemus based on the cost of py.code vendoring and the to expect breakages of the api location changes i'm strictly opposed to vendoring py.path.local
I don't think there's much cost involved in vendoring py.path
, specially that with it we gain dropping py
from pytest, but I don't mind it much, and given you are strictly opposed, I won't argue in favor.
my progression recommendation would be to
I agree, except with "dropping" anything: we really won't gain much by dropping stuff, given that we will just archive py
in the future: dropping modules will just break working code needlessly, forcing some user to pin to an old version. But I'm in favor of deprecating everything of course (except py.path
and py.error
, as mentioned before), possibly mentioning alternatives that were extracted.
maybe I'm overlooking things, but I think we could do something like:
# pytest/py/__init__.py
try:
import py.path
except ImportError:
sys.modules['py'] = sys.modules[__name__]
else:
warnings.warn(FutureWarning, 'py lib is deprecated, please remove the dependency and use `pytest.py.path` instead, or better yet pathlib blah blah')
sys.modules['pytest.py'] = py
I agree, except with "dropping" anything: we really won't gain much by dropping stuff, given that we will just archive
py
in the future: dropping modules will just break working code needlessly, forcing some user to pin to an old version. But I'm in favor of deprecating everything of course (exceptpy.path
andpy.error
, as mentioned before), possibly mentioning alternatives that were extracted.
What we will gain is killing the noise of "security" advisories against pytest. This won't go away by deprecating things (we'd still be forced to fix it if we want people to not see security alerts for pytest). It will only go away by either dropping the py
dependency from pytest (i.e. vendoring py.path
), or by actually removing code nobody cares about, except apparently people looking for their next shiny CVE.
If we don't do either one or the other, the problem prompting me to open this issue won't be solved.
@nicoddemus the drop would bascially jsut add stuff like iniconfig and apipkg back to the dependency lsit instead of keeping a vendored copy
I put together a proof of concept: https://github.com/pytest-dev/pytest/pull/10396
What we will gain is killing the noise of "security" advisories against pytest. This won't go away by deprecating things (we'd still be forced to fix it if we want people to not see security alerts for pytest). It will only go away by either dropping the py dependency from pytest (i.e. vendoring py.path), or by actually removing code nobody cares about, except apparently people looking for their next shiny CVE.
OK, I agree. :+1:
ok so I've basically got the tests passing minus flake8 / mypy complaining -- the vendor hack actually doesn't seem that terrible? (it'll use py
if installed, otherwise it'll use the vendored copy) -- this preserves the import paths -- https://github.com/pytest-dev/pytest/pull/10396
If the hack @asottile created goes in, we can probably just move pylib to obsolete
re py.process - pytest-xdist depends pytest-forked which does use the module https://github.com/pytest-dev/pytest-forked/blob/master/src/pytest_forked/__init__.py#L73
yep, we'll be removing pytest-forked
as a dependency (as slated in the deprecation messages for the xdist 3.0.0 release)
we'll be removing pytest-forked as a dependency
PR open: https://github.com/pytest-dev/pytest-xdist/pull/821 👍
FWIW, outside of pytest + plugins, I expect the biggest consumers of pylib to be other ex-hpk42-and-friends projects:
Regarding devpi:
iniconfig
, py.builtin
etc)py.io.StdCaptureFD
with. It is only used for tests, so I guess something (maybe internal) from pytest, but I'm unsure which of the capture stuff would be the best match. For subprocess calls, I can most likely replace it with check_output
, but there are other uses which would be harded. It is used in fixtures only afaict.TerminalWriter
with a wrapper for rich
or something like that in devpi-common
.py.path.local
into devpi-common
, similar to pytest, but without the backward compatibility import stuff.devpi-web
release branch.py.error.E*
, might be related to vendoring py.path.local
.I strongly recommend against vendoring of local path as long term solution, pytest intents to drop it as soon as possible
FWIW, I use py.io.StdCaptureFD to capture output from a C process launched via pybind11 which may hang. I've found nothing else that could replace it, but I'm open to exploring alternatives.
I'm willing to be wrong, but I don't think rich has anything like it.
As replacement for StdCaptureFD I'm now using this:
from _pytest import capture
cap = capture.MultiCapture(
in_=capture.FDCapture(0),
out=capture.FDCapture(1),
err=capture.FDCapture(2))
@fschulze That's all well and good if you happen to be using this for the purposes of pytest. I'd prefer a library having nothing to do with testing to capture output. That's what py.io provided.
I personally do not know anything that implements StdCapture
indeed.
An option might be for someone to fork py.io
into its own library and maintain that separately; a smaller and more focused library might be an interesting little side project.
Can anyone tell me what py.error.EBUSY
is for and what exception is equivalent in Python? It seems to be Windows only and there is capture of it in devpi-server.
I see this reference in the code:
https://github.com/pytest-dev/py/blob/6b219734fcd8d2f6489c2f50585a435b34c029c2/py/_error.py#L28
Prompted by #287, I'd propose to drop big parts of pylib in a (hopefully more or less final) py 2.0 release.
Clearly nobody is interested in maintaining pylib, the current situation seems to be that a couple of pytest maintainers keep it on life-support. While we already did a lot of work to get rid of it in pytest, this is still an ongoing process.
However, pytest only uses
py.path
.Everything else in pylib is being used by an small number of projects. Some of the submodules are both high-risk and almost zero-usage (
py.path.svn*
, looking at you!). Others have some real-world usage, but at some point, we should probably start deprecating those modules at least, so people can start looking for alternatives (given that pylib probably will cease to be maintained entirely after pytest is split off).Thus, my proposal would be:
1) Either vendoring
py.path
in pytest (probably going to open a separate issue in the pytest repo for this), and then deprecating pylib in it's entirety, thus archiving this repository after dropping thepy
dependency inpytest
. 2) Or dropping big parts ofpy
(anything where we can't seem to find anything significant usage at the least, or perhaps even anything exceptpy.path
). This is what this issue will be about.I used Sourcegraph to try and get an overview over how the remaining parts of pylib are used. Here's what I found:
py.code
Integrated into pytest with pytest 2.9.0 back in early 2016.
There's a handful of remaining real-world usage (~30 results). The most striking one is probably Intellij/PyDev monkeypatching it, but that does already account for the new
_pytest._code
location anyways. Other than that I can see around 10 projects using it. Only 2 (schevo, pymtl3) seem to be outside of testsuites. The rest are testsuites doing things like their ownpytest.raises
, which would probably be better off importing frompytest._code
instead.My verdict: +0.5 for dropping
py.error
Used in a couple of places (166 results), mostly out of pytest. From what I can see, mostly used in conjunction with
py.path
.Superseded by more fine-grained
OSError
exceptions in Python 3. Sadly,py.path.local
raises its own exceptions rather than those...My verdict: -0.5 for dropping, should be kept as long as we keep
py.path
around for pytest. Thankfully, it's comparatively small and simple.py.std
Deprecated in 2017, still some usage, but from what I can tell, mostly in some old PyPy forks or something?
My verdict: Maybe needs some more investigation, but +1 for dropping, given it was deprecated before.
py.process
Imported in a couple of places. Funnily enough, the 3 or 4 projects I actually checked imported it, but never used it...
Maybe this happened due to some "automatic import" IDE feature for typo'd variables?
My verdict: +1 for dropping
py.apipkg
Zero usage. Probably needs to be kept for pylib itself, but at least not exposed publicly.
py.iniconfig
Split into separate project for pytest 6 in 2020.
Still used in tox unfortunately, 1 other usage in tox-factor.
My verdict: Probably wouldn't drop it right now, just because it's used in tox. We should coordinate with them to switch over to the split-off
iniconfig
, however. Maybe also find a way to raise deprecation warnings.py.path.local
Obviously keep. The only reason pylib is still around really...
py.path.svnwc
,py.path.svnurl
,py.path.SvnAuth
The whole reason I'm opening this issue, see #287 and #256. Complex, high-risk, and just not worth any of the trouble.
Also, unused outside of what seems like some PyPy development scripts, which are probably unused themselves, given that PyPy switched to Mercurial 12 years ago.
My verdict: Burn it with fire.
py.builtin
Used in a surprisingly high number of places, including devpi and pytest plugins.
Probably mostly from "Python 2 support" times, but at the same time, it seems to be a lot of code churn for a rather simple module
My verdict: -0.5 for removing it. Maybe find a way to raise deprecation warnings first.
py.io
Mostly used for
TerminalWriter
in various projects from what it seems.My verdict: -1 for removing it, as migration is probably more painful than e.g. for
py.builtin
. Might want to raise deprecation warnings telling people to userich
or something.py.xml
Apparently used in various projects, especially for HTML generation.
While it's high-risk, it's also arguably the most-used part of pylib (other than
py.path.local
and perhapsTerminalWriter
). Maybe raise deprecation warnings? What alternatives are there?py.log
Used in pypy and xdist from what I can see.
As above: Probably deprecate, but do not remove.
That leaves us with:
py.code
: Drop, affected pytest monkeypatching code can switch topytest._code
py.error
: Keep, useful forpy.path
py.std
: Drop, long deprecated and moving away is trivialpy.process
: Drop (or possibly deprecate?)py.apipkg
: Drop / make privatepy.iniconfig
: Probably deprecate until tox moved awaypy.path.local
: Keep obviouslypy.path.svnwc
,py.path.svnurl
,py.path.SvnAuth
: Drop and kill it with firepy.builtin
: Probably keep, maybe deprecate?py.io
: Probably keep, maybe deprecate?py.xml
: Probably keep, maybe deprecate?py.log
: Probably keep, maybe deprecate?@RonnyPfannschmidt @bluetech @nicoddemus what are your thoughts?