Repository: scrapy/scrapy
Pull request #1581 (2015-11-03 18:03:09)

[MRG+1] Make data_path work when outside project (used by HttpCacheMiddleware and Deltafetch plugin)
I ran into an issue trying to enable cache with `scrapy runspider`: the middleware gets disabled because it tries to create the `httpcache` directory inside a project data directory.

The [`utils.project.data_path`](https://github.com/scrapy/scrapy/blob/389882eb131250a1eb8418580b165625dd854bf0/scrapy/utils/project.py#L42) currently assumes that you're inside the project dir (maybe reasonably, since it's in the project module?).

I know this solution breaks compatibility a little bit, but IMO it would be solving a bug.

Btw, I noticed that this `data_path()` function is only being used by the HttpCacheMiddleware -- does it make sense to keep it as an util function?

codecov-io: ## [Current coverage][1] is `82.88%`
> Merging **#1581** into **master** will not affect coverage as of [`083ccf6`][3]



[1]: https://codecov.io/github/scrapy/scrapy?ref=083ccf677078dcd5861a98d8c4e89904edd1dc11
[2]: https://codecov.io/github/scrapy/scrapy/features/suggestions?ref=083ccf677078dcd5861a98d8c4e89904edd1dc11
[3]: https://codecov.io/github/scrapy/scrapy/commit/083ccf677078dcd5861a98d8c4e89904edd1dc11
[4]: https://codecov.io/github/scrapy/scrapy/compare/57f87b95d4d705f8afdd8fb9f7551033a7d88ee2...083ccf677078dcd5861a98d8c4e89904edd1dc11

> Powered by [Codecov](https://codecov.io). Updated on successful CI builds.
jdemaeyer: Hey Elias,

the http cache should definitely work with relative paths even for stand-alone spiders, so +1 for implementing this.

> I know this solution breaks compatibility

Hmm how's that? Is there code that relies on `data_dir` raising an exception if not inside a project?

> Btw, I noticed that this data_path() function is only being used by the HttpCacheMiddleware -- does it make sense to keep it as an util function?

It's a little misnamed but I think it makes sense as a utility function. "Give me this path relative to the project if I am in one" seems quite re-usable.

Could you add a test?
eliasdorneles: > Hmm how's that? Is there code that relies on data_path() raising an exception if not inside a project?

None in Scrapy itself, I meant it as a theoretical argument, since we can't say for sure that no user is using it. :)

> It's a little misnamed but I think it makes sense as a utility function. "Give me this path relative to the project if I am in one" seems quite re-usable.

+1, so if we agree it's okay to change this function, maybe we can even rename it. Perhaps `project_path_join` or something.
eliasdorneles: Btw, my current workaround to be able to use `-s HTTPCACHE_ENABLED=1` with `scrapy runspider` is to create an empty `scrapy.cfg` file to trick Scrapy to think it's inside a project...
Digenis: > > I know this solution breaks compatibility

> Hmm how's that? Is there code that relies on data_path() raising an exception if not inside a project?

`data_path()` raises NotConfigured
which then bubbles up to the httpcache `__init__`.
This in effect disables caching in the absence of a data path.
I assume people do their testing using the httpcache
and then they expect it not to be disabled in production.

If this change is applied then such projects can suddenly start caching.

I'd rather see this follow the API stability policy
and have a deprecation warning first.
When someone configures a relative cache dir without a project,
`data_path()`, before raising NotConfigured, it should warn
that such relative path will be being resolved by the next stable release.

jdemaeyer: Hm, I'm not sure I can follow you @Digenis 

> If this change is applied then such projects can suddenly start caching.

This PR will have no impact on http caching in projects. Scrapy will *always* cache as soon as `HTTPCACHE_ENABLED` is set to `True`, independent of whether `HTTPCACHE_DIR` is a relative or an absolute path (and independent of whether that paths exists before running the spider). When people don't want the http cache in their production environment, they need to set `HTTPCACHE_ENABLED = False` in that environment's `local_settings.py`.

This PR will only affect the http cache behaviour for *stand-alone spiders* (those that you run through `scrapy runspider` instead of `scrapy crawl`), in that they can now also use relative paths. That someone explicitly sets `HTTPCACHE_ENABLED = True` in their `Spider.custom_settings` and then finds the http cache disabled (because they're not in a project) is definitely a bug and not part of an API in my eyes.
Digenis: I typed the wrong word at some point, see edits.

I use scrapyd for deployment.
`scrapy.utils.conf` uses `os.path`
and `scrapyd-deploy` never added `scrapy.cfg` as a package resource to the generated `setup.py`.
So the most popular way to deploy projects (scrapyd) has been "project-less" so far.

If I try `HTTPCACHE_DIR = './httpcache/' + some stuff`
`2015-11-06 22:30:19 [scrapy] WARNING: Disabled HttpCacheMiddleware: Unable to find scrapy.cfg file to infer project data dir`
Yes, it is a warning that should worry the project maintainer
but it speaks only about being outside of a project
not misusage of the middleware.

To cache without scrapy.cfg
I can either pass to scrapy `-s HTTPCACHE_DIR="$(readlink -f .)/httpcache"`
or in settings: `HTTPCACHE_DIR = os.path.join(os.getcwd(), 'httpcache')`

I do think it's fair for people
to be able to define a cache directory as simply as possible in a project-less spider
but I expect such a change to introduce special pitfalls
for someone who moves a spider in or out of a project.
I think the `cache <-> no cache` problem is better than `cache on ./ <-> cache on ../../.scrapy/`.
Especially with a misnamed function (should have been  more like "project_data_dir")
and a documentation (for httpcache) that doesn't clearly states where one is expected to find the cache.

However I am aware of the issue and I don't have affected code
so this all is just my 2 cents.
kmike: @Digenis so the backwards-incompatible change users can face in practice is that with `HTTPCACHE_ENABLED=True` cache will be enabled when using scrapyd? Did I get this right? 
Digenis: Exactly.
kmike: A nice catch about scrapyd. It sounds like a bug fix though; I think we should make this change, but add a note about it.
kmike: ping! I think the PR is almost ready, and it fixes an important problem - you can't use http cache with CrawlerRunner or CrawlerProcess without a Scrapy project (right?) 
eliasdorneles: Right, I still think that this is a bug fix.
It's not impossible to use http cache with them, there is an ugly workaround I mentioned above (creating a `scrapy.cfg` file in the current/root directory).

I agree with @jdemaeyer, I think it's reasonable to expect that if `HTTPCACHE_ENABLED=True` Scrapy should try to use the cache and I'm not keen on doing a cycle of deprecation just for this change.
Is there another way we could alleviate the problem for that case, like logging a message or something?
eliasdorneles: @redapple @kmike do you think if we document the backward incompatibility, we could get this in 1.2 as well?
redapple: @eliasdorneles , I'm ok with including this for 1.2
I don't know if I can explain the backwards incompatiblity myself, can you give it a shot for https://github.com/scrapy/scrapy/pull/2216 ?
Also, can this feature have a test for it?
eliasdorneles: Sure, I'll see if I can add a test and describe in a comment on #2216 (@redapple the branch is on your repo, I'm unable to commit on it).

Patch:
diff --git a/scrapy/utils/project.py b/scrapy/utils/project.py
index a15a0d90f..95c6a8035 100644
--- a/scrapy/utils/project.py
+++ b/scrapy/utils/project.py
@@ -12,6 +12,7 @@ from scrapy.exceptions import NotConfigured
 ENVVAR = 'SCRAPY_SETTINGS_MODULE'
 DATADIR_CFG_SECTION = 'datadir'
 
+
 def inside_project():
     scrapy_module = os.environ.get('SCRAPY_SETTINGS_MODULE')
     if scrapy_module is not None:
@@ -23,6 +24,7 @@ def inside_project():
             return True
     return bool(closest_scrapy_cfg())
 
+
 def project_data_dir(project='default'):
     """Return the current project data dir, creating it if it doesn't exist"""
     if not inside_project():
@@ -39,16 +41,22 @@ def project_data_dir(project='default'):
         os.makedirs(d)
     return d
 
+
 def data_path(path, createdir=False):
-    """If path is relative, return the given path inside the project data dir,
-    otherwise return the path unmodified
+    """
+    Return the given path joined with the .scrapy data directory.
+    If given an absolute path, return it unmodified.
     """
     if not isabs(path):
-        path = join(project_data_dir(), path)
+        if inside_project():
+            path = join(project_data_dir(), path)
+        else:
+            path = join('.scrapy', path)
     if createdir and not exists(path):
         os.makedirs(path)
     return path
 
+
 def get_project_settings():
     if ENVVAR not in os.environ:
         project = os.environ.get('SCRAPY_PROJECT', 'default')


Test patch:
diff --git a/tests/test_utils_project.py b/tests/test_utils_project.py
new file mode 100644
index 000000000..1239195e8
--- /dev/null
+++ b/tests/test_utils_project.py
@@ -0,0 +1,35 @@
+import unittest
+import os
+import tempfile
+import shutil
+import contextlib
+from scrapy.utils.project import data_path
+
+
+@contextlib.contextmanager
+def inside_a_project():
+    prev_dir = os.getcwd()
+    project_dir = tempfile.mkdtemp()
+
+    try:
+        os.chdir(project_dir)
+        with open('scrapy.cfg', 'w') as f:
+            # create an empty scrapy.cfg
+            f.close()
+
+        yield project_dir
+    finally:
+        os.chdir(prev_dir)
+        shutil.rmtree(project_dir)
+
+
+class ProjectUtilsTest(unittest.TestCase):
+    def test_data_path_outside_project(self):
+        self.assertEquals('.scrapy/somepath', data_path('somepath'))
+        self.assertEquals('/absolute/path', data_path('/absolute/path'))
+
+    def test_data_path_inside_project(self):
+        with inside_a_project() as proj_path:
+            expected = os.path.join(proj_path, '.scrapy', 'somepath')
+            self.assertEquals(expected, data_path('somepath'))
+            self.assertEquals('/absolute/path', data_path('/absolute/path'))