ensures cache dir does exist #319

holgi · 2020-02-20T16:30:28Z

I ran into a problem when installing another project with flit for development purpose using flit install --pth-file. The user had a non-existing home directory set (`/nonexistent') and therefore the download of the trove classifiers failed.

The /nonexistent "home" directory is used on POSIX systems to denote a user without a home directory, e.g. user nobody or users only set up for services.

These proposed changes ensures that a valid directory is returned by the flit.validate.get_cache_dir() function.

If the flit-subdirectory of the cache-directory does not exist and could not be created, a temporary directory is used instead. To ensure that always the same temporary directory is used, the function is wrapped with the functors.lru_cache().

A tests for this new behavior is also included. The tests passed so far on Mac OS X (Python 3.7 and 2.7) and on FreeBSD (Python 3.6)

If the "flit" sub-directory could not be created, a temproary directory is used instead

this hopefully fixes the build errors on appveyor for windows builds

Windows allows the creation of the path "/dev/null/nonexistent/flit" - this leads to a failing test. Since I don't know any path that is protected under Windows, this test is marked as skipped.

takluyver

Thanks, this makes sense. Just a few small suggestions.

tests/test_validate.py

takluyver · 2020-02-22T18:04:56Z

flit/validate.py

+        pass
+    except (OSError, PermissionError):
+        cache_dir = Path(tempfile.TemporaryDirectory().name)
+        cache_dir.mkdir(parents=True)


AFAIK, TemporaryDirectory() always creates the directory, so we don't need to do so here.

I thought so too, but in one of my test setups this was not working reliably. Since I'm not at home and can't remember by heart which one it was, I'd need to check on this later.

Aha, I think I've got it. TemporaryDirectory() cleans itself up when the object is destroyed - which is immediately, because the code doesn't keep a reference to it. Use mkdtemp instead to get a directory which won't clear itself up.

Of course, this does mean each run of flit will create a new temp directory and not clear it up, which isn't ideal.

m(
Thanks for pointing this out. I ran head over heels into this kind of race condition in the first place and made things worse afterwards. Feeling kind of stupid now ;-).
Do you have any idea how to solve this without falling back on a module scope variable?

Unfortunately adding a new attribute to a pathlib.Path instance doesn't work.

To do what you're currently doing, with a temporary directory that never gets cleaned up, the easiest thing is to use the mkdtemp() function instead of TemporaryDirectory().

The other possibility would be - as you described - to use mkdtemp directly, but the cleanup afterwards must also be managed somehow.

Sorry for spamming you with comments.
I think I have a fix in commit bef5f2b using a context manager. I had to slightly rewrite validate_classifiers also. #

@takluyver Sorry, I didn't see your reply – my connection is a little bit flaky, sitting on a train today. I actually want an automatic cleanup. Circumventing it was just a stupid mistake on my side.

takluyver · 2020-02-24T17:50:49Z

That solves the cleanup problem, but if we create a new 'cache directory' each time the function is used, we're not really caching anything, so we can probably simplify things.

holgi · 2020-02-24T18:51:37Z

I know that it's not caching anything. But this will only be in effect for (posix) users that don't have a (writable) home directory set, so probably related to some service / daemon. And also only while using flit to install the project (not if the install is done via pip).

Since I can't think of an posix path that has guaranteed write access for all users, I think not caching in this simple case is acceptable.

Currently an install in such a case fails, so this is an improvement IMHO.

Another possibility would be to introduce and check for a "FLIT_CACHE_DIR" environmental variable (similar to the suggestion in #320). In a case where there is no writable automagical cache path, the flit command could be prefixed like FLIT_CAHCH_DIR=/vatr/tmp/cache flit install --pth-file.

holgi · 2020-02-24T19:04:46Z

Another possibility would be to move the check for FLIT_NO_NETWORK to the start of the validate_classifiers() function, so when FLIT_NO_NETWORK is set, the trove classifier check is passed completely.

takluyver · 2020-02-24T22:04:29Z

Sorry to be unclear. I was thinking, if there's nothing being cached anyway, we don't need to bother creating a temporary directory and saving a file to disk - we can just get the data from the network and use it directly. If it's not clear how to do that, I can have a go myself next time I have a bit of spare time to work on it.

holgi · 2020-02-25T10:18:01Z

Since I had some time this morning, I removed the caching. Should I add the changes to this pull request or if you want to close this I'll open a new one.

takluyver · 2020-02-25T10:53:02Z

But it can still cache when there is a normal home directory, right?

If you're happy with the changes, feel free to push them to this PR. Or if you're not sure, it's fine to make another PR and we can work out what's best.

holgi · 2020-02-25T13:14:33Z

Yes. With the changes I made today (not pushed so far), caches would be created and used as before if it is possible.

After a download of the classifiers an attempt is made to create a cache file for later use, but it doesn't matter if this is successful or not.

takluyver

Thanks! I'm happy with the overall structure of this, so I'm reviewing the details now.

takluyver · 2020-03-01T12:07:27Z

flit/validate.py

@@ -34,17 +34,15 @@ def get_cache_dir() -> Path:
                or os.path.expanduser('~\\AppData\\Local')
        return Path(local, 'flit')

-def _verify_classifiers_cached(classifiers):
+
+def _read_classifiers_cached():
    """Check classifiers against the downloaded list of known classifiers"""


The docstring here needs to be updated.

This is fixed in commit 909f7b4

takluyver · 2020-03-01T12:07:38Z

flit/validate.py



-def _download_classifiers():
+def _download_and_chache_classifiers():


Suggested change

def _download_and_chache_classifiers():

def _download_and_cache_classifiers():

Typo is fixed in commit 909f7b4

takluyver · 2020-03-01T12:11:33Z

flit/validate.py

@@ -54,10 +52,27 @@ def _download_classifiers():
    cache_dir = get_cache_dir()
    try:
        cache_dir.mkdir(parents=True)
-    except FileExistsError:
+    except (FileExistsError, PermissionError, OSError):
+        # readonly mounted file raises OSError


I don't really like catching any OSError - that's quite a broad category. OSError objects normally have an e.errno attribute which you can check against constants in the errno module to look for specific errors. E.g. it sounds like this might be EROFS (read only file system) - but check if that's what you're hitting.

This pattern was more common in Python 2 code, before subclasses like FileExistsError and PermissionError were split out as their own subclasses.

I was running into this problem on a Mac with 10.15 "Catalina" running Python 3.7. The root and other system directories are read-only.

Trying to create the dir "/nonexistent/" raises a OSError for a readonly file system – unfortunately not a subclassed error like PermissionError but an OSError itself:

>>> open("/foo", "w") Traceback (most recent call last): File "<stdin>", line 1, in <module> OSError: [Errno 30] Read-only file system: '/foo'

I changed the try-except-block to be a little bit more verbose and check for an explicit errno.EROFS

tests/test_validate.py

takluyver · 2020-03-01T12:47:36Z

tests/test_validate.py

+
+
+def test_download_and_chache_classifiers():
+    classifiers = fv._download_and_chache_classifiers()


In general, I like it to be possible to run the tests without requiring network access. There are two approaches for this:

Mark tests which need network access, so there's a clear way to exclude them. I don't think there are any examples in Flit, but here's one from another of my projects: https://github.com/takluyver/pynsist/blob/bf2408bbd01b5da63b93107f48435a5a57f9048b/nsist/tests/test_pypi.py#L11-L14

Mock out the network responses, e.g. see the test_upload module.

Since you used responses already, I've choosen the second option.

holgi · 2020-03-02T10:35:24Z

Thanks for your patience with me and my changes ;-)

takluyver

Thanks, the actual code is looking code, I've just got a couple more comments about the tests.

Sorry that it's taken me a while to get back to this.

tests/test_validate.py

holgi · 2020-03-21T18:11:08Z

Sure no problem. You are probably doing this in your spare time 😄 👍

takluyver · 2020-03-22T13:22:26Z

I am indeed doing this in my spare time - though there might be more spare time with all these social distancing measures.

holgi · 2020-03-23T15:11:33Z

I removed the two tests as you proposed. Thanks again for your work and the patience with this pull request :-)

takluyver · 2020-03-23T18:41:08Z

Thanks for your patience with my reviewing. 🙂

holgi · 2020-03-24T06:30:31Z

👍

holgi added 3 commits February 20, 2020 17:11

ensures cache dir does exist

8cec86f

If the "flit" sub-directory could not be created, a temproary directory is used instead

changed fixture for get_cache_dir() test

1f61e66

this hopefully fixes the build errors on appveyor for windows builds

marked the test for get_cache_dir as skipped on windows

e7e0ffe

Windows allows the creation of the path "/dev/null/nonexistent/flit" - this leads to a failing test. Since I don't know any path that is protected under Windows, this test is marked as skipped.

takluyver reviewed Feb 22, 2020

View reviewed changes

takluyver mentioned this pull request Feb 24, 2020

use HOME to resolve the cache directory #320

Closed

holgi added 2 commits February 24, 2020 11:36

test fixture now uses pytest.monkeypatch

81eab27

fixing temp dir race condition

bef5f2b

create a cache file if possible, fail silently

b2737a1

takluyver added this to the 2.3 milestone Mar 1, 2020

takluyver reviewed Mar 1, 2020

View reviewed changes

holgi added 3 commits March 2, 2020 10:34

fixed typos, docstrings

909f7b4

marked test with network access

053e5f2

mocked network access

672008f

holgi added 3 commits March 2, 2020 12:01

fixing test for python3.5

8cf6a3c

fixing mistyped fixture

dca3628

added tests for catching OSError for readoly file systems

70f7231

takluyver reviewed Mar 21, 2020

View reviewed changes

tests/test_validate.py Outdated Show resolved Hide resolved

tests/test_validate.py Outdated Show resolved Hide resolved

tests/test_validate.py Outdated Show resolved Hide resolved

fixed typos in test cases

d2609d9

removed two extensive OSError test cases on request by @takluyver

9f3b9cf

takluyver merged commit 359114b into pypa:master Mar 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ensures cache dir does exist #319

ensures cache dir does exist #319

holgi commented Feb 20, 2020 •

edited by takluyver

Loading

takluyver left a comment

takluyver Feb 22, 2020

holgi Feb 24, 2020

takluyver Feb 24, 2020

holgi Feb 24, 2020

holgi Feb 24, 2020

takluyver Feb 24, 2020

holgi Feb 24, 2020

holgi Feb 24, 2020

holgi Feb 24, 2020

takluyver commented Feb 24, 2020

holgi commented Feb 24, 2020

holgi commented Feb 24, 2020 •

edited

Loading

takluyver commented Feb 24, 2020

holgi commented Feb 25, 2020

takluyver commented Feb 25, 2020

holgi commented Feb 25, 2020 •

edited

Loading

takluyver left a comment

takluyver Mar 1, 2020

holgi Mar 2, 2020

takluyver Mar 1, 2020

holgi Mar 2, 2020

takluyver Mar 1, 2020

holgi Mar 2, 2020

takluyver Mar 1, 2020

holgi Mar 2, 2020

holgi commented Mar 2, 2020

takluyver left a comment

holgi commented Mar 21, 2020

takluyver commented Mar 22, 2020

holgi commented Mar 23, 2020

takluyver commented Mar 23, 2020

holgi commented Mar 24, 2020



		def _download_classifiers():
		def _download_and_chache_classifiers():

	def _download_and_chache_classifiers():
	def _download_and_cache_classifiers():



		def test_download_and_chache_classifiers():
		classifiers = fv._download_and_chache_classifiers()

ensures cache dir does exist #319

ensures cache dir does exist #319

Conversation

holgi commented Feb 20, 2020 • edited by takluyver Loading

takluyver 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

Choose a reason for hiding this comment

takluyver commented Feb 24, 2020

holgi commented Feb 24, 2020

holgi commented Feb 24, 2020 • edited Loading

takluyver commented Feb 24, 2020

holgi commented Feb 25, 2020

takluyver commented Feb 25, 2020

holgi commented Feb 25, 2020 • edited Loading

takluyver 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

holgi commented Mar 2, 2020

takluyver left a comment

Choose a reason for hiding this comment

holgi commented Mar 21, 2020

takluyver commented Mar 22, 2020

holgi commented Mar 23, 2020

takluyver commented Mar 23, 2020

holgi commented Mar 24, 2020

holgi commented Feb 20, 2020 •

edited by takluyver

Loading

holgi commented Feb 24, 2020 •

edited

Loading

holgi commented Feb 25, 2020 •

edited

Loading