Repository: fatiando/pooch

Issue:
pooch.create should test for write access to directories and have a failback for failures
See realworld implementation problem with Unidata/MetPy#933

`pooch.create` can fail to write to whatever cache directory it is presented with.  A failback mechanism should exist, like what does with `matplotlib` when it attempts to find writable locations.  There is also a [XDG Base Directory Specification](https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html) for this. 

```python
POOCH = pooch.create(
    path=pooch.os_cache('metpy'),
    base_url='https://github.com/Unidata/MetPy/raw/{version}/staticdata/',
    version='v' + __version__,
    version_dev='master',
    env='TEST_DATA_DIR')
```

## Full error message

```
File "/opt/miniconda3/envs/prod/lib/python3.6/site-packages/pooch/core.py", line 143, in create
   os.makedirs(versioned_path)
 File "/opt/miniconda3/envs/prod/lib/python3.6/os.py", line 210, in makedirs
     makedirs(head, mode, exist_ok)
  File "/opt/miniconda3/envs/prod/lib/python3.6/os.py", line 210, in makedirs
    makedirs(head, mode, exist_ok)
  File "/opt/miniconda3/envs/prod/lib/python3.6/os.py", line 220, in makedirs
    mkdir(name, mode)
PermissionError: [Errno 13] Permission denied: '/usr/share/httpd/.cache'
```

thank you



Discussion:
leouieda: Waiting on conda-forge/pytest-feedstock#55 to fix the pytest issues on Mac.

Patch:
diff --git a/pooch/core.py b/pooch/core.py
index 7093b7c..1967913 100644
--- a/pooch/core.py
+++ b/pooch/core.py
@@ -2,9 +2,10 @@
 Functions to download, verify, and update a sample dataset.
 """
 import os
+import sys
 from pathlib import Path
 import shutil
-from tempfile import NamedTemporaryFile
+import tempfile
 from warnings import warn
 
 import requests
@@ -12,6 +13,11 @@ import requests
 from .utils import file_hash, check_version
 
 
+# PermissionError was introduced in Python 3.3. This can be deleted when dropping 2.7
+if sys.version_info[0] < 3:
+    PermissionError = OSError  # pylint: disable=redefined-builtin,invalid-name
+
+
 def create(path, base_url, version, version_dev, env=None, registry=None):
     """
     Create a new :class:`~pooch.Pooch` with sensible defaults to fetch data files.
@@ -27,8 +33,6 @@ def create(path, base_url, version, version_dev, env=None, registry=None):
     ``https://github.com/fatiando/pooch/raw/v0.1/data``). If the version string
     contains ``+XX.XXXXX``, it will be interpreted as a development version.
 
-    If the local storage path doesn't exit, it will be created.
-
     Parameters
     ----------
     path : str, PathLike, list or tuple
@@ -73,9 +77,6 @@ def create(path, base_url, version, version_dev, env=None, registry=None):
     ...              registry={"data.txt": "9081wo2eb2gc0u..."})
     >>> print(pup.path.parts)  # The path is a pathlib.Path
     ('myproject', 'v0.1')
-    >>> # We'll create the directory if it doesn't exist yet.
-    >>> pup.path.exists()
-    True
     >>> print(pup.base_url)
     http://some.link.com/v0.1/
     >>> print(pup.registry)
@@ -89,8 +90,6 @@ def create(path, base_url, version, version_dev, env=None, registry=None):
     ...              version_dev="master")
     >>> print(pup.path.parts)
     ('myproject', 'master')
-    >>> pup.path.exists()
-    True
     >>> print(pup.base_url)
     http://some.link.com/master/
 
@@ -103,8 +102,6 @@ def create(path, base_url, version, version_dev, env=None, registry=None):
     ...              version_dev="master")
     >>> print(pup.path.parts)
     ('myproject', 'cache', 'data', 'v0.1')
-    >>> pup.path.exists()
-    True
 
     The user can overwrite the storage path by setting an environment variable:
 
@@ -127,10 +124,6 @@ def create(path, base_url, version, version_dev, env=None, registry=None):
     >>> print(pup.path.parts)
     ('myproject', 'from_env', 'v0.1')
 
-    Clean up the files we created:
-
-    >>> import shutil; shutil.rmtree("myproject")
-
     """
     version = check_version(version, fallback=version_dev)
     if isinstance(path, (list, tuple)):
@@ -138,9 +131,25 @@ def create(path, base_url, version, version_dev, env=None, registry=None):
     if env is not None and env in os.environ and os.environ[env]:
         path = os.environ[env]
     versioned_path = os.path.join(os.path.expanduser(str(path)), version)
-    # Create the directory if it doesn't already exist
-    if not os.path.exists(versioned_path):
-        os.makedirs(versioned_path)
+    # Check that the data directory is writable
+    try:
+        if not os.path.exists(versioned_path):
+            os.makedirs(versioned_path)
+        else:
+            tempfile.NamedTemporaryFile(dir=versioned_path)
+    except PermissionError:
+        message = (
+            "Cannot write to data cache '{}'. "
+            "Will not be able to download remote data files.".format(versioned_path)
+        )
+        if env is not None:
+            message = (
+                message
+                + "Use environment variable '{}' to specify another directory.".format(
+                    env
+                )
+            )
+        warn(message)
     if registry is None:
         registry = dict()
     pup = Pooch(
@@ -185,10 +194,10 @@ class Pooch:
         """
         Get the absolute path to a file in the local storage.
 
-        If it's not in the local storage, it will be downloaded. If the hash of file in
-        local storage doesn't match the one in the registry, will download a new copy of
-        the file. This is considered a sign that the file was updated in the remote
-        storage. If the hash of the downloaded file doesn't match the one in the
+        If it's not in the local storage, it will be downloaded. If the hash of the file
+        in local storage doesn't match the one in the registry, will download a new copy
+        of the file. This is considered a sign that the file was updated in the remote
+        storage. If the hash of the downloaded file still doesn't match the one in the
         registry, will raise an exception to warn of possible file corruption.
 
         Parameters
@@ -206,15 +215,27 @@ class Pooch:
         """
         if fname not in self.registry:
             raise ValueError("File '{}' is not in the registry.".format(fname))
+        # Create the local data directory if it doesn't already exist
+        if not self.abspath.exists():
+            os.makedirs(str(self.abspath))
         full_path = self.abspath / fname
         in_storage = full_path.exists()
-        update = in_storage and file_hash(str(full_path)) != self.registry[fname]
-        download = not in_storage
-        if update or download:
-            self._download_file(fname, update)
+        if not in_storage:
+            action = "Downloading"
+        elif in_storage and file_hash(str(full_path)) != self.registry[fname]:
+            action = "Updating"
+        else:
+            action = "Nothing"
+        if action in ("Updating", "Downloading"):
+            warn(
+                "{} data file '{}' from remote data store '{}' to '{}'.".format(
+                    action, fname, self.base_url, str(self.path)
+                )
+            )
+            self._download_file(fname)
         return str(full_path)
 
-    def _download_file(self, fname, update):
+    def _download_file(self, fname):
         """
         Download a file from the remote data storage to the local storage.
 
@@ -223,8 +244,6 @@ class Pooch:
         fname : str
             The file name (relative to the *base_url* of the remote data storage) to
             fetch from the local storage.
-        update : bool
-            True if the file already exists in the storage but needs an update.
 
         Raises
         ------
@@ -232,22 +251,13 @@ class Pooch:
             If the hash of the downloaded file doesn't match the hash in the registry.
 
         """
-        destination = Path(self.abspath, fname)
+        destination = self.abspath / fname
         source = "".join([self.base_url, fname])
-        if update:
-            action = "Updating"
-        else:
-            action = "Downloading"
-        warn(
-            "{} data file '{}' from remote data store '{}' to '{}'.".format(
-                action, fname, self.base_url, str(self.path)
-            )
-        )
-        response = requests.get(source, stream=True)
-        response.raise_for_status()
         # Stream the file to a temporary so that we can safely check its hash before
         # overwriting the original
-        with NamedTemporaryFile(delete=False) as fout:
+        with tempfile.NamedTemporaryFile(delete=False) as fout:
+            response = requests.get(source, stream=True)
+            response.raise_for_status()
             for chunk in response.iter_content(chunk_size=1024):
                 if chunk:
                     fout.write(chunk)


Test patch:
diff --git a/pooch/tests/test_core.py b/pooch/tests/test_core.py
index 55a8f42..28d6bb0 100644
--- a/pooch/tests/test_core.py
+++ b/pooch/tests/test_core.py
@@ -2,7 +2,9 @@
 Test the core class and factory function.
 """
 import os
+import sys
 from pathlib import Path
+import tempfile
 
 try:
     from tempfile import TemporaryDirectory
@@ -12,11 +14,16 @@ import warnings
 
 import pytest
 
-from .. import Pooch
+from .. import Pooch, create
 from ..utils import file_hash
 from .utils import pooch_test_url, pooch_test_registry, check_tiny_data
 
 
+# PermissionError was introduced in Python 3.3. This can be deleted when dropping 2.7
+if sys.version_info[0] < 3:
+    PermissionError = OSError  # pylint: disable=redefined-builtin,invalid-name
+
+
 DATA_DIR = str(Path(__file__).parent / "data")
 REGISTRY = pooch_test_registry()
 BASEURL = pooch_test_url()
@@ -104,3 +111,66 @@ def test_pooch_load_registry_invalid_line():
     pup = Pooch(path="", base_url="", registry={})
     with pytest.raises(ValueError):
         pup.load_registry(os.path.join(DATA_DIR, "registry-invalid.txt"))
+
+
+def test_create_makedirs_permissionerror(monkeypatch):
+    "Should warn the user when can't create the local data dir"
+
+    def mockmakedirs(path):  # pylint: disable=unused-argument
+        "Raise an exception to mimic permission issues"
+        raise PermissionError("Fake error")
+
+    data_cache = os.path.join(os.curdir, "test_permission")
+    assert not os.path.exists(data_cache)
+
+    monkeypatch.setattr(os, "makedirs", mockmakedirs)
+
+    with warnings.catch_warnings(record=True) as warn:
+        pup = create(
+            path=data_cache,
+            base_url="",
+            version="1.0",
+            version_dev="master",
+            env="SOME_VARIABLE",
+            registry={"afile.txt": "ahash"},
+        )
+        assert len(warn) == 1
+        assert issubclass(warn[-1].category, UserWarning)
+        assert str(warn[-1].message).startswith("Cannot write to data cache")
+        assert "'SOME_VARIABLE'" in str(warn[-1].message)
+
+    with pytest.raises(PermissionError):
+        pup.fetch("afile.txt")
+
+
+def test_create_newfile_permissionerror(monkeypatch):
+    "Should warn the user when can't write to the local data dir"
+    # This is a separate function because there should be a warning if the data dir
+    # already exists but we can't write to it.
+
+    def mocktempfile(**kwargs):  # pylint: disable=unused-argument
+        "Raise an exception to mimic permission issues"
+        raise PermissionError("Fake error")
+
+    with TemporaryDirectory() as data_cache:
+        os.makedirs(os.path.join(data_cache, "1.0"))
+        assert os.path.exists(data_cache)
+
+        monkeypatch.setattr(tempfile, "NamedTemporaryFile", mocktempfile)
+
+        with warnings.catch_warnings(record=True) as warn:
+            pup = create(
+                path=data_cache,
+                base_url="",
+                version="1.0",
+                version_dev="master",
+                env="SOME_VARIABLE",
+                registry={"afile.txt": "ahash"},
+            )
+            assert len(warn) == 1
+            assert issubclass(warn[-1].category, UserWarning)
+            assert str(warn[-1].message).startswith("Cannot write to data cache")
+            assert "'SOME_VARIABLE'" in str(warn[-1].message)
+
+            with pytest.raises(PermissionError):
+                pup.fetch("afile.txt")
diff --git a/pooch/tests/test_integration.py b/pooch/tests/test_integration.py
index aeb4ada..d7dd16f 100644
--- a/pooch/tests/test_integration.py
+++ b/pooch/tests/test_integration.py
@@ -26,15 +26,16 @@ def pup():
     )
     # The str conversion is needed in Python 3.5
     doggo.load_registry(str(Path(os.path.dirname(__file__), "data", "registry.txt")))
+    if os.path.exists(str(doggo.abspath)):
+        shutil.rmtree(str(doggo.abspath))
     yield doggo
     shutil.rmtree(str(doggo.abspath))
 
 
 def test_fetch(pup):
     "Fetch a data file from the local storage"
-    # Make sure the storage exists and is empty to begin
-    assert pup.abspath.exists()
-    assert not list(pup.abspath.iterdir())
+    # Make sure the storage has been cleaned up before running the tests
+    assert not pup.abspath.exists()
     for target in ["tiny-data.txt", "subdir/tiny-data.txt"]:
         with warnings.catch_warnings(record=True) as warn:
             fname = pup.fetch(target)