Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#42 | install_composer-2315545-42.patch | 1.4 KB | timmillwood |
Comments
Comment #1
andyceo CreditAttribution: andyceo commentedI 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.
Comment #2
kim.pepperNot sure about the self-update. There is still a lot of instabilty with composer releases, and we've had issues in the past.
Comment #3
andyceo CreditAttribution: andyceo commentedWell, 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).
Comment #4
drummcomposer 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.Comment #5
drumm#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:
Comment #6
drummLooks 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).
Comment #7
bojanz CreditAttribution: bojanz commentedIt 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.
Comment #8
drummDo we need a whitelist like we have for make files? See also #1856762: Revisit and Redefine Drupal's Policy on hosting 3rd party files and/or files that is not strictly GPL.
Comment #9
andyceo CreditAttribution: andyceo commentedVoting +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..
Comment #10
XanoThis 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.
Comment #11
catchThis also needs --no-dev, so we avoid a classmap like https://api.drupal.org/api/drupal/core!vendor!composer!autoload_classmap...
Comment #12
bojanz CreditAttribution: bojanz commentedI 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.
Comment #13
Mile23I'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 acomposer 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 insteadFinal
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 ofcore/
.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.
Comment #14
YesCT CreditAttribution: YesCT commentedComment #15
timmillwoodAnother thing we need before this goes in is testbot to install composer dependencies when running tests.
Comment #16
timmillwoodComment #17
davidwbarratt CreditAttribution: davidwbarratt as a volunteer and commented#16,
Doesn't the
--working-dir
(or-d
) need to be set tocore
?https://getcomposer.org/doc/03-cli.md#global-options
Comment #18
timmillwood@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.
Comment #19
davidwbarratt CreditAttribution: davidwbarratt as a volunteer and commented#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.Comment #20
timmillwoodWhat was the reasoning behind setting up drupal/core in /composer.json, when drupal/core isn't really a thing?
Comment #21
davidwbarratt CreditAttribution: davidwbarratt at Golf Channel commented#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.Comment #22
timmillwoodTaken 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
Comment #23
dawehnerI'm curious whether this code runs on every packaging for contrib as well?
I'd kill to see this ...
Comment #24
davidwbarratt CreditAttribution: davidwbarratt at Golf Channel commented#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
Comment #25
timmillwoodright, 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.
Comment #26
timmillwoodUpdating this patch to also run "--optimize-autoloader" on install
This addition will give the same result as the patch in #2539220: Updating autoload_classmap
Comment #27
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI 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.
Comment #28
Mile23Ugh.. sorry. Got this issue mixed up with #2315537: Install composer dependencies before running tests. Please ignore this comment. :-)
Comment #29
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@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.
Comment #30
cweagansI 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.
Comment #31
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@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.
Comment #32
cweagansFair 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.
Comment #33
davidwbarratt CreditAttribution: davidwbarratt at Golf Channel commented#30,
Please see #21 for why we can't do that. :)
Comment #34
timmillwoodI 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.
Comment #35
timmillwoodFurther 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.9Comment #36
timmillwoodAttached 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 outputComment #37
timmillwood@drumm - Is there anything I can do to help get this deployed? What are the blockers?
Comment #38
drummLet's not add
'composer' => '/usr/local/bin/composer'
. That isn't really configurable, except for extending the class, which is impractical. Invokingcomposer
directly and letting$PATH
take care of finding what to run is okay.Comment #39
timmillwoodTaking the suggestion from #38 and removing
'composer' => '/usr/local/bin/composer'
in favour of invokingcomposer
directly.Comment #40
timmillwood@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.
Comment #41
MixologicThis 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
Comment #42
timmillwoodUpdating to match the deployed patch from #2315537: Install composer dependencies before running tests.
Comment #43
Mile23Relevant bug: #2579663: Can't use 'composer install' with missing composer.lock and vendor folder
Comment #45
drummI committed #42 with a little code style change, and one substantive change: replaced
$this->release_node->language
withLANGUAGE_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.Comment #47
drummAnd one more commit to bring back
--ignore-platform-reqs
Comment #48
drummThis has been deployed. Thanks everyone!