Make cache directory selection more robust in situations where selected location is not writable

The problem here is with the drush commandfile cache. Drush examines various environment variable and the current $HOME directory to determine where to cache these files. The webserver does not always have $HOME defined, and the environment can similarly be minimal. If Drush could not find an appropriate place to put the cache files, it erroneously tried to put them at /cache.

I committed a fix that makes the cache folder location more robust.

You should follow the instructions in drush topic docs-cron, which explains how to set up cron to run drush-5. If $HOME/.drush/cache is not writable, then drush should not be using that location any longer. If you are experiencing different results, please detail your experiences here, following the reporting guidelines.

I was relying on drush_mkdir to determine if the location is writable, but drush_mkdir calls drush_set_error.

I committed yet another improvement to the cache directory selection code that more robustly handles situations where a cache directory that drush would like to use exists, but is unwritable.

If this code does not work in your environment, then try adding CACHE_PREFIX=/path/to/cache in your list of environment variables when you call cron, and make sure that it specifies a cache directory that the www-data user can write to.

No, my fault. First, I should have tested in your exact configuration, and second, I should not have assumed that a "simple" change to drush_mkdir could just go in as a bugfix without review. It turns out that Drush assumes that it can call drush_mkdir($path) for any $path, working or not, without a drush_set_error being called. My previous change violated this assumption, which caused serious problems when certain directories are not writable. As you mention above, requiring these folders to be writable would be undesirable.

Please try the enclosed patch, which should work better. Note, however, that your test in #7 is incorrect. You must set CACHE_PREFIX like this:

$ CACHE_PREFIX=/tmp/drush
$ export CACHE_PREFIX
$ drush version

You need both the patch here and the export CACHE_PREFIX in order for this to work.

Please also run all of the Drush tests for me. I ran them yesterday, but it was a pain; I'm out of town for a week and on a mifi connection that is slow and capped. I'll be back in town next week and will run them then, but you could speed things along by doing the tests for me.

Feedback from other maintainers on another point would also be helpful: should drush_mkdir call drush_set_error under any circumstances? Drush 4.x does not, and it seems that the existing Drush code does not always handle that correctly. It's hard to tell, under the current codebase, whether a failed call to drush_mkdir is a "test", and Drush will just go on to some other directory if the current call fails, or a requirement. The existing calls to drush_set_error were added because there are some instances where failures in creating required directories caused hard-to-track silent failures. Perhaps what we really need is two different variants of drush_mkdir -- drush_mkdir and drush_mk_required_dir, which work analogously to php's concept of 'include' and 'require'. It would take some effort to figure out which drush_mkdir needs to become the 'required' version, but I think it might be necessary.

Seems like the function is more flexible if it does not throw an error. So, calls that need the error reported should do it themselves. Add a comment in doxygen for drush_mkdir instructing folks to throw own errors.

Here is the patch from #8 that I forgot to attach. It is not yet adjusted per #10.

re #9, Drush uses different caches for different purposes. /tmp/.drush/cache is, I believe, the extension cache, which I thought had been backed out of 5.x and 4.x alike. If not, that's a separate issue. /tmp/.drush is not a good cache location because it causes problems when you share drush between multiple users; if user A makes the cache dir, then user B cannot write to it.

Setting your own cache location is niche enough that I think that the environment variable is fine. Usually Drush should just choose a reasonable location for you.

This new patch will use /tmp/drush/USERNAME/cache if other locations are not available. This avoids the unpleasant possibility that two different users might end up using the same cache directory. This also means that you will not need to explicitly set CACHE_PREFIX when calling drush cron from the www-data user unless you really want to.

I kept the error checking inside of drush_mkdir, because this function can be more specific than the caller can when an error is encountered. Permission errors are annoying (especially in support requests), and specific reporting is advantageous. Reporting is controlled by a $required parameter, that should be set to TRUE to enable reporting. It defaults to FALSE, which means that the caller should pick another location or handle the error themselves. I made a quick pass at explicitly setting $required for clients that were already well-behaved in this respect. Callers that are not doing adequate error checking I left alone.

If someone could run the unit tests for me... otherwise, I'll return to this next week.

Not applying cleanly for me.

Whoops. Here's a better version.

I don't see where we change the signature for drush_mkdir(). I was checking for corresponding Doxygen add.

I got one failure:

1) pmDownloadCase::testDestination
Failed asserting that file "/tmp/drush-sandbox/web/sites/dev/modules/devel/README.txt" exists.

/Users/mweitzman/c/h/drush/tests/pmDownloadTest.php:47

The doxygen change for drush_mkdir was in #15, but I accidentally left it off in #17. Here is yet another patch with the correct update.

phpunit pmDownloadTest.php all worked for me. I guess I'll need to wait until I get back when I have the time and bandwidth to run everything clean and see what's up.

n.b. drush/tests/README.txt has fairly good instructions on how to set up and run the unit tests.

Oh, I do think this patch is okay, but I will run all tests clean on Sunday or Monday to confirm.

looks good to me.

Committed. I'm seeing one failure with and without this patch:

There was 1 failure:

1) siteUpgradeCase::testUpgrade
Unexpected exit code: /usr/bin/drush --root=/tmp/drush-sandbox/web --uri=dev --nocolor user-create example --mail='example@example.com' --password=password
Failed asserting that matches expected .

I'll look at it again.

Care to contribute some unit tests that fail when a key cache location is not writable? :)

Hm, running the unit tests as www-data is a good idea, and probably best from a pragmatic point of view. From a practical standpoint, though, I don't think this is a practice that the maintainers are going to get into on a regular basis. I was thinking that perhaps a test class that explicitly made the unit test's $HOME folder unwritable (and restored its permissions after the fact) and then did some tests in the unwritable state would be an ideal way to catch future defects in this class.

I was just careless somewhere in the commit; I'll fix it again, and write unit tests so it does not happen again, just as soon as I have a chance. Thanks for your feedback and help.

Drush was requiring that the 'lib' dir exists and is writable. I changed the test to allow it to be write-protected, so long as it already exists.

Thanks. Re-setting status to 'fixed'.

Edit: nevermind.

Comment on #15:

Setting your own cache location is niche enough that I think that the environment variable is fine.

I really did need to set the cache folder because of my specific situation. I added this to the drushrc.php file:

putenv('CACHE_PREFIX=/home/staff/shared/drush');