Support sys.pycache_prefix on py38 #5864

nicoddemus · 2019-09-19T23:28:34Z

src/_pytest/assertion/rewrite.py

asottile

neat

asottile · 2019-09-23T16:29:17Z

src/_pytest/assertion/rewrite.py

@@ -102,7 +104,7 @@ def create_module(self, spec):
        return None  # default behaviour is fine

    def exec_module(self, module):
-        fn = module.__spec__.origin
+        fn = Path(module.__spec__.origin)


lol, in my last patch I moved this away from pathlib due to needing fspath hax -- this is fine though I guess 🤷‍♂

asottile · 2019-09-23T16:30:00Z

src/_pytest/assertion/rewrite.py

@@ -291,12 +294,12 @@ def _read_pyc(source, pyc, trace=lambda x: None):
    Return rewritten code if successful or None if not.
    """
    try:
-        fp = open(pyc, "rb")
+        fp = open(fspath(pyc), "rb")


this one shouldn't need fspath should it?

It does, on py35 open doesn't support Path objects, and there are a bunch of tests which call _read_pyc directly to handle different situations correctly. I think it is fine to keep it.

if we need hax for py35 to use pathlib I'd prefer we don't use pathlib until we drop python3.5

I don't think it is hack to be honest... it is a compatibility function, like we have many others in compat. It is literally a if/else with an implementation specific to py35.

This request would need me to review the entire patch, and I really would like to avoid that unless we have good reason.

I agree that this particular case is just a compat function, but pathlib in general feels like a liability if it's going to blow up mysteriously in python3.5

if it's going to blow up mysteriously in python3.5

Definitely agree for external APIs, but this is an internal change. And it doesn't blow mysteriously: remove that call and it does fail on python3.5 immediately, with a good error message. Or perhaps you mean something else by mysteriously?

this particular usage doesn't, but I'm not confident of test coverage about all of our uses of such functions

there's also potentially more surface area than expected here since it's an import hook -- but maybe it's fine

I just have my reservations about pathlib being a good idea when there's some pretty easy footguns :) (and I don't like pathlib myself)

Well, I disagree, so let's hold this PR until we drop py35 then. 👍

I'm fine with this PR btw, just concerned is all ;) -- I think if you use exist_ok=True below I'll press approve :D

asottile · 2019-09-23T16:31:30Z

src/_pytest/assertion/rewrite.py

@@ -1021,9 +1024,9 @@ def visit_Compare(self, comp: ast.Compare):
 def try_mkdir(cache_dir):
    """Attempts to create the given directory, returns True if successful"""
    try:
-        os.mkdir(cache_dir)
+        os.makedirs(fspath(cache_dir))


exist_ok=True and then I think you can avoid the except: block maybe -- or just use suppress

I changed to use cache_dir.mkdir as you suggest, unfortunately I could not get rid of FileExistsError handling (got some test failures), plus test_try_mkdir seems to be fine tuned already and I would have to change it now that cache_dir would be a Path, so I would prefer to not really change this if possible. 😬

I also tried to use suppress, but it loses its appeal because we still need try/except for the other errors (so we can return False on some of them):

diff --git a/src/_pytest/assertion/rewrite.py b/src/_pytest/assertion/rewrite.py index 415676aaa..0527a3639 100644 --- a/src/_pytest/assertion/rewrite.py +++ b/src/_pytest/assertion/rewrite.py @@ -1,5 +1,6 @@ """Rewrite assertion AST to produce nice error messages""" import ast +import contextlib import errno import functools import importlib.abc @@ -1024,8 +1025,8 @@ warn_explicit( def try_mkdir(cache_dir): """Attempts to create the given directory, returns True if successful""" try: - os.makedirs(fspath(cache_dir)) - except FileExistsError: + with contextlib.suppress(FileExistsError): + os.makedirs(fspath(cache_dir)) # Either the pycache directory already exists (the # common case) or it's blocked by a non-dir node. In the # latter case, we'll ignore it in _write_pyc.

I mean to use makedirs(..., exist_ok=True) ~essentially mkdir -p but only available in python3 :)

asottile · 2019-09-23T16:33:37Z

src/_pytest/assertion/rewrite.py

+        #   path = '/home/user/proj/test_app.py'
+        # we want:
+        #   '/tmp/pycs/home/user/proj'
+        return Path(sys.pycache_prefix) / Path(*file_path.parts[1:-1])


this seems broken for windows if I had to guess

Why? If you see the unittest, it seems correct.

oh I see, it's using .parts -- thought this was slicing strings -- I'll have to look into this more :/

asottile · 2019-09-23T16:35:19Z

src/_pytest/compat.py

+        """os.fspath replacement, useful to point out when we should replace it by the
+        real function once we drop py35.
+        """
+        return str(p)


this is not ~quite right -- but maybe good enough for our uses? here's the implementation from python3.6:

def _fspath(path): """Return the path representation of a path-like object. If str or bytes is passed in, it is returned unchanged. Otherwise the os.PathLike interface is used to get the path representation. If the path representation is not str or bytes, TypeError is raised. If the provided path is not str, bytes, or os.PathLike, TypeError is raised. """ if isinstance(path, (str, bytes)): return path # Work from the object's type to match method resolution of other magic # methods. path_type = type(path) try: path_repr = path_type.__fspath__(path) except AttributeError: if hasattr(path_type, '__fspath__'): raise else: raise TypeError("expected str, bytes or os.PathLike object, " "not " + path_type.__name__) if isinstance(path_repr, (str, bytes)): return path_repr else: raise TypeError("expected {}.__fspath__() to return str or bytes, " "not {}".format(path_type.__name__, type(path_repr).__name__))

but maybe good enough for our uses?

I think so. We will be using esporadically to convert Path objects only, and __fspath__ of Path objects implement a call to str():

def __fspath__(self): return str(self)

And it is also cached:

def __str__(self): """Return the string representation of the path, suitable for passing to system calls.""" try: return self._str except AttributeError: self._str = self._format_parsed_parts(self._drv, self._root, self._parts) or '.' return self._str

So I think it is fine as is. Also we will be dropping py35 soon I think, so we can just call os.fspath directly. 👍

nicoddemus · 2019-09-27T00:57:50Z

src/_pytest/assertion/rewrite.py

@@ -291,12 +294,12 @@ def _read_pyc(source, pyc, trace=lambda x: None):
    Return rewritten code if successful or None if not.
    """
    try:
-        fp = open(pyc, "rb")
+        fp = open(fspath(pyc), "rb")


It does, on py35 open doesn't support Path objects, and there are a bunch of tests which call _read_pyc directly to handle different situations correctly. I think it is fine to keep it.

nicoddemus · 2019-09-27T00:59:13Z

src/_pytest/assertion/rewrite.py

+        #   path = '/home/user/proj/test_app.py'
+        # we want:
+        #   '/tmp/pycs/home/user/proj'
+        return Path(sys.pycache_prefix) / Path(*file_path.parts[1:-1])


Why? If you see the unittest, it seems correct.

nicoddemus · 2019-09-27T01:01:35Z

src/_pytest/compat.py

+        """os.fspath replacement, useful to point out when we should replace it by the
+        real function once we drop py35.
+        """
+        return str(p)


but maybe good enough for our uses?

I think so. We will be using esporadically to convert Path objects only, and __fspath__ of Path objects implement a call to str():

def __fspath__(self): return str(self)

And it is also cached:

def __str__(self): """Return the string representation of the path, suitable for passing to system calls.""" try: return self._str except AttributeError: self._str = self._format_parsed_parts(self._drv, self._root, self._parts) or '.' return self._str

So I think it is fine as is. Also we will be dropping py35 soon I think, so we can just call os.fspath directly. 👍

nicoddemus · 2019-09-27T01:02:21Z

testing/test_assertrewrite.py

+    @pytest.mark.parametrize(
+        "prefix, source, expected",
+        [
+            ("c:/tmp/pycs", "d:/projects/src/foo.py", "c:/tmp/pycs/projects/src"),


@asottile here are the tests for Windows, which I obtained by testing how sys.pycache_prefix worked on my system.

nicoddemus · 2019-09-27T01:17:53Z

src/_pytest/assertion/rewrite.py

@@ -1021,9 +1024,9 @@ def visit_Compare(self, comp: ast.Compare):
 def try_mkdir(cache_dir):
    """Attempts to create the given directory, returns True if successful"""
    try:
-        os.mkdir(cache_dir)
+        os.makedirs(fspath(cache_dir))


I changed to use cache_dir.mkdir as you suggest, unfortunately I could not get rid of FileExistsError handling (got some test failures), plus test_try_mkdir seems to be fine tuned already and I would have to change it now that cache_dir would be a Path, so I would prefer to not really change this if possible. 😬

I also tried to use suppress, but it loses its appeal because we still need try/except for the other errors (so we can return False on some of them):

diff --git a/src/_pytest/assertion/rewrite.py b/src/_pytest/assertion/rewrite.py index 415676aaa..0527a3639 100644 --- a/src/_pytest/assertion/rewrite.py +++ b/src/_pytest/assertion/rewrite.py @@ -1,5 +1,6 @@ """Rewrite assertion AST to produce nice error messages""" import ast +import contextlib import errno import functools import importlib.abc @@ -1024,8 +1025,8 @@ warn_explicit( def try_mkdir(cache_dir): """Attempts to create the given directory, returns True if successful""" try: - os.makedirs(fspath(cache_dir)) - except FileExistsError: + with contextlib.suppress(FileExistsError): + os.makedirs(fspath(cache_dir)) # Either the pycache directory already exists (the # common case) or it's blocked by a non-dir node. In the # latter case, we'll ignore it in _write_pyc.

nicoddemus · 2019-10-16T22:01:50Z

So I think it is fine as is. Also we will be dropping py35 soon I think, so we can just call os.fspath directly.

Actually someone brought to my attention that py35 support ends in late 2020, not late 2019 as I thought initially.

Regarding this PR, should we:

Go ahead with it the way it is, using the fspath in compat for py35.
Revert all the Path changes.
Wait until we drop py35.

I myself vote for 1), str(p) is how we have been dealing with Path on Python versions that don't have os.fspath since we introduced pathlib.

asottile · 2019-10-16T22:08:44Z

I'm fine with this PR btw, just concerned is all ;) -- I think if you use exist_ok=True below I'll press approve :D

yeah I'm ok to go ahead with this -- just want to see os.makedirs(..., exist_ok=True)

nicoddemus · 2019-10-16T22:39:51Z

just want to see os.makedirs(..., exist_ok=True)

Done in 8707521.

asottile

Fix pytest-dev#4730

nicoddemus force-pushed the sys-pycache-prefix branch 2 times, most recently from d9f1026 to b456eb2 Compare September 20, 2019 00:11

blueyed closed this Sep 21, 2019

blueyed reopened this Sep 21, 2019

nicoddemus commented Sep 21, 2019

View reviewed changes

src/_pytest/assertion/rewrite.py Outdated Show resolved Hide resolved

nicoddemus force-pushed the sys-pycache-prefix branch from 2b8d203 to 7c98874 Compare September 23, 2019 15:44

asottile reviewed Sep 23, 2019

View reviewed changes

nicoddemus commented Sep 27, 2019

View reviewed changes

nicoddemus changed the title ~~Support sys.pycache_prefix on py38~~ Support sys.pycache_prefix on py38 [waiting until py35 is dropped] Sep 28, 2019

nicoddemus force-pushed the sys-pycache-prefix branch from 7c98874 to 8707521 Compare October 16, 2019 22:39

asottile approved these changes Oct 16, 2019

View reviewed changes

Support sys.pycache_prefix on py38

f93f284

Fix pytest-dev#4730

nicoddemus force-pushed the sys-pycache-prefix branch from 8707521 to 6d42284 Compare October 26, 2019 13:24

nicoddemus changed the title ~~Support sys.pycache_prefix on py38 [waiting until py35 is dropped]~~ Support sys.pycache_prefix on py38 Oct 26, 2019

Introduce compat.fspath

6f20b4b

nicoddemus force-pushed the sys-pycache-prefix branch from 6d42284 to 6f20b4b Compare October 26, 2019 13:38

nicoddemus merged commit 1ad4ca6 into pytest-dev:features Oct 26, 2019

nicoddemus deleted the sys-pycache-prefix branch October 26, 2019 14:29

This was referenced Oct 28, 2019

Support Python3.8 PYTHONPYCACHEPREFIX (parallel filesystem cache for .pyc) #6095

Closed

assert rewriting: support sys.pycache_prefix (py38) #4730

Closed

The-Compiler mentioned this pull request Nov 11, 2021

TestPyCacheDir.test_sys_pycache_prefix_integration fails on Windows / Python 3.9 #9298

Open

sparrowt mentioned this pull request Dec 2, 2021

pytest hangs if it tries to rewrite modules without having access rights on Windows 10 #5844

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support sys.pycache_prefix on py38 #5864

Support sys.pycache_prefix on py38 #5864

nicoddemus commented Sep 19, 2019

asottile left a comment

asottile Sep 23, 2019

asottile Sep 23, 2019

nicoddemus Sep 27, 2019

asottile Sep 27, 2019

nicoddemus Sep 27, 2019

asottile Sep 27, 2019

nicoddemus Sep 27, 2019

asottile Sep 28, 2019

nicoddemus Sep 28, 2019 •

edited

asottile Sep 29, 2019

asottile Sep 23, 2019

nicoddemus Sep 27, 2019

asottile Sep 27, 2019

asottile Sep 23, 2019

nicoddemus Sep 27, 2019

asottile Sep 27, 2019

asottile Sep 23, 2019

nicoddemus Sep 27, 2019

nicoddemus Sep 27, 2019

nicoddemus Sep 27, 2019

nicoddemus Sep 27, 2019

nicoddemus Sep 27, 2019

nicoddemus Sep 27, 2019

nicoddemus commented Oct 16, 2019

asottile commented Oct 16, 2019

nicoddemus commented Oct 16, 2019

asottile left a comment

Support sys.pycache_prefix on py38 #5864

Support sys.pycache_prefix on py38 #5864

Conversation

nicoddemus commented Sep 19, 2019

asottile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus Sep 28, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus commented Oct 16, 2019

asottile commented Oct 16, 2019

nicoddemus commented Oct 16, 2019

asottile left a comment

Choose a reason for hiding this comment

nicoddemus Sep 28, 2019 •

edited