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

Plan for dropping/deprecating submodules of py and releasing v2.0 #288

Open
The-Compiler opened this issue Oct 19, 2022 · 30 comments
Open

Comments

@The-Compiler
Copy link
Member

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 the py dependency in pytest.
  2. Or dropping big parts of py (anything where we can't seem to find anything significant usage at the least, or perhaps even anything except py.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 own pytest.raises, which would probably be better off importing from pytest._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 use rich 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 perhaps TerminalWriter). 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 to pytest._code
  • py.error: Keep, useful for py.path
  • py.std: Drop, long deprecated and moving away is trivial
  • py.process: Drop (or possibly deprecate?)
  • py.apipkg: Drop / make private
  • py.iniconfig: Probably deprecate until tox moved away
  • py.path.local: Keep obviously
  • py.path.svnwc, py.path.svnurl, py.path.SvnAuth: Drop and kill it with fire
  • py.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?

@nicoddemus
Copy link
Member

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.

@The-Compiler
Copy link
Member Author

I see a couple of reasons:

  1. As evident in Snyk has identified a vulnerability in version 1.9.0 #265, Vulnerable Regular Expression in svnwc.py #256, ReDoS vulnerability in svnurl.py #287, ..., people with no knowledge of the code or context around it seem to like reporting security "issues" against py, probably just because it's a popular library thanks to pytest. This wastes time all around - the reporter's, the maintainer's, and our user's. It also might give pytest a reputation of depending on "insecure" libraries if people who read those announcements then don't understand the context.
  2. Dropping 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.
  3. Even once 2. is done, I suspect we plan to have a 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?
  4. As seen in the searches above, there unfortunately is somewhat widespread pylib usage in the wild. This includes things like pytest plugins, projects somewhere in pytest's heritage (tox, devpi, PyPy, etc.), but also random other projects. We might not like that, and we might think of pylib only being around for pytest, but that's just not how things look. The minimum we can (and have to, IMHO) do is making all those users aware of the project being dead, and in the case of bigger projects (e.g. tox), maybe even help them migrate away. The best way to make them aware is probably a new release with deprecation warnings.

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.

@RonnyPfannschmidt
Copy link
Member

Hmm, I'd rather just put pylib in unmaintained state

@The-Compiler
Copy link
Member Author

The-Compiler commented Oct 19, 2022

@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.

@RonnyPfannschmidt
Copy link
Member

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

@nicoddemus
Copy link
Member

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.

@RonnyPfannschmidt
Copy link
Member

@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

@RonnyPfannschmidt
Copy link
Member

my progression recommendation would be to

  1. deprecate anything not in use in pytest/key pytest plugins
  2. drop anything thats already extracted
  3. remove svnwc as its broken and unmainained to begin with
  4. completely deprecating pylib once pytest/key plugins are migrated off it

@asottile
Copy link
Member

we could probably get away with vendoring and a little sys.modules trickery to preserve the module paths

@RonnyPfannschmidt
Copy link
Member

i'd prefer to keep it outside the pytest codebase if feasible

@nicoddemus
Copy link
Member

@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.

@asottile
Copy link
Member

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

@The-Compiler
Copy link
Member Author

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.

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.

@RonnyPfannschmidt
Copy link
Member

@nicoddemus the drop would bascially jsut add stuff like iniconfig and apipkg back to the dependency lsit instead of keeping a vendored copy

@asottile
Copy link
Member

asottile commented Oct 19, 2022

I put together a proof of concept: pytest-dev/pytest#10396

@nicoddemus
Copy link
Member

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. 👍

@asottile
Copy link
Member

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 -- pytest-dev/pytest#10396

@RonnyPfannschmidt
Copy link
Member

If the hack @asottile created goes in, we can probably just move pylib to obsolete

@kapilt
Copy link

kapilt commented Oct 21, 2022

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

@asottile
Copy link
Member

yep, we'll be removing pytest-forked as a dependency (as slated in the deprecation messages for the xdist 3.0.0 release)

@nicoddemus
Copy link
Member

we'll be removing pytest-forked as a dependency

PR open: pytest-dev/pytest-xdist#821 👍

@The-Compiler
Copy link
Member Author

The-Compiler commented Oct 25, 2022

FWIW, outside of pytest + plugins, I expect the biggest consumers of pylib to be other ex-hpk42-and-friends projects:

@fschulze
Copy link

Regarding devpi:

  1. I replaced several uses with recommended replacements today (iniconfig, py.builtin etc)
  2. I'm unsure what to replace 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.
  3. Once devpi-client drops Python 2.7 support (when it becomes work to fix running the tests) I can replace TerminalWriter with a wrapper for rich or something like that in devpi-common.
  4. I will most likely vendor py.path.local into devpi-common, similar to pytest, but without the backward compatibility import stuff.
  5. py.xml.html is already gone in next major devpi-web release branch.
  6. I still have to investigate uses of py.error.E*, might be related to vendoring py.path.local.

@RonnyPfannschmidt
Copy link
Member

I strongly recommend against vendoring of local path as long term solution, pytest intents to drop it as soon as possible

tamuri added a commit to UCL/TLOmodel that referenced this issue Oct 31, 2022
…` lib

- from pytest 7.2.0 changelog: pytest no longer depends on the py library...If you need other py.* modules, continue to install the deprecated py library separately, otherwise it can usually be removed as a dependency.
- additional notes here pytest-dev/py#288
tmadlener added a commit to tmadlener/iLCInstall that referenced this issue Dec 7, 2022
Starting with pytest 7.2.0(?) py.test.raises only works if py is also
installed. Given pytest-dev/py#288 it is
probably easiest to just use pytest directly if possible
tmadlener added a commit to iLCSoft/iLCInstall that referenced this issue Dec 7, 2022
Starting with pytest 7.2.0(?) py.test.raises only works if py is also
installed. Given pytest-dev/py#288 it is
probably easiest to just use pytest directly if possible
@bphunter1972
Copy link

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.

@fschulze
Copy link

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

@bphunter1972
Copy link

@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.

@nicoddemus
Copy link
Member

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.

@fschulze
Copy link

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.

@nicoddemus
Copy link
Member

I see this reference in the code:

13: errno.EBUSY, # empty cd drive, but ENOMEDIUM seems unavailiable

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

No branches or pull requests

7 participants