The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.I've been testing out various .make file functionality for http://drupal.org/community-initiatives/drupalorg/distribution-packaging
I tried this:
core = 7.x
api = 2
projects[update_test_module][type] = module
projects[update_test_module][download][type] = git
projects[update_test_module][download][revision] = 377964f102b9565bd7bafff30f70e35e85a53590
projects[update_test_module][download][branch] = 7.x-6.x
Since I'd like to be able to specify both a revision and a branch for #1371306: Add validation to ensure that if a .make file includes a git hash, it also defines a branch.
All I see is this:
% drush make --no-core test.make foo
update_test_module cloned from [ok]
git://git.drupal.org/project/update_test_module.git.
% echo $!
0
% ls -al foo/sites/all/modules/update_test_module
total 0
drwxr-xr-x 2 dww dww 68 Feb 9 08:55 ./
drwxr-xr-x 3 dww dww 102 Feb 9 08:55 ../
So, make failed to do what I asked it, but didn't tell me anything or actually return non-0. Looking deeper, I found this in the code:
function make_download_git($name, $download, $download_location) {
...
// more than one option is set so we throw a error message
elseif ($download['branch'] !== 'master' || $download['tag'] || $download['revision']) {
make_error('DOWNLOAD_ERROR', dt("You can only specific branch or tag or revision but not combined in make file."));
return false;
}
...
That 'DOWNLOAD_ERROR' is never showing up. If I add a drush_log('Badness 10000', 'error') right before that return false, I see it in the output, so I know that's the error case I'm hitting.
Based on my limited understanding of drush internals, make_error() seems like it should be working:
/**
* Logs an error unless the --force-complete command line option is specified.
*/
function make_error($error_code, $message) {
if (drush_get_option('force-complete')) {
drush_log("$error_code: $message -- build forced", 'warning');
}
else {
drush_set_error($error_code, $message);
}
}
Sooo... WTF? ;)
A) Why doesn't make_error() seem to print anything? At least not here. In other places, it does, like if you don't specify a 'core' attribute in your make file, you see this message:
if (empty($info['core'])) {
make_error('BUILD_ERROR', dt("The 'core' attribute is required"));
$errors = TRUE;
}
B) Why don't we die here with a non-0 exit code?
Is it something funky with the concurrency stuff? The download happens concurrently and therefore, the output is lost and the fact we hit an error is ignored since that state gets set in a forked child process and not propagated to the parent?
This seems like a major DX WTF for people trying to use .make files and getting various forms of validation errors and stuff. As more folks try to use drush make for distribution packaging on d.o, it seems essential that we find and successfully propagate errors. I'd love to help resolve this if I knew how. ;)
Help,
-Derek
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | 1434376-2.drush-make-integrate-true.patch | 1.17 KB | dww |
Comments
Comment #1
greg.1.anderson commentedIs make_download_git being called via drush_invoke_process (or any other function that calls through to drush_backend_invoke)? I think the problem here is that the default value for $integrate is confusing. In Drush-3, this value pretty much always defaulted to TRUE, but in Drush-4 I introduced some commands where it defaulted to FALSE. When the various invoke functions were simplified in Drush-5, I think this situation became confused.
I think we should go back to defaulting $integrate to TRUE everywhere. I don't know what this does to Drush-4 vis-a-vis backwards compatibility, but perhaps we can change the Drush-4 version of drush_invoke_process in this respect on the theory that this API is provided as a "forward compatible" call -- old code probably isn't calling it, and new code will expect it to behave like Drush-5. I'm not sure about our policy interpretation here.
The upshot for Drush make, though, is that $integrate should always be TRUE. If $integrate is FALSE, that means that the caller is expected to examine the return value of the backend invoke and handle errors directly. If it is TRUE, then errors are displayed and integrated into the current process, so if the child calls drush_set_error, the parent process will in turn also call drush_set_error when the child returns.
This will take a little investigation time. See also #1431594: Drush-4 should handle backend set results the same way that Drush-5 does.
Comment #2
dwwSweet! Thanks for the pointer. Quick testing is that the attached is working great.
I should probably do something more extensive with other .make files to ensure this doesn't introduce any problems.
Comment #3
dwwHrm, evil. With this patch, one of the make tests now fails:
Not sure if that's a bug in make or in the test. I'll dig now. Either way, we can't commit this patch as-is.
Comment #4
moshe weitzman commentedI agree that we can change drush_invoke_process() in 4.x to be forward compatible.
Comment #5
dwwUhh, weird. When I run drush make interactively on the failed test .make file, it works fine. With and without the patch, I get the same build hash as the test expects. So I ran the make tests again with the patch applied and got... different results. :/ Yay for non-deterministic tests.
And the third time (with the patch) I get the same failure I reported in #3. And the forth time, a combination of the two:
Fun,
-Derek
Comment #6
greg.1.anderson commentedRegarding the patch in #2, I think that 'integrate' was set to false there to reduce the amount of output that Drush make produced. Varying the output of 'integrate' based on the value of 'verbose' or 'debug' switches is definitely wrong! I should have caught that when that code went by for review the first time, but I did not.
We need to insure that 'integrate' is always TRUE here. We may need to add a new option to backend invoke that has the same affect as 'integrate' => FALSE does on log output (maybe excepting drush_set_error output?) so that the log output from Drush make can come out as desired.
Comment #7
RichardLynch commentedJust skimmed, but personally, I'd do an error_log of the drush-get_option('force-complete') and see if that's the deciding factor...
Comment #8
jhedstromThe reason integrate is currently set to FALSE, goes back to #1378992: Clean-up drush make output. With integrate set to TRUE, we go back to really verbose output. There are other issues that may require make handle output or return values from sub-invocations (for instance, solving #1016924: Dealing with different versions of the same module in recursive makefiles will potentially require than recursive invocations return structured data to the parent for comparison of versions), so I'm not entirely opposed to some light handling of errors.
Comment #9
jhedstromAfter talking with dww in IRC, I'm going to attempt to bring back the idea of a
--quiteparameter to the pm-download command, rather than leaving integrate off, which suppresses everything.Comment #10
jhedstromActually, setting integrate to TRUE doesn't impact the output of drush make. The translation issue failing was unrelated and has been fixed (it was a race condition wherein one translation would wipe out the l10n server for another that was never caught when we switched to processing projects concurrently).
Comment #11
jhedstromNow I remember why integrate was set to false. Without that, we get double output of downloads:
(from building git-simple.make)
Comment #12
jhedstromClosing this since #1417020: pm-download output for contrib projects duplicated when called from drush make is open to track the duplication issue.