Over in #1475510: Remove external dependencies from the core repo and let Composer manage the dependencies instead we are discussing moving the composer managed files out of the git repo. This requires some changes to packaging, as it will need to call composer install before creating tarballs and zip files.

This is an initial attempt.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andyceo’s picture

+++ b/drupalorg_project/plugins/release_packager/DrupalorgProjectPackageRelease.class.php
@@ -8,6 +8,7 @@ class DrupalorgProjectPackageRelease implements ProjectReleasePackagerInterface
+    'composer' => 'composer.phar', // I have no idea where this is installed!

I think we should install composer to /usr/local/bin/composer, because this is the ordinary place for the non-repo software. Owner and group of the file: root, permissions: rwxr-xr-x

Also, every time testbot is run, we should do 'sudo composer self-update' before installing something with composer.

kim.pepper’s picture

Not sure about the self-update. There is still a lot of instabilty with composer releases, and we've had issues in the past.

andyceo’s picture

Well, we should either freeze composer version we can provide support for, or do 'sudo composer self-update' every time we run testbot, because users will install dependencies with last composer. And composer always remind to user do 'composer self-upgrade' if it older than 30 days (or 15, I don't remember).

I think freezing is more stable option. In this case, we can just provide another 'bleeding edge' testbot with fresher environment ('sudo composer self-update', and other soft updated).

drumm’s picture

composer is installed at /usr/local/bin/composer, however I think we may want to stop using full paths for executables and expect $PATH to be set up correctly.

drumm’s picture

Status: Active » Postponed

#1475510: Remove external dependencies from the core repo and let Composer manage the dependencies instead doesn't seem to have agreement yet, so I'm postponing this issue.

For when this does want to move forward:

  • Is this core-only, or good for every project packaged on Drupal.org?
  • Will having this in place before dependencies are removed from Git have unpredictable effects on packaging? Such as packaging versions of dependencies that core maintainers didn't explicitly include?
drumm’s picture

Looks like this would also make HTTP requests to GitHub servers. It could run into #2304983: 'wget' used by packager has trouble downloading certain files from GitHub (like TinyMCE).

bojanz’s picture

Is this core-only, or good for every project packaged on Drupal.org?

It would need to work with every project.
More and more modules require external libraries (Commerce 2.x included), having them predownloaded and prepackaged would
provide a much better experience to end users.

drumm’s picture

andyceo’s picture

Is this core-only, or good for every project packaged on Drupal.org?

Voting +1 for the "every project" feature. In the project I am doing now, we definitely need 3-d party bundles and libraries, installed by composer.

May be it would be nice these bundles can be installed on module enable..

Xano’s picture

This works on the application level only, meaning it works for core, but not for contrib, because if we do it for contrib we'd end up with multiple projects' *.zip and *.tar.gz files including the same third party libraries, which isn't just duplicate code, but can also cause version conflicts. A current solution that almost works for contrib is Composer Manager.

catch’s picture

+++ b/drupalorg_project/plugins/release_packager/DrupalorgProjectPackageRelease.class.php
@@ -142,6 +143,12 @@ class DrupalorgProjectPackageRelease implements ProjectReleasePackagerInterface
+    if (!drush_shell_cd_and_exec($this->temp_directory, "%s --prefer-dist install", $this->conf['composer'])) {

This also needs --no-dev, so we avoid a classmap like https://api.drupal.org/api/drupal/core!vendor!composer!autoload_classmap...

bojanz’s picture

I now agree with #9 and #10.
Each site needs to have a single vendor/ directory. For us, that's core/vendor.
So this feature must be limited to Drupal core.

Mile23’s picture

Title: Install composer dependencies when packaging » Install composer dependencies to D8 when packaging
Status: Postponed » Needs work

I'm taking the liberty of limiting the scope of this issue a little, as per #9, #10, etc. Basically the user should be installing dependencies using composer. Since core is supposed to be useful to people who don't know how to use composer, however, we have to build the vendor directory for them in the tarball.

I think this will be pretty stable, in no small part because composer caches all of the packages it downloads. https://getcomposer.org/doc/03-cli.md#composer-cache-dir

There are also tools such as Satis to host packages locally, should the need arise.

So for D8 we can move in a few phases:

Experimentally

Currently the repo has core/vendor. This means that we could experimentally add a composer install phase *after* the tarball is created, in order to check stability by reading logs.

Actual

Remove core/vendor from the repo and depend on the packing script to use composer to install dependencies. This should sound alarms when it doesn't work. See: #1475510: Remove external dependencies from the core repo and let Composer manage the dependencies instead

Final

Hopefully, we'll move the vendor directory to the root directory where it belongs, for all kinds of reasons: #2380389: Use a single vendor directory in the root This will necessitate changing this script *again.* But it should be a minor change, issuing the composer install command in the root dir, instead of core/.

If I knew how to work a patch for d.o's infrastructure, I'd do that, but the original patch looks like it does the right thing, I guess... Except that it does need to cd into core/ before installing dependencies initially, and then not cd into core as we progress through the phases above.

YesCT’s picture

Issue tags: +Composer
timmillwood’s picture

Another thing we need before this goes in is testbot to install composer dependencies when running tests.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
1.21 KB
davidwbarratt’s picture

#16,

Doesn't the --working-dir (or -d) need to be set to core?
https://getcomposer.org/doc/03-cli.md#global-options

timmillwood’s picture

@davidwbarratt only if we want the vendor directory to be in core, that still seems to be up in the air.

If we do decide it will be in core still then IMHO that should be set in composer.json, and not in the composer install command.

davidwbarratt’s picture

Status: Needs review » Needs work

#18,

I guess since this is packaging it should be fine, I'm just afraid that the drupal/core package will not be up to date when the packaging script runs.

See https://packagist.org/about

basically if packagist hasn't updated drupal/core yet, then what will be packaged is the previous version of drupal/core not the current version.

A way we could get around this is by creating a webhook for packagist, but we'll also need to add a check to see if it's been updated or not.. you could hit this endpoint:
https://packagist.org/packages/drupal/core.json
to see if it's been updated or not, but if it hasn't I'm not sure what would happen except for fail completely. :(

The only alternative in my mind is to set the working directory on install. You can't do this in the root composer.json because you run into the same problem I've just described.

timmillwood’s picture

What was the reasoning behind setting up drupal/core in /composer.json, when drupal/core isn't really a thing?

davidwbarratt’s picture

#20,

Long Answer:
In the beginning there was ./core/composer.json #1424924: Use Composer for updating Symfony components (without removing Symfony code from repo) and it was good. One day, ./core/composer.json became ./composer.json #1805316: Move composer.json to the project root and the world was sent into chaos and confusion. Then the people came together and moved ./composer.json back to ./core/composer.json #1975220: Allow a Composer user to manage Drupal, modules, and PHP dependencies with a custom root composer.json and setup a subtree split #2352091: Create (and maintain) a subtree split of Drupal core and there was much rejoicing. One day the people hope to #2385387: Permanently split Drupal and Drupal core into seperate repositories and the world will be perfect, but that day is not today, so the people live in the hope of the future to come.

Short Answer:
drupal/core is certainly a "thing" in the same way that every Symfony component is a "thing". It's a thing because 1) it has it's own repository and 2) it has it's own composer.json file.

Basically, it's the lesser of two evils. Several people on #1975220: Allow a Composer user to manage Drupal, modules, and PHP dependencies with a custom root composer.json argued that it should not be a thing (for some legitimate points), but in the end, the lesser of two evils is that drupal/drupal and drupal/core are different things. Ultimately, it makes it a lot easier. The user doesn't have to deal with the drupal/core dependencies and making sure they are in sync. They can use Composer to update Drupal core and all of it's dependencies. To do it the other way around would mean that you could have a ./core and a ./vendor that are incompatible. If you use Composer with the drupal/drupal template, then that becomes impossible.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
1.23 KB

Taken on @davidwbarrett's suggestion of adding --working-dir.

Also submitted a similar patch for testbot #1923582: Add ability for testbot to run 'composer install' during installation

dawehner’s picture

I'm curious whether this code runs on every packaging for contrib as well?

I'd kill to see this ...

davidwbarratt’s picture

#23,

We can't do this for contrib, because if two modules required the same dependencies, you'd get a Fatal PHP error after installing both of them. The only way to install contrib modules with dependencies is doing it directly with Composer.

See #2477789: Use composer to build sites.

Also see the documentation on how to this here:
https://www.drupal.org/node/2404989

timmillwood’s picture

right, this cant work with contrib, contrib modules cannot ship with dependencies. We'll have to lean on thr composer manager module or composer command line.

Although not tested, i think this patch only works for core packaging.

timmillwood’s picture

FileSize
1.25 KB

Updating this patch to also run "--optimize-autoloader" on install

This addition will give the same result as the patch in #2539220: Updating autoload_classmap

amateescu’s picture

FileSize
1.59 KB
1.92 KB

I looked into this a bit and I don't think the patch from #26 would run only for core 8.x. Also, we shouldn't add --optimize-autoloader until #1818628: Use Composer's optimized ClassLoader for Core/Component classes is resolved in some way.

Updated patch attached.

Mile23’s picture

Ugh.. sorry. Got this issue mixed up with #2315537: Install composer dependencies before running tests. Please ignore this comment. :-)

amateescu’s picture

@Mile23, is your comment generic or I just don't get it? Because that's exactly what I did with the patch in #27 as opposed to the previous patch: limit the composer install to core and removed the classmap optimization.

cweagans’s picture

I don't think vendor should be located in core. There's some other issue open to move the vendor dir to be a sibling of index.php, and IIRC, there's strong consensus for that plan. We should just do that here.

amateescu’s picture

@cweagans, no, that's not in scope for this issue. That will have to be figured out in #2380389: Use a single vendor directory in the root and/or #2489646: Start using $root/vendor for non-core dependencies only, and we will have to change the drupal.org packaging part (i.e. this patch) accordingly when consensus is reached in one of those two issues.

cweagans’s picture

Fair enough. I didn't realize that 2489646 existed, so you're right - it's probably better to wait. It's an easy change later anyhow.

davidwbarratt’s picture

#30,

Please see #21 for why we can't do that. :)

timmillwood’s picture

I am currently testing the patch from #27 on https://package-composer-drupal.redesign.devdrupal.org.

There are two issues I have faced so far:
- --working-dir needs an absolute path [resolved]
- It is trying to build 8.1.x which doesn't have a composer.json in /core, someone needs to rebase 8.1.x branch from 8.0.x, or we need to check for a composer.json before running.

timmillwood’s picture

Further issue, d.o dev server is running php 5.4.44 (production is even older) therefore composer refuses to run composer install because D8 needs php 5.5.9

timmillwood’s picture

FileSize
1.72 KB
1.74 KB

Attached patch has been tested on https://package-composer-drupal.redesign.devdrupal.org

Differences to 27:
- Added absolute path to core directory
- Checking the core directory has a composer.json (8.1.x branch doesn't and was causing error)
- Added --ignore-platform-reqs to ignore the fact that d.o server isn't running php 5.5.9
- Add --quiet to produce a little less output

timmillwood’s picture

@drumm - Is there anything I can do to help get this deployed? What are the blockers?

drumm’s picture

Status: Needs review » Needs work

Let's not add 'composer' => '/usr/local/bin/composer'. That isn't really configurable, except for extending the class, which is impractical. Invoking composer directly and letting $PATH take care of finding what to run is okay.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
1.55 KB
1.45 KB

Taking the suggestion from #38 and removing 'composer' => '/usr/local/bin/composer' in favour of invoking composer directly.

timmillwood’s picture

@drumm - Does the patch in #39 work for you? I'm wondering if there's a way we can ensure composer is available via $PATH.

I'll be in Barcelona next week if you want to talk any of this through.

Mixologic’s picture

Status: Needs review » Postponed
Parent issue: » #2578765: Setup GitHub OAuth for Composer during Packaging

This both needs work (since we moved things into /vendor) And is also postponed until we get a github oauth token in place. We dont want packaging to choke if it has to download a bunch of git clones and our api usage gets cut off. This can unpostpone when the parent issue is closed

timmillwood’s picture

Status: Postponed » Needs review
FileSize
1.4 KB

Updating to match the deployed patch from #2315537: Install composer dependencies before running tests.

Mile23’s picture

  • drumm committed b9f13aa on 7.x-3.x, dev authored by timmillwood
    Issue #2315545 by timmillwood, amateescu, kim.pepper: Install composer...
drumm’s picture

Status: Needs review » Fixed
Issue tags: +needs drupal.org deployment

I committed #42 with a little code style change, and one substantive change: replaced $this->release_node->language with LANGUAGE_NONE. That was changed throughout the file when we enabled translation for the Drupal 8 release announcement and quite a few things broke, including packaging.

  • drumm committed 066e91c on 7.x-3.x, dev authored by timmillwood
    Issue #2315545 by timmillwood, amateescu, kim.pepper: Add --ignore-...
drumm’s picture

And one more commit to bring back --ignore-platform-reqs

drumm’s picture

Issue tags: -needs drupal.org deployment

This has been deployed. Thanks everyone!

Status: Fixed » Closed (fixed)

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