Install composer dependencies before running tests

This is already done on run.sh at line 242:

#install drush via composer on ${REPODIR}
if [ -f ${REPODIR}/vendor/drush/drush/drush ];
  then
    echo "Local Drush found on ${REPODIR}/vendor/drush/drush/drush"
  else
	cd ${REPODIR}
	curl -sS https://getcomposer.org/installer | php -- --install-dir=${REPODIR}
	${REPODIR}/composer.phar -d="${REPODIR}" global require drush/drush:dev-master
fi

Also, each docker php5.3/5.4/5.5 container installs it:

# Drush and dependencies
RUN /usr/local/bin/composer global require drush/drush:dev-master
RUN /.composer/vendor/drush/drush/drush --version

1) Great, so composer should already be available in /usr/local/bin/composer

2) The difference is that the quoted code only calls the 'require' command for Drush. The suggested addition here is to call the 'install' command before executing any tests.

What sun said ... the request here is that running "composer install" will have to be a setup build step in any simpletest test run.

Install composer for the testbot.
Install drush for the testbot through composer.
Eventually install phpcs and Drupal standard through composer.

For PHPUnit: composer install for each suite. Drupal currently runs PHPUnit separately for each test class, but really only do this once.

For SimpleTest: composer install for each fixture build, since at some point modules will be able to require stuff through composer.

Multiply by each cell in the PHP-version vs. database-type matrix.

So it's a good thing composer caches everything.

Simple re-roll.

Looks like this is core-only. That's good, if anything is needed for contrib can be a separate issue.

Synchronizing this with packaging script changes, and the actual change in core, will be more trouble than its worth. Will this alter the existing vendors directory if run with it checked in? Will this be okay with the vendors directory existing?

This shouldn't make any changes because we are committing composer.lock and running composer install (not update), it will install the current versions.

Yes, and it will not touch the files that are already there as long as they are in the repo. So once this is in and the packaging scripts have a similar treatment we can remove the dependencies from the core repo at any point in time and it will just work (famous last words).

The supplied patch is against /web/run.sh, which is no longer being executed ... the web container is started up with the apache daemon as PID 1, and other commands are passed into the container via the default job definition. run.sh should be considered deprecated.

What is needed is pretty trivial:

1. Fill in the empty buildComposerCommand() method in /src/DrupalCI/Plugin/BuildSteps/generic/Composer.php. Assume that this method gets passed the composer arguments as $data, and returns "composer [arguments]", which then gets passed to ContainerCommand->run() method (i.e. parent::run()).
2. Create a definition preprocessor class /src/DrupalCI/Plugin/Preprocess/variable/DCI_DrupalInstall.php. The purpose of this class will be to interpret the value of the environment variable DCI_DrupalInstall, and if the value of this variable is 'composer', insert a new entry into the job definition array. The inserted entry should look as follows:
   

   $definition => array(
     [... other build steps ...]
     'setup' => array(   [... setup steps ...]  ),
     'install' => array(
         'composer' => 'install',
     ),
     'execute' => array(  [... other build steps ...]  ),
     [... other build steps ...]
   

   Note that the key order in this array is important, so it may be worth re-ordering the array to make sure that the "install" array gets inserted at the right position before the 'execute' array. (Right now, it's hard-coded there for the simpletest job, but we're doing that by adding an empty key into the job definition, and that feels a bit fragile for future job types.)

3. Have drupal.org (or more accurately, Jenkins) set the DCI_DrupalInstall variable when requesting a D8 test. When requesting a D7 test, the value of DCI_DrupalInstall should be something different; and trigger (as an example) a mock simple browser click-through of the installation process.

For the longer term, this example triggered a realization that, while we can use the variable preprocessors to modify the value of multiple variables based on a single input variable, we could probably benefit from the ability to modify a single input variable based on the value of multiple source variables as well. Right now, we can't build preprocessor logic around the combination of 'DCI_CoreRepository' AND 'DCI_CoreRepositoryBranch' ... which is essentially what we need here to be able to distinguish between D7 and D8 tests, and insert/remove the composer step accordingly.

While lacking this capability, it may be possible for us to fake things out with an interim variable instead of the above ... so an alternative for #3 above could be:

1. DCI_CoreRepository preprocessor logic sets the first part of a DCI_IsDrupal variable string to 'drupal',
2. DCI_CoreRepositoryBranch preprocessor logic sets the second part of a DCI_IsDrupal variable string to '7' or '8',
3. DCI_IsDrupal definition preprocessor adds the $definition['install'] key as defined above into the job definition (along with any other Drupal-specific job definition additions we might need to make).

The biggest challenges with this approach are:

1. we can't guarantee which sequence the DCI_CoreRepository and DCI_CoreRepositoryBranch plugins will run in, so the logic in each will need to accommodate the plugins running in either order,
2. when processing each of the DCI_CoreRepository/DCI_CoreRepositoryBranch variables, we do not have visibility to the other variable. We might know that the branch is '8.0.x', but we can't know for sure that the repository is the core drupal repository.
3. We will also want to be able to run the Drupal install step for forks/branches/local versions of the repository, so we can't make assumptions about what set of DCI_CoreRepository values actually are legitimately Drupal codebases.

Given these issues (especially the last one), I don't see an easy way around setting a standalone 'DCI_IsDrupal' flag; and if we're doing that, we might as well just set 'DCI_DrupalInstall' directly, until we have a second need to key off such a flag.

Having a go based on @jthorson's comment.

Giving this a go too.

Aiming to get a patch within the next hour with a rough shell of what's needed as a base for others.

The TODO: item in /src/DrupalCI/Plugin/BuildSteps/generic/Composer.php suggests using composer programatically like http://stackoverflow.com/questions/17219436/run-composer-with-a-php-scri... I'm not sure this will work.

Honestly, not really sure what I'm doing, but followed @jthorson's instructions to make a start.

Instead of hardcoding 'install', I'd put $data at the end of that line ... that way, we can pass in (as an example) array('composer' => 'install') or array('composer' => 'update') and process both with the same plugin.

Your 'src/DrupalCI/Plugin/Preprocess/variable/DrupalInstall.php' should then be at 'src/DrupalCI/Plugin/Preprocess/definition/DrupalInstall.php' ... look at http://cgit.drupalcode.org/drupalci_testbot/tree/src/DrupalCI/Plugin/Pre... for an example as to how we insert additional information into the 'job definition' array with a definition preprocessor class.

The 'value' of the ['composer' => 'install'] array is what gets passed into Composer->run(), inside of the $data parameter.

Had a feeling it needed to be 'definition' and not 'variable', but went off the info in #11.

Updated the patch with guidance from #18. Think I'm still a way off because not 100% sure how it fits together, but feels like progress.

+++ b/src/DrupalCI/Plugin/Preprocess/definition/DrupalInstall.php
@@ -0,0 +1,27 @@
+    $composer_commands = array('install', 'update');
+    if (in_array($value, $composer_commands)) {
+      $definition['install']['composer'] = $value;
+    }
+    else {
+      $definition['install']['composer'] = 'install';
+    }

I think the if in this case can be shortened to $value == 'update', no?

@tstoeckler - Re: #20, yes, you're right, but I was thinking of more future proof concept where we might want to allow composer clearcache or composer dumpautoload further down the line, but always fall back to install is $value is not one of the given commands.

Was initially going to do !empty($value) in the if, but $value = 'foobar'; would cause issues ;). So thought it needed validation.

Ahh, sure, that makes sense. Thanks for the explanation!

Looks close.

1. We should make that $install['composer'][] = $value, so that we don't overwrite any existing composer commands which could be included in the job definition. Not really relevant for the Drupal.org and core case, but leaves it open for developers to add composer steps directly into a custom job definition.
2. I like having the validation there, but the in_array() check prevents us from being able to add additional composer arguments through the job definition, or alternatively, pass in a single dependency. It would be nice to be able to support both ['composer' => '-V'] or ['composer' => 'update monolog/monolog'] alternatives as well.
3. Are the --working-dir core and --prefer-dist arguments drupal-specific? If so, I'd prefer not to hard-code it into the plugin like this ... but we can for now as long as we add a 'TODO:' to de-drupalize it for a more generic use case later. The value for --working-dir, at the very least, is going to need to be dynamic in the long run, so that we can also support the 'contrib testing' use case. I'm not sure the best way to do this without putting a bit of extra thought into it; but the tools we have available are:
   1. Job 'variable' preprocessors: Based on the content of one DCI_* variable, these modify the values of other DCI_* variables. The $plugin->process() method will be executed once for each target DCI_* variable returned by the $plugin->target() method.
   2. Job 'definition' preprocessors: These run after the variable preprocessors. Based on the content of one DCI_* variable, these modify the values of the job definition array (passed in via $definition). The job definition array contains the list of build steps and associated values which define the actions to be taken by the test runner.
   3. Default job definition: The src/DrupalCI/Plugin/JobTypes/simpletest/drupalci.yml default job definition template contains steps common to all simpletest jobs. This file provides the initial $definition array, which is then modified by the job definition preprocessors.
   4. %DCI_variable% substitution: As the last step of the job definition processing, after each of the job definition preprocessors have run, the final job definition will be parsed and any %DCI_variable% placeholders are replaced with the value of their associated DCI_* variable. (Note that this means that %DCI_variable% placeholders can be added to the job definition inside of a job definition preprocessor, and the end result will undergo variable substitution.)

Made this more flexible so anything can be passed to the composer command, but the default (if nothing else is defined) is --working-dir core --prefer-dist install.

I agree contrib testing is going to be complex.

Looks like you uploaded the same patch as #19. :)

Let's try that again, the interdiff in #24 was valid for this patch.

I spent quite some time today thinking about this and regarding #23.3, we basically have two options:

1) assume that different core major versions will have their own job type (e.g. 'simpletest7' and 'simpletest8') and therefore separate templates, in which case we could just add:

install:
  composer: install --working-dir core --prefer-dist

to the job definition template for Drupal 8 since that will be needed for both core and contrib testing.

2) take the approach suggested in #11 and introduce a DCI_DrupalInstall variable and an accompanying definition preprocessor.

I really liked the simplicity of option 1), but I think @jthorson is more in favor of 2), so the patch attached builds on top of #26 and puts everything into place :)

Thanks guys! I committed the ComposerCommand changes to our 'dev' branch, with the intent that the DA staff can merge that into production at a time that is convenient.

For the DrupalInstall class, my thought was that DCI_DrupalInstall would be used to define what installation method we might want to use for a given test run ... with potential options including 'composer', 'drush', or 'browser' (i.e. simulated click-through). This approach gives us greater flexibility in how we set up the site under test, and in doing so, better test coverage of multiple installation methods.

However, as I was taking another stab at the definition preprocessor, I realized that it would be a trivial change for us to expose the complete set of DCI_* variables to the definition preprocessors, and that gives us greater flexibility when defining our preprocessor logic ... so with that change, we can look at things like DCI_CoreRepositoryBranch to determine our Drupal core version, rather than having to pass it in via the definition file.

The resulting code can be viewed in the respository, inside the 'dev' branch. It also contains the skeleton code structure for a process_drush_install() method, which (once populated) will unblock D7 simpletest testing as well.

I see this hasn't made it's way to the production branch yet. Any timescale on that?

In -dev ... just needs production merge. DA folks? Can I merge this?

I've been reviewing the code, and there is a misunderstanding of what composer install does and what installing drupal with drush/browser clickthrough does. composer install downloads the rest of the required dependencies, but it does not result in an 'installed' drupal site, it just makes sure that all the files that are required by the application are there. So we don't want these coupled together, as composer install is not a 'drupal installation method', it is a step than needs to happen before drupal can be installed. (Fetch all the dependencies, then install drupal).

This is an additional challenge with working with a 'dev' branch. There are portions of what has been added to the dev branch that are usable now, and portions that need work. We could merge in the features that are ready if they were separate (i.e. in their own feature branches), but as it is, the dev branch is a combination of composer support, and d7 installation support. This makes it difficult to review, and approve these separately, when they aren't related at all. It would be ideal to eliminate the dev branch and do all further work on feature branches.

Yes, the dev branch is a combination of composer support and d7 support ... because the work was based on the (obviously flawed!) assumption that 'composer install' was an install step, which makes the two essentially a single feature. :p I've stripped out the composer logic, and am moving it to it's own definition preprocessor.

I disagree with the suggestion that we get rid of the dev branch. I'm not really interested in a 'branch per patch' development approach. Outside of the one commit which added a new class in it's entirety, the largest diffstat I've added on that branch is -9/+29 ... that really can't be as hard to review as you're suggesting.

As for 'review and approve' ... really, I'm just asking you to 'test' then 'deploy'. ;)

Updated the composer logic in the -dev branch. For now, DCI_ComposerInstall=true will trigger the composer install command as the first step in the 'setup' build step. (Technically, it moves any existing 'composer' key to the first element of the 'install' build step array, and then adds the 'composer install' command to the end of the ['install']['composer'] array.)

Nix #33 ... I just moved the composer install step to a 'pre-install' build step, which is where it actually belongs. This also greatly simplified the definition preprocessor logic.

Feature branches are how all of our work is managed internally, because it makes it so that we can keep changes atomic and separate from each other. The size of the diffstat isn't an issue, its the fact that multiple, separate changes are mixed into one changeset. So, in this example, I couldn't commit the d7 drush install fixes because the composer functionality still needed work. If we were working with patches, it wouldn't be acceptable to put multiple features into one patch either.

So, for example, this feature would have been held up on this commit: http://cgit.drupalcode.org/drupalci_testbot/commit/?h=dev&id=166fca337ee... because none of the jenkins jobs are currently setting DCI_Runoptions, and --keep-results is required to have any xml results, therefore I cant merge the whole dev branch in (which I see contains additional fixes, like the php version number - cool) until we've got the time to change those jenkins job configs.

Anyhow, I went ahead and backed that change out of the drupalci.yml file until we can make it happen on the jenkins jobs.

https://dispatcher.drupalci.org/job/DCI_Run/34/consoleFull shows composer installing, and I got into the container and verified that the only changes (according to git) were expected.

I'll merge in the dev branch into prod. I'll still have to modify the other jenkins jobs to incorporate the composer install variable before this will do anything.

So awesome to see this running.

Thanks for the work.

It looks like from the patch https://www.drupal.org/node/1475510#comment-10291471 that this is not deployed / working on production yet.

The patch failed due to Failed opening required '/var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/autoload.php' therefore core/vendor must not be getting added by composer during testing.

looks like the patch passed as far as I can see. are you referring to the qa.drupal.org bots or the drupalci.testbots?

ah, I had wondered why is said pass and fail, guess a misunderstanding on my part.

thanks again for the awesome work mixologic!

Looks like we can mark this as fixed.

Is this also resolved for contrib modules that use composer in Drupal 8? Or do we have a separate task for tracking that?

We need a separate task to track that because core and composer a lot more complex.

So far, this will only execute core's composer file.

There's no decided-upon way to deal with contrib composer dependencies. Start here to participate: #2002304: [META] Improve Drupal's use of Composer

In order to get #2380389: Use a single vendor directory in the root in, we needed to disable this feature temporarily as it is running composer from the /core directory. Once that core issue makes it in, we can re-enable it, and run it from the root directory. Once that is happening we can then remove the files from /vendor.

Before we turn this back on, we should probably make sure that the github oauth tokens are securely fastened. - Adding a parent issue.

#2380389: Use a single vendor directory in the root is in.

I've made a placeholder issue for contrib: #2597778: Install composer dependencies for contrib projects before running tests

I've worked on installing with composer, it seems to be working.

It'll happen after the fetch and patch, and is part of the setup steps.

I created a patch that removed the /vendor dir and ran it in the staging job

failures due to github having issues

• https://dispatcher.drupalci.org/job/staging_simpletest_job/282/console
• https://dispatcher.drupalci.org/job/staging_simpletest_job/284/console

success

• https://dispatcher.drupalci.org/job/staging_simpletest_job/285/consoleFull
• https://dispatcher.drupalci.org/job/staging_simpletest_job/286/consoleFull

@isntall - Great work, is this ready to be deployed?

This has been deployed and DCI_ComposerInstall=True is now the default.

Nice.

If you add --no-progress it won't show all the funky percentage updates.