The way drush messes around with the working directory is really confusing. I found out in #1158490: drush dl --destination should assume a relative path that we can't really rely on the current directory being what it was when the process was started because a bunch of functions mess around with the current working directory. The boostrap process and package managers are the main culprits here.

Now, there's a handy drush_cwd() command to work around that, but you'll notice it doesn't work on windows, because windows doesn't set $PWD when the process start. $PWD is nothing magic - it's inherited by the shell, so we shouldn't expect it to be available.

During the bootstrap of Drupal, we do set the original directory:

../../includes/environment.inc:727:  drush_set_context('DRUSH_OLDCWD', getcwd());

Interestingly enough, drush_cwd() says:

 * TODO: Could cache result, but it isn't really expensive.

This sure looks like a cache to me!

So to summarize here, the problem is as follows:

* getcwd() is not reliable within Drush
* to work around this, we have drush_cwd(), which isn't reliable either (because it relies on PWD)

My suggestion is that we move drush_cwd() to a cache-only operation and configure the cache earlier in the bootstrap. Then we have a more uniform and reliable interface, and we actually make OLDCWD useful.

CommentFileSizeAuthor
#1 1158504_cwd_wtf.patch2.42 KBanarcat

Comments

anarcat’s picture

Status: Active » Needs review
StatusFileSize
new2.42 KB

i think this will clarify things a bit.

jonhattan’s picture

IMO this patch is fine for 4.x (in case someone is using DRUSH_OLDCWD) and in 5.x we can use a static variable.

moshe weitzman’s picture

Seems fine to me. Any objections? greg?

greg.1.anderson’s picture

Version: » All-versions-4.x-dev
Status: Needs review » Patch (to be ported)

Works great on Windows, and is a big improvement overall; committed to master. I don't have any objection to jonhattan's suggestion in #2 about a static variable; that would be another fine improvement, but I did not find it pressing.

msonnabaum’s picture

I think this is fine for 4 as well. Waiting for tests to pass and I'll commit.

Although a static variable seems redundant since the whole context system IS a static variable.

msonnabaum’s picture

Status: Patch (to be ported) » Fixed

Automatically closed -- issue fixed for 2 weeks with no activity.