I believe it would be better and cleaner if drush would download and extract files in place instead of within a temporary directory. First, it would avoid conditions where /tmp isn't big enough and we run out of space extracting the archice. Second, it would make drush respect permissions (like +s or ACL defaults) set on the directory (like what was noted here: #841310: How to change permissions and owner after dl / update?).

To expand on the latter one, with the provision ACL module, it's possible for Aegir to set ACLs that allow developpers to access the files and themes of a site without any special privileges other than being part of the client group. This makes multi-user collaboration much easier and intuitive. Unfortunately, when a user downloads a module within one of those directories, with drush, drush first downloads it into the /tmp directory and then moves it in place. This makes the resulting files not respect the policies set on the directory, and then other users cannot access the files, and Aegir cannot remove the directory.

I will look into a patch to fix this.

CommentFileSizeAuthor
#24 drush-1168812.patch2.77 KBjonhattan
#2 1168812_inplace-4.patch2.54 KBanarcat
#1 1168812_inplace.patch2.94 KBanarcat
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

anarcat’s picture

anarcat CreditAttribution: anarcat commented
Status: Active » Needs review
FileSize
1168812_inplace.patch2.94 KB

Here's a patch for this. It seems relatively minor - the only change is that we create the temporary directory in the target directory instead of temp. It requires a minor API change (adding an optional parameter) so I think it could even be merged in 4.x if it works for you guys.

anarcat’s picture

anarcat CreditAttribution: anarcat commented
FileSize
1168812_inplace-4.patch2.54 KB

And here's the same patch for drush 4.

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson commented

I haven't tested this yet (e.g. on Windows), but it looks like a good improvement to me.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

Not sure it is a god idea to use sites/all/modules as staging ground like that. if someone visits the modules page while the code is in its staging area, the tmp location gets written to the system table. also, what if a process dies and leaves the code in the tmp area. It is then in the searchpath for drupal, its info file gets loaded, etc. Maybe we don't worry about this.

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson commented

Maybe we could use a sibling folder, such as sites/all/modules.tmp? Folks would have to know to apply their magic to sites/all instead of sites/all/modules (or just pre-create modules.tmp), but that could be solved with documentation.

Alternately, maybe we could invent an option or a hook that would let people specify their tmp directory -- relative paths being taken as relative to the drupal root (or use current algorithm if drupal not bootstrapped yet).

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
CreditAttribution: jonhattan commented

Not sure if I'm missing something obvious here. We already do if (_drush_recursive_copy($request['full_project_path'], $request['project_install_location'])) {, where full_project_path is the temporary directory used to download the package.

jonhattan@jengibre:/var/www/drupal7$ touch sites/all/modules/contrib/x
jonhattan@jengibre:/var/www/drupal7$ ls -lh sites/all/modules/contrib/x 
-rw-r--r-- 1 jonhattan jonhattan 0 jun  7 18:22 sites/all/modules/contrib/x

jonhattan@jengibre:/var/www/drupal7$ chown jonhattan:staff sites/all/modules/contrib/
jonhattan@jengibre:/var/www/drupal7$ chmod g+s sites/all/modules/contrib

jonhattan@jengibre:/var/www/drupal7$ touch sites/all/modules/contrib/y
jonhattan@jengibre:/var/www/drupal7$ ls -lh sites/all/modules/contrib/y
-rw-r--r-- 1 jonhattan staff 0 jun  7 18:20 sites/all/modules/contrib/y

jonhattan@jengibre:/var/www/drupal7$ drush dl devel
Project devel (7.x-1.0) downloaded to /var/www/drupal7/sites/all/modules/contrib/devel.                            [success]
Project devel contains 4 modules: devel_generate, performance, devel_node_access, devel.
jonhattan@jengibre:/var/www/drupal7$ ls -lhd sites/all/modules/contrib/devel
drwxr-xr-x 5 jonhattan staff 4,0K jun  7 18:21 sites/all/modules/contrib/devel

Other problem of #1 is that we really don't know the final location of a project until it has been downloaded and inspected by implementing COMMANDFILE_drush_pm_download_destination_alter(), as pm itself does.

anarcat’s picture

anarcat CreditAttribution: anarcat commented

So one of the issues here is that this would be a first step to downloading files in place.

Right now we do pretty insane stuff for SVN support:

0. make a backup
1. download the tarball
2. uncompress to tmp
3. remove all files but VCS meta-files and directories
4. copy files in those directories one by one
5. remove temp files

That seems convoluted, to say the least.

What i'm looking forward is:

0. make a backup
1. download the tarball
2. delete the old files
3. extract the new files

Done. The thing this will do however is that it will completely scrap VCS support for dl. *but*, if you are using version-control on a module, you should use version-control-aware code anyways, so this shouldn't normally happen. Plus we're thinking of ditching SVN support (now that we use git) so that would make sense.

So this code would change a bit more after that patch, but it's a first step, since with that we can just move the directory instead of copy it in place.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

I'll just add that it is really frustrating to see pages of file copy log mmessages when doing pm commands with --debug. I'm hoping this will reduce instances of that.

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson commented

I have been meaning to fix #8. The solution is to just call 'copy' directly from _drush_recursive_copy, and leave off the 'drush_op'. You'd also need to make sure that everyone who calls _drush_recursive_copy does it with drush_op, or make _drush_recursive_copy check for simulated on entry.

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson commented

Regarding #7, maybe we should add a hook to dl so that VCS-aware code (e.g. dog) could completely change the behavior of functions such as drush dl and drush upc. Maybe hook pre_download and pre_updatecode and change the drush command to something else, or otherwise cause the in-process invoke to not run any more hook functions (without causing an error). That would work for dl; upc would be a lot more complicated.

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson commented

#7 / #10 addressed as a separate issue in #1216716: _drush_recursive_copy produces too many log messages when run in --debug mode..

realityloop’s picture

realityloop
he/him
CreditAttribution: realityloop commented
Status: Needs review » Reviewed & tested by the community

Using drush locally on OSX with aegir. drush dl by default gives the wrong group (wheel) to downloaded files before applying the patch, running the steps that drush uses manually I was able to confirm this is caused by the creation of temp directory for download and extraction in /tmp.

After applying the #2 patch downloads get the correct group after extraction (in this case _www) and downloading over the top works correctly still by asking if I'm sure I want to overwrite for a Drupal core for instance.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Assigned: anarcat » jonhattan
Status: Reviewed & tested by the community » Needs work

Not all identified issues have been addressed. Needs more input from jonhattan as well.

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
CreditAttribution: jonhattan commented

#7 identifies a problem (or a convoluted approach) with a false solution. We can already scrap VCS support for dl in the current approach and simplify it. Same argumentation applies.

I'm still in #6. AFAIK switching from move to copy fixes all issues related to user/group, mask and acl. We already discussed this in #1190712: `drush up drupal` inconsistency when used by a non privileges user and permissions are not well set.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

Sounds like this is a Won't Fix as per #15?

drasgardian’s picture

drasgardian CreditAttribution: drasgardian commented

@jonhattan, In #6 you've shown that drush dl will respect the chmod g+s setting and the resulting folders are created with the group set to staff

drwxr-xr-x 5 jonhattan staff 4,0K jun  7 18:21 sites/all/modules/contrib/devel

However, this resulting folder hasn't inherited the g+s setting from its parent, which it would have done in a normal mkdir scenario.
i.e.

drwxr-sr-x 5 jonhattan staff 4,0K jun  7 18:21 sites/all/modules/contrib/devel

For us, this means that our developers (staff group) have permission to edit the files that drush has downloaded, but for any new files or folders they will have to manually set the group back to staff. The patch in #1 doesn't appear to help this either.

threexk’s picture

threexk CreditAttribution: threexk commented

I have the same problem as #17. The directories are created with the correct group, but do not inherit the setgid bit like normal. Just wanted to report that drasgardian wasn't the only person having this problem.

s4j4n’s picture

s4j4n CreditAttribution: s4j4n commented

I continue to have the same problem also as described in #17.

g+s and ACL settings are ignored by drush - which creates a mess I have to fix every time I use it.

I think it would be ideal if drush would follow the standard behavior of many other linux type command line applications - e.g. tar, mkdir, cp, vi, emacs - all of which respect g+s and ACL settings.

Drush is awesome - I hope this can be fixed soon.

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson commented

Maybe this is a daft idea, but here is something I just thought of.

  1. Create a new empty folder with a temp name at the final destination. It should inherit g+s settings from its parent.
  2. Move the new folder to Drush's temporary folder. It should bring its g+s settings with it.
  3. Perform the download into this folder that is now out of the way (per #4).
  4. Move + rename the temp folder to bring it back to the final destination, as we do today. Perms should still be fine.

I haven't tested this, but I suspect it would work. Perhaps someone who needs this behavior will try it out and roll a patch.

s4j4n’s picture

s4j4n CreditAttribution: s4j4n commented

Your idea will preserve g+s but unfortunately it ignores and in fact removes ACL settings.

How about if...

Drush would use the drupal sites tmp directory as set in Admin > Site Configuration > File System as a staging area?

If it's set to 

/tmp

(the default tmp directory) so be it - but if it's set to 

sites/default/files/tmp/

(for example) then that should respect g+s and any ACL's set within /var/www (or where ever that particular drupal site might be located in the file system).

Onus is on the site owner to set it to something that would work for their server setup.

Drush would then be just following their wishes, with default behavior (i.e. use /tmp) unchanged from how it works currently.

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
CreditAttribution: jonhattan commented
Status: Needs work » Active

The problem mentioned in #17 was introduced with the patch in #1073768-5: drush_copy_dir() does not keep file modes. If you revert it, g+s is inherited.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

Would be great if jonhattan or jhedstrom could look into a fix here

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
CreditAttribution: jonhattan commented
Title: download files in place instead of a temp dir » Respect filesystem permissions
Status: Active » Needs review
FileSize
drush-1168812.patch2.77 KB

Reimplemented #1073768 to only preserve execute bit. This allow to preserve group and s-bit inheritance.
Added also two tests in a new "filesystem" test class.

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
CreditAttribution: jonhattan commented
Status: Needs review » Fixed

Committed. http://drupalcode.org/project/drush.git/commit/55e3054

Status: Fixed » Closed (fixed)

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