pm-download sets a 'soft' error during failure [#1561666]

The test failure in recursion.make caused by #1561654: pm-download fails for --variant=profile-only fails without displaying an error, even though the backend pm-download command (drush dl drupal_forum) does throw an error:

drush make --no-core /home/jhedstrom/work/contributions/utilities/drush/tests/makefiles/recursion.make . --contrib-destination=profiles/drupal_forum --md5
 >> Project views contains 2 modules: views, views_ui.
 >> views-7.x-3.0-rc3 downloaded.                                                                         [ok]
 >> Install location /tmp/make_tmp_1336160917_4fa4329588c00/__build__/profiles/drupal_forum already exists. Do you want to overwrite it? (y/n): y
 >> drupal_forum-7.x-1.0-alpha2 downloaded.                                                               [ok]
Build hash: d41d8cd98f00b204e9800998ecf8427e                                                              [ok]

Comment	File	Size	Author
#4	set_error.patch	790 bytes	moshe weitzman

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

jhedstrom

English

Portland, OR

CreditAttribution: jhedstrom commented 4 May 2012 at 23:29

Component:

PM (dl, en, up ...)

» Make

Actually, I think this is a problem with the way make is parsing the return data from drush_invoke_process().

Comment #2

jhedstrom

English

Portland, OR

CreditAttribution: jhedstrom commented 4 May 2012 at 23:34

Title:	drush_backend_invoke_concurrent() not propagating errors	» drush_invoke_process() not propagating errors
Component:	Make	» Base system (internal API)

Hmm, no, unless I'm missing something, both 'error_status' and 'error_log' are empty on the failed call, even though there is a log of type error in the 'log' return.

Comment #3

Steven Jones CreditAttribution: Steven Jones commented 6 May 2012 at 05:41

Seeing this problem with Aegir too.

Comment #4

moshe weitzman CreditAttribution: moshe weitzman commented 9 May 2012 at 19:35

Title:	drush_invoke_process() not propagating errors	» pm-download sets a 'soft' error during failure
Assigned:	Unassigned	» jonhattan
Status:	Active	» Needs review

File	Size
set_error.patch	790 bytes

The problem isn't error propogation per se, the problem is that pm-download is setting an error with drush_load('foo', 'error') instead of drush_set_error(). When you do that, you don't properly set exit code to 1 and backend calls think you finished successfully.

The attached patch changes pm-download to error immediately upon any download failure. I'm not sure why we decided to use this soft failure. Anyone remember? The hard error in this patch might be too big a change for drush5? I'm not sure what the repercussions are.

I'd love to hear from Jonhattan and Greg if they have ideas.