Closed (fixed)
Project:
Drush
Component:
PM (dl, en, up ...)
Priority:
Critical
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
19 Nov 2010 at 22:07 UTC
Updated:
23 Dec 2010 at 19:40 UTC
Jump to comment: Most recent file
I've got a drupal site setup and installed on my macbook, and although drush used to work, it doesn't seem to be now, and I don't know what to change to get it back. I'm having trouble with drush up. If I try to drush up a specific module to a specific version:
> drush up wysiwyg-6.x-2.x-dev
Refreshing update status information ...
Done.
Update information last refreshed: Fri, 11/19/2010 - 16:01
Update status information on all installed and enabled Drupal projects:
Name Installed version Proposed version Status
Drupal core 6.19 6.19 Up to date
wysiwyg Unknown Unknown No version information found (if you have a CVS checkout you should install CVS Deploy module)
No code updates available. [ok]
No database updates required [success]
'all' cache was cleared [success]
Finished performing updates.Running "drush sm" results in a correct listing of the modules downloaded, but it doesn't include versions or update status.
| Comment | File | Size | Author |
|---|---|---|---|
| #59 | drush-977048.patch | 7.95 KB | jonhattan |
| #55 | drush-977048.patch | 5.5 KB | jonhattan |
| #48 | drush-977048.patch | 15.01 KB | jonhattan |
| #47 | drush-977048.patch | 13.56 KB | jonhattan |
| #45 | drush-977048.patch | 7.1 KB | jonhattan |
Comments
Comment #1
jonhattanMake sure you have the module enabled. If not, this is the result you will obtain.
Comment #2
jonhattanCommitting attached patch to detect if given project is installed and enabled.
@idlewilder test with drush HEAD and reopen if neccesary.
Comment #3
safetypinIt looks like your patch is working correctly, I get a warning when the module is not enabled. However, drush is still unable to detect the version when the module is enabled:
Comment #4
safetypinIs it possible that this is an issue with the wysiwyg module? I'll try getting dev and head versions for testing.
Comment #5
jonhattanwhoops I missed to remove a print_r() in te previous commit. Now removed. If you see the debug information in your post, [version] is empty. I think it is indicative of 'datestamp' missing in the module's .info.
I didn't ask but do you a cvs checkout of the module? That explains it. In that case you need cvs_deploy, as the status column shows.
I've tested with wysiwyg and no problem so far.
Comment #6
jonhattanComment #7
safetypinOkay, I looked back at my drushrc.php file, and I had commented out the package-handler option. After enabling it, nothing changed, but after downloading/installing the cvs_deploy module, it worked as expected. Thanks! Is the cvs_deploy module a new dependency? I've got an older drush version on a server that detects module version without the cvs_deploy module, and I don't even think I'm using the cvs package handler.
Comment #8
jonhattanit's not a new dependency. It's neither a dependency strictly speaking. AFAIK it is needed for cvs checkouts as they don't provide a datestamp in the .info file -- that's added by d.o packaging system.
Comment #9
greg.1.anderson commentedThe patch in #2 breaks
drush pm-update drupal.The problem we have is that $project_info contains info for all installed modules, but it does not include an entry for 'drupal', so DRUSH_PM_UNKNOWN_PROJECT is set. Unfortunately, the error message is never printed, because updatecode returns TRUE (no error). The calls to drush_set_error above should be
return drush_set_error(..., as shown in the enclosed patch. You might prefer to let the loop run and check drush_get_error at the end of the function, but it is important to return FALSE when an error condition occurs, as the drush command dispatcher currently requires this.Comment #10
jonhattanThanks Greg. Commited as is. Perhaps I come back to make it more user friendly in a future.
Comment #11
luchochs commented#2's patch is a bug, but #9's patch does not seem the appropriate solution. Also it seems good to clarify that relates not only to drupal, but with any "package" (i.e. cck etc.).
Update: better expressed.
Comment #12
greg.1.anderson commented@luchochs: I don't understand.
@jonhattan: I think that
drush_log(..., 'error')might also be a better option here...?Comment #13
luchochs commenteddrush_get_projects()returns a unreliable result.So if I today want to upgrade cck (or any other "package"), I can't.
Comment #14
greg.1.anderson commentedAh, I see. Today, drush's naming conventions are very confusing in the code. Usually, a project means the name of a collection of modules and/or themes (a "project" on drupal.org). Unfortunately, there are places where drush also uses the word "project" to mean "either a module or a theme". In my opinion, these comments and variable and method names should be changed to avoid confusion. Not sure what they should be changed to; maybe "moduleOrTheme" instead of "project"? A bit wordy. (n.b. Drush does not use the word "package" as you do in #13 (Edit: yes it does), and should not. Since d.o calls projects "projects", so should drush.)
Note that
drush help --filter=pmis already pretty accurate on whether an argument should be a project or a module/theme. It seems that the problem is that the argument to pm-update is a project, but the functiondrush_get_projects()returns a list of modules.If this is the case, then I would not say that
drush_get_projects()does not return an unreliable result, but its use is incorrect in pm-updatecode, because updatecode is expecting a list of project names, but it is instead getting a list of module names. This misuse is in part due to the "fuzzy naming" problem I refer to in the first paragraph of this comment.Is that correct? If so, I think that the correct solution would be to (a) fix the naming conventions of existing functions, so drush_get_projects becomes drush_get_modules_and_themes (perhaps), and (b) make a new function drush_get_projects that works like drush_get_modules_and_themes, but returns the names of the projects containing the modules and themes returned by said routine.
Comment #15
moshe weitzman commentedI noticed that the Drupal docs team has standardized on 'extension' as their word for ModuleOrTheme. We should do the same. That way, we reserve project to refer to a package that contains modules/themes and is downloadable from drupal.org.
Comment #16
luchochs commentedCopy & paste the current terminology for a good reference and see what needs to be modified (if that were the case).
Comment #17
greg.1.anderson commentedYes, you are completely correct. Contrary to what I said in #14, the drush package manager uses "package" all over the place. This is confusing, though, since the "package" item in Drupal .info files is used to refer to something completely different: a collection of projects.
From Writing .info files:
I think that "pm" should be renamed to "project manager" for consistency with usage in d.o; I also like Moshe's suggestion to use "extension" to refer to "module or theme", as is also done in d.o.
There is at least one --package flag that will have to change, so this should be done prior to 4.0 stable...
Comment #18
jonhattanIt's not the first time we hit the inconsistency/ambiguity in naming. There's some old issue and I also remember this: http://groups.drupal.org/node/91784#comment-292894
So I'll not repeat myself. On occasion I have proposed 'subproject', that's a weird and unexact word, in the lack of a better one. If 'extension' is the new word for a single .info thing let's use it.
The other problem we have here is the reverse mapping of 'extensions' to 'projects'. Given
contentthere's no direct way to know it is part ofcck, although some heuristics can be made (using update service, checking the parent directory name). So some commands works for projects, some for extensions:pm-download cckvspm-info content.Perhaps drush.ws could provide such a webservice by leveraging #112692: Drupal ports collection - automate package installation and dependency checking or perhaps http://drupal.org/project/project_api
Comment #19
moshe weitzman commentedRe #17 - the usage of 'package' in .info files is unfortunate. We typically use that term in the sense of apt-get and yum. The only time we should use 'package' like .info files uses it is in the listing and filter for pm-list when we are trying to match the functionality of the Modules page.
the word 'extension' is is being used and discussed a bit at #607028: Add README.txt to core.
Comment #20
greg.1.anderson commentedYes, I like the idea of #112692: Drupal ports collection - automate package installation and dependency checking quite a bit. Drush can already map from extension to project for downloaded projects; pm-enable does that. That should be enough here, since we are dealing with enabled modules, which therefore must have their dependencies met.
Comment #21
luchochs commentedI think these changes in terminology would be appropriate:
Comment #22
greg.1.anderson commentedI agree with Moshe in #19; therefore, the last line in #21 should be removed, and the definition of request should be changed to:
- Request: a requested project (string or keyed array), with a name and (optionally) versionThe other changes in #21 are good.
Comment #23
moshe weitzman commentedOK, we have some agreement. Someone has to go through the pm.drush.inc and actually implement the terminology changes.
Comment #24
luchochs commentedAttached a patch that apparently solves the current problem in this issue, and also addresses the @todos in pm-releases and pm-releasenotes.
It is likely that some
project's &package's variables (and flags in the latter case) need to be renamed, but to start I think this is fine.Comment #25
greg.1.anderson commented#24 works great.
The attached patch is the same code as #25, but with updated help text and updated comments. Variable names and array keys are not renamed. We may want to adjust the documentation somewhere (README.txt? Elsewhere?) to explain project / extension / module / theme / etc.
Comment #26
luchochs commentedThat good! I am trying to find a way of which we can support the DRUSH_PM_DISABLED_PROJECT's check. In the current pach I chose to eliminate it due to the inconsistent results for projects' status (not for the extensions).
Comment #27
luchochs commentedThe problem described in #26 seems to be fixed now.
P.S.: I exclude myself as a candidate to help with the documentation; yes, too engrish-prone :).
Comment #28
greg.1.anderson commentedAfter all of my changes in #25, it made #27 kind of hard to review. It tests out, though, and I think we've met our main objectives for this issue. I suggest that moshe &/or jonhattan should review and commit, and remaining documentation / variable munging should happen in a separate issue.
Comment #29
jonhattanI'm made a commit reverting #2, #9 and terminology changes based on #27, also including changes in variable and function names.
Following a patch with luchochs' code not shipped in http://drupal.org/cvs?commit=457910 up to discussion.
Comment #30
jonhattanHere is ~diff between #27 and #29 (it includes #2).
Comment #31
jonhattanMoving back to the original issue. Some facts:
1. cvs_deploy is required for drush up to work if modules are checked out from cvs. It is required for drush as far as it is also required by update.module. See the cvs deploy page: http://drupal.org/project/cvs_deploy
2. when a list of projects is given to drush up, updatecode will consider you want to explicitly update those projects. It doesn't check the projects are already downloaded and any of its extensions enabled. **It doesn't find the project in the update status info and it assumes the project was checkedout from cvs**.
3. As a consequence of 2) trying
drush up anythingwill print the cvs_deploy message.4. If cvs_deploy is not in the game update.module and drush up are blind to projects checked out from cvs.
So the problem we have with updatecode is the need to check that requested projects do exist in the filesystem and are enabled. Meaning a project is enabled that any of its extensions is enabled.
We have a similar problem for pm-release/notes. If the solution for updatecode is fine for pm-releases we can fix everything in this issue but the focus must be on updatecode. This other issue is in principle the place to fix pm-release/notes: #977584: Improvements to pm-releasenotes and other commands leveraging it..
patch in the oven.
Comment #32
luchochs commentedGood idea to separate the code, all more clear now.
Perhaps even more clearly separate this in:
- #977048 (this issue), and the proposed solution (#2).
- #XXXXXX (e.g.
drush_pm_get_extensions()'s refactor), and the proposed solution (#30 for now).Comment #33
jonhattan0. I've done a rename of a variable in updatecode.pm.inc. Don't get distracted by the noise.
1. I've moved some functions from includes/environment.inc to pm.drush.inc. Also other functions moved around within pm.drush.inc to be grouped. This is even more noise in the patch,... just put attention on
drush_get_projects()and_pm_project_exists_in_drupalorg().2. new funcion drush_get_projects(): obtain installed project names by interpolation of available extensions. If project was checkd out from cvs and no cvs_deploy it will try to guess the project name based on the parent directory. It doesn't work at all for projects with a directory name badly changed: cck -> mycck; works for a normal case, including cck-DRUPAL-6--2. Reiterate: all of this is only for the case of cvs co and no cvs_deploy. **EDIT** this is a dead end where the ports collection is needed.
3. On topic: updatecode checks for a)not installed projects b)projects with no enabled extensions c)projects from cvs.
Unexistant projects:
Projects with all extensions disabled (ignored by update.module):
all of this needs a second look from my part. Just wanted to dump it here and continue tomorrow.
Comment #34
luchochs commentedI will review tonight after dinner. I'm not a CVS/cvs_deploy-guy, therefore on this subject I may not say much.
For now +1 for the new Doxygen group.
Comment #35
luchochs commented#34's complement:
My opinion based on a couple of tests:
pm-update continues working OK (I can't test #33-3-c).
My opinion based on the code:
We also need
drush_get_projects()for pm-releases & pm-releasenotes commands, i.e. we also needdrush_get_projects()in_drush_pm_get_releases(). So we need the list of releases for each project.Basically, I think that
drush_get_projects()should be even moredrush_pm_get_extensions()-like (when @param $like_projects is TRUE, in #30).The idea of the new Doxygen group seems to me a good improvement as the new check, i.e. #33-3-c (I repeat: not tested).
Comment #36
luchochs commentedHmm, thinking this a bit more, I'm not sure if it's necessary the new code related to CVS users and no cvs_deploy module.
If the proposed code meets or improves the service provided by cvs_deploy.module, great. Otherwise a warning message like the current I think that would be enough.
Anyway, this issue is too confusing, I think it would be better if we create a issue to define
drush_get_projects()'s code (which is the main necessity), then continue with the improvements in some other issue. If it seems appropriate, I have no problems doing so.Comment #37
greg.1.anderson commentedSetting status pending arrival of new patch...
Comment #38
luchochs commentedMore opinions and less ellipsis please.
My opinion.
Comment #39
jonhattanI've commited the reorganization of some pm- helper functions, the rename of $projects to $update_info in updatecode.pm.inc and two minor fixes. All this was part of my patch in #33. http://drupal.org/cvs?commit=458896
New patch is much more clear.
Sumarizing:
1. introduce new function drush_get_projects(): obtain installed project names off available extensions. It is simplified in regards to my previous patch: it doesn't try to do the magic if cvs_deploy is not present.
2. the function is primarily used by updatecode to check for a)not installed projects b)projects with no enabled extensions c)projects from cvs.
3. change in pm-releases/notes to use the new function.
Comment #40
jonhattanComment #41
luchochs commentedMy opinion on the patch in #39:
The @param $extensions is not necessary, may be added in the future by not involve changing the function itself.
The current check (DRUSH_PM_NO_VERSION) does a good job on this.
We need the list of releases for each project (each extension has an array 'releases').
In short: I think we need re-roll #30.
Comment #42
luchochs commentedComment #43
jonhattanNew patch. Changes:
Removed $extensions argument.
---
Degraded this log to a debug message.
---
Added project version. Now the check in pm-releases just works.
@luchochs the list of releases is not available in $extensions and is not needed in $projects.
Comment #44
luchochs commentedWe are near the goal. Now suppose I have an installed version of drupal = 6.17.
Hm.
Hmm.
Hmmm.
And this check is redundant (at least I find no use to it):
In short: simpler is better.
Comment #45
jonhattanWith attached patch upc, releases and rln commands work with drupal.
The check for cvs_deploy is not redundant. In fact we need to be more aggresive if the user is using cvs checkouts and cvs_deploy is not enabled: the initial case of this issue --wysiwyg module-- was fixed by checking the extension exists and is enabled. It was wrong because extensions != projects and that solution doesn't work for projects like cck. OTOH, with drush_get_projects() we are unable to obtain the project name from its extensions.
So we need to pay explicit atention to sites using cvs checkouts without cvs_deploy enabled. Options are:
1. the implementation of drush_get_projects() in #33. Discarded. It is better to just copy cvs_deploy to drush.
2. print a warning message in a couple of places: drush_get_projects() and somewhere after bootstrap if --package-handler=cvs was given.
still needs work. I'll give a chance to option 2.
Comment #46
moshe weitzman commentedFYI, cvs_deploy will be basically deprecate in mid Feb when drupal moves to git.drupal.org. Then we depend on git_deploy which is at http://drupal.org/project/git_deploy
Comment #47
jonhattanChanges:
* new _validate() function for package-handler engine backends. It is called in the _validate() of pm-downoad and pm-update.
* extend to git_deploy.
* removed updatecode specific cvs_deploy message. It is never reached because the project can't be detected.
Comment #48
jonhattanUpdated patch after #983222: Suggest releases to download when there is no recommended release; use pm-releases without bootstrapping Drupal
Comment #49
greg.1.anderson commentedNow we have:
I think that the logic and placement of
DRUSH_PM_NO_VERSIONwas better, wasn't it? If we print the warning in drush_get_projects(), then we see it in instances such as the above, where the warning is accurate, but irrelevant to the operation. Also, the handling of DRUSH_PM_NO_VERSION allowed the warning to appear in the pm-updatecode table, which I think is better than removing the extension from the table.The other code changes in this patch look and work well.
Comment #50
jonhattanThe underlying logic has changed.
now updatecode checks for existent projects. For drupal (and drush) cck from cvs (and no cvs_deploy enabled) is the same as a not installed or a non-existent project. This is why drush_get_projects() raises a warning. The warning is on the extension, not the project --that is unknown,.. although some extension names match its project name.
At this point, if a project is not found, updatecode can't check for $update_info[$project]. The project is not found and is neither present in $update_info. DRUSH_PM_NO_VERSION is just a non reliable message. It doesn't mind if you did
drush up cckordrush up anything. See comments #31 and #45.The warning make sense for updatecode but I decided to not add a $quiet argument to not disturb other commands because it is hardly needed to enable cvs/git deploy if you're working with checkouts. (I have no problem if you prefer $quite).
In this line, I did introduce the package-handler validator. But it doesn't substitutes the d_g_projects() warning because you can inherit a site that uses cvs checkouts with no cvs deploy, so drush_get_projects()'s warning come to your aid.
Comment #51
greg.1.anderson commentedI think that the warning should be changed to 'debug' level so that it does not show up in pm-releases, etc., for unrelated projects. The user message should move back to pm-updatecode, but now the thing that upc will be noticing is that it has an extension with an unknown project. "Project not found. Perhaps it was checked out from CVS? Use cvsdeploy."
It would be slick if upc did check for a project name that matched the extension name, as this is common. If found and there is a CVS etc. directory there, then the warning could be more definitive.
Comment #52
jonhattanI prefer
function drush_get_projects($verbose=FALSE)and flag drush_log with a warning or debug, depending.I also think we shouldn't add to updatecode's table a row for a hipothetical project name that in some probability can match but we can't update anyway (because of the missing cvs_deploy).
Implementation of drush_get_projects() in #33 was an attempt to get the project name for non-drupalorg-packaged projects from the available extensions. I decided to drop that because it was simply easier to ship cvs_deploy (and git_deploy) with drush.
Anyway I don't know if I'm being stubborn here. I'm really blind on what to write in updatecode to improve the scenario when extension name match the requested project. Feel free to provide some code. Consider this options :)
(*) examples and others have all extensions in differente subdirectories. No one in the project root. You need at least two extensions enabled to make _pm_get_project_path() reliable in that case. **EDIT** it is just a extreme case I found with the approach in #33. It is not based on cvs/git deploy approach.
and let's remember.,.. we're discussing on the edge case where someone wants to explicitly update a project that was checked out from cvs/git and cvs_deploy is not ,.....
Comment #53
greg.1.anderson commentedYes, I understand the use cases you describe above; thank you for documenting them in the issue.
It comes down to a question of whether it's better to put the wrong information in the table, or to have no information at all, or put the critical information on a line outside the table where it might be missed. I feel that having the info outside the table is only marginally better than not having it, and therefore would prefer to see the information in the table, even though it is potentially wrong. The user is likely to be able to figure out what the project name is, or can follow the instructions to install CVS deploy / GIT deploy to make it right again.
In summary, I prefer current behavior to #48, even though there are some improvements in that patch. If moshe feels otherwise, then I will go along with #48, but I think it should at a minimum be enhanced so that the message includes a note about cvs deploy / git deploy.
Comment #54
luchochs commentedJonhattan, I sent an email in Spanish to better communication. If I made any mistake I apologize, really.
Comment #55
jonhattanI've commited some parts of the previous patch in #992282: drush_get_projects() to obtain installed projects and fix pm-releases/notes and #992264: Validate package handlers.
Here's a new patch, that fits current implementation (every request in the table). I still want to implement two things but it is reviewable in current status.
Comment #56
moshe weitzman commentedWhat does DRUSH_PM_REQUESTED_PROJECT_NOT_PACKAGED mean? The project exists but there are no releases? I'm unclear about the 'packaged' part.
Comment #57
greg.1.anderson commentedThis is looking way better vis-a-vis approach. I will review more completely later.
Comment #58
greg.1.anderson commentedWell heck, now I think I'm crazy.
I downloaded a module with --package-handler=cvs (and also used --select --all so that I could start with an old version). I got a warning message that I should use cvs deploy. Awesome.
I enabled the module and tried
drush pm-updatecode modulename. I got an error right in the update status table. Even more awesome.Next I tried
drush pm-updatecode. Hm, my new module has disappeared from the table. Not so awesome. I tried again with drush-3.3. Same behavior -- unknown projects do not show up in the table unless they appear on the commandline. Hm.I thought I had tested that in #49 and #51, but I guess I didn't. I still wish that the unknown module would show up (for every enabled module with unknown project information, put an entry in the table), but since this can be fixed with cvs_deploy and since it has never worked like this before (despite what my deluded memory seems to think it used to do), then I'd say that's a separate issue, and a minor one at that.
As far as I am concerned this is RTBC, clarify the comments per moshe's question in #56. I think I understand the code, though; DRUSH_PM_REQUESTED_PROJECT_NOT_PACKAGED means checked out with cvs rather than downloaded with
drush dl --package-handler=cvs, yes? If that explanation is not right, I still think it is okay to clarify with a comment and commit.Comment #59
jonhattanI've think of a new approach. It has the same features as before and also add some extras to the table. Perhaps a bit verbose but some messages are ideally to be shown only once (then you go and install cvs_deploy).
Explained with an example:
no cvs_deploy enabled as usual; ask pm-updatecode to update:
* cck (it is a cvs checkout)
* wysiwyg (project not installed)
* googleanalytics (note the project name is google_analytics)
* relativity (extension is disabled)
* views_ui (extension is enabled but the package is views!) **CORRECTION** extension is disabled.
* devel_generate
What was done is:
1. all enabled extensions obtained from cvs/git are in the table (content, text, number).
2. cck and wysiwyg are listed as unknown projects.
3. relativity is shown as not updateable because is disabled.
4. views project is selected given its extension views_ui. Similar case for google analytics.
The situation could still be improved for cck,content,text,number by checking extensions path.
Comment #60
greg.1.anderson commentedThis is super-awesome! I'm getting everything I want for Christmas this year.
Comment #61
jonhattanfinally committed!
for the record: DRUSH_PM_REQUESTED_PROJECT_NOT_PACKAGED means the project is from cvs/git and not a drupal.org package (already documented).