Note:

See #121: "Summary from core committer discussion: everyone is on board with this change happening. However, it should not block D8's release."

Problem/Motivation

Drupal 8's repo includes all the Composer-based dependencies. This makes many operations cumbersome, such as patches which update libraries.

It also reflects a no-build testing strategy which is sub-optimal.

Proposed resolution

Composer is an increasingly popular tool for project dependency management that auto-downloads stuff for you. We're already looking to use it for getting code into Drupal in the first place, and it lets us not keep duplicate copies of thousands of lines of code in our repo.

So: Remove the vendor directory from the repo. This will be easy to do, but will require some infrastructure changes:

Remaining tasks

User interface changes

Developers will need to regularly run composer install

API changes

Original report by [username]

Now that Drupal 8 is bringing in some of the Symfony components, we need a way to manage them. Composer is a PHP package management system which is heavily inspired by npm, and with Packagist, it provides great support for Symfony.

What this means for Drupal.org is having the the packager process composer.json file during the package build process.

drush dl composer
drush composer install

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task
Issue priority Normal
Prioritized changes The main goal of this issue covers many areas reduces the size of the git repo, makes updates easier, improves deployment and maintenance of sites but adding more emphasis on composer.
Disruption If managed correctly there should be minimal disruption. Firstly testbot needs to be updated to do composer install, and this would also need to be done when creating the zip/tarball packages.
CommentFileSizeAuthor
#222 1475510-223.patch12.41 MBalexpott
#222 1475510-223.for-review-do-not-test.patch516.73 KBalexpott
#183 1475510-183-for-review-do-not-test.patch497 KBclaudiu.cristea
#182 remove_external-1475510-182.patch11.34 MBtimmillwood
#180 remove_external-1475510-180.patch11.34 MBtimmillwood
#174 remove_external-1475510-174.patch11.34 MBtimmillwood
#165 1475510-165.patch11.34 MBhussainweb
#161 1475510-161.patch12.15 MBpfrenssen
#159 remove_external-1475510-159.patch12.15 MBhussainweb
#156 remove_external-1475510-156.patch12.08 MBhussainweb
#137 remove_external-1475510-137.patch17.39 MBhussainweb
#137 remove_external-1475510-137-edits-only.patch1.64 KBhussainweb
#135 remove_external-1475510-135.patch17.28 MBtimmillwood
#126 non_vendor_only.txt536 bytesMile23
#126 1475510_125_fail.patch17.24 MBMile23
#42 issues-user-os.png104.38 KBwebchick
#42 git-instructions-os.png103.85 KBwebchick
#81 no-vendor-1475510-81.patch832.38 KBdavidwbarratt
#83 no-vendor-1475510-83.patch14.31 MBdavidwbarratt
#94 1475510-94.patch17.92 MBtimmillwood
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

  1. The issue summary needs a clear Problem and Goal statement.
  2. Based on my wild guess of what is being proposed (and why), -1 on requiring any additional tools to make a Drupal core git clone/checkout work.
Crell’s picture

Problem: To use 3rd party code right now, we have to include it directly in our repo. That makes updates to it annoying to do as well.

Solution: Composer is an increasingly popular tool for project dependency management that auto-downloads stuff for you. We're already looking to use it for getting code into Drupal in the first place, and it lets us not keep duplicate copies of thousands of lines of code in our repo.

RobLoach’s picture

Thanks Crell, didn't really have the time to write a proper issue summary. I just copied and pasted that in for now, probably have some time tomorrow to write up more docs.

sun’s picture

So whenever I do a git pull you require me to do an additional

php composer --blah blub bleh

on the command line?

That's not going to fly at all for me.

git is supposed to control, ensure, and maintain the state of files. Not a separate tool that completely bypasses the version control mechanism.

I don't even know whether my git client would support that (TortoiseGit). Can't find an option for defining custom post-checkout/post-pull hooks. (which of course, require to have a working PHP CLI in the first place)

In turn, there'd be no guarantee that everyone works on the same revision of files.

This would make contributing to Drupal considerably harder.

Crell’s picture

You specify a version of the dependency in the composer.json file.

We're talking about 3rd party libraries; the Git-native option is git submodules, which few people like. :-)

If your git client doesn't support a post-pull hook, then it's Doing It Wrong(tm). (I'm pretty sure all git implementations do that.) That said, it's not something you should need to do with every pull since I don't expect us to be changing code in those libraries all that often.

dww’s picture

Status: Active » Postponed

Note that "building Core" is pretty ill-defined. There are at least 4 ways this happens, probably more:

  1. The d.o packaging script (what you're talking about here).
  2. Direct git clone
  3. drush make
  4. drush pm-download

Obviously, we could handle #1 here if there was agreement this is a good thing to do. #3 and #4 are do-able, as well, although that requires a separate issue and patch (at least both now live in drush core, so that slightly simplifies things).

#2 is the main thing that will be broken if we do this. That seems like a pretty big problem. I believe the testbot is just doing direct checkouts, so we'd need to teach it about this change, too. That also requires a separate issue if we go this route, since that code lives in PIFT/PIFR (I can never keep those straight).

In an ideal world, I agree it's better to just pull in 3rd party code at "build" time instead of stuffing it in our repo. However, this would be a pretty drastic change to basically cripple native Git clones without an additional step, and I don't think that's just a d.o infrastructure discussion to have.

The experience of the (cvs|git)_deploy modules is that effectively no one ever found out about this additional step if you just get our code directly from the VCS. A handful of people In The Know(tm) use them, but we still regularly get bug reports/support requests wondering why the Update manager is "broken" when you clone from Git. No one reads README files anymore, nor release notes, so I don't know what we can do to help teach people they need this step. Ideally, if you visited a D8 core site that hadn't taken this step we'd print a nice helpful page about it instead of WSOD. But, that's additional complexity and a performance hit in the lowest possible levels of bootstrapping the system, so I doubt that's going to fly. Maybe that's at least something the installer could know about. And maybe we can reuse the authorize.php/update manager plumbing directly in the installer to run Composer for you if you haven't already. This is totally off-topic here in the d.o infrastructure queue, but that's what I mean -- the non-d.o-tarball case is the one where this is going to create the most trouble, so I feel like we really need a discussion about that, first. Once that's agreed and decided, the actual fix to case #1 above is pretty trivial.

There is also the increased complication of development when it's less clear that everyone's actually running the same code, as sun points out. While this isn't a show-stopper, it is a concern. Perhaps there's something we can do to mitigate this, like make it really easy to snapshot your /vendor directory (or wherever all this code is going to end up) to see if you're actually up to date or not, etc. Maybe Composer itself solves this, I don't know.

This is also a double-edged sword when it comes to making sure those 3rd party dependencies are kept up-to-date. In some ways, managing them outside the Drupal core Git repo makes it easier to update these things when necessary. However, that also implies there's going to be a long tail of people who will never bother. That's already a problem with Drupal itself, and I imagine it's also going to get worse if there are other cryptic steps (from the POV of end-users that just want a website and aren't PHP developers deeply in the know) they have to take. If we start going this route, we should open an issue for Update manager (if there isn't one already) about teaching it to notice if your vendor tree is missing updates and to warn the user about it (and ideally, run this for them).

And, I can almost guarantee there are going to be cases where 3rd parties rev their code in ways that Drupal isn't prepared to handle and the flip-side will be equally problematic -- people upgrading vendor before Drupal is ready and breaking their sites that way. Fun!

Of course, as we start relying more and more heavily on 3rd-party code for chunks of core functionality (YAY!) all of these problems are only going to get worse at the same time that the need for *not* putting all that code in our repos gets greater. So, I'm really torn. Mostly I think we should do this. But I don't think we should sugar-coat what a PITA/WTF this is going to entail. That said, this has been a problem in software development for decades (I won't even begin to start telling stories of the nightmares this stuff used to cause us when I worked on Condor), and the tools to manage it have only gotten better and better. So it's not like we're the first people to run into this and have to solve everything completely on our own. I haven't actually used Composer myself yet, but it sounds like a Good Thing(tm), and if it's reliable and simple, it's probably the right way forward.

That said, I'm going to flat-out refuse to touch the d.o packaging script until some of these other issues are hashed out in more appropriate places, and the summary here (or somewhere) links to all the various issues I've mentioned that need to be submitted (at least a couple of core issues, drush, PIFT/PIFR, etc).

Thanks,
-Derek

dww’s picture

p.s. Or we can turn this into the meta issue for this whole initiative, mark it active, and open a separate issue specifically for the trivial change to the packaging script (which would then be postponed). I didn't mean to obstruct progress, just that if this issue is about the packaging script itself, it's blocked on other things happening first. Thanks.

sun’s picture

the need for *not* putting all that code in our repos gets greater.

Frankly, I don't get this argument at all. This entire proposal came out of the blue and tries to fix a "problem" that I haven't heard of a single time from anyone else before.

Apparently, #2 states only "updating" as a problem, nothing else. Updating the libraries we're using within core is an issue, yes. But mostly a policy issue. And for the scope of updating, we already have a proposal in #1424924: Use Composer for updating Symfony components (without removing Symfony code from repo).

However. Once that Symfony code is in core, and once we're actively relying on it throughout core, your chances for being able to update it without adjusting tons of code throughout core at the same time are close to zero.

And that's where the version control system comes in and plays a key role in the game. If we update external libraries shipped with core, then we also need to update the consuming code in almost all cases. Updating only one of both will - as far as PHP library code is concerned - lead to fatal errors.

In the end, I don't get why we're trying to rely on an external tool to do the job that the version control system tool is supposed to do. Use the right tool for the job. And apparently, git was designed from the ground up to handle larger repository sizes in a smart way.

katbailey’s picture

Title: Process Composer components when building Drupal core » Remove symfony components from the core repo and let Composer manage the dependencies instead
Project: Drupal.org infrastructure » Drupal core
Version: » 8.x-dev
Component: Packaging » wscci
Category: task » feature
Status: Postponed » Active

Per dww's suggestion in #7 and seeing as this thread has so far been about whether this move is a good idea, changing its details to reflect that. Not sure whether to mark it as Policy, seeing as it's about making the decision and we can't provide a patch until #1424924: Use Composer for updating Symfony components (without removing Symfony code from repo) itself is in.

Will open sub-issues and mark them postponed for now.

Note: I'm actually pretty new to the core issue queue (apart from small stuff) so let me know if I am Doing It Wrong (tm) - I won't take it personally ;-)

katbailey’s picture

katbailey’s picture

Oh, and here's an interesting commit on the silex project: https://github.com/fabpot/Silex/commit/4ce45f1eecc3e7cfb69c70a7ba5abdd5d...

Sun, I don't get what you mean when you say

If we update external libraries shipped with core, then we also need to update the consuming code in almost all cases.

Surely for that to be true, the external library in question would have to have been extremely poorly architected, with no conception of abstraction or encapsulation or any of the lovely OO design principles that constitute the reasons we are embracing Symfony. Or maybe I am simply misunderstanding things here :-/

patcon’s picture

Re: @sun #8

Once that Symfony code is in core, and once we're actively relying on it throughout core, your chances for being able to update it without adjusting tons of code throughout core at the same time are close to zero.

Not sure how Symfony rolls, but if they're following some sort of semantic versioning policy (which package managers tend to get people into the swing of), then it's entirely possible that patch- and minor-level updates could be happening fairly painlessly. Only major-level should break backwards compatibility so long as we're using the public methods we're supposed to be, if I understand correctly -- and it's entirely possible I don't :)

In the end, I don't get why we're trying to rely on an external tool to do the job that the version control system tool is supposed to do. Use the right tool for the job. And apparently, git was designed from the ground up to handle larger repository sizes in a smart way.

My angle would be that version control is NOT intended to store large amounts of code, but meant to store all the information that is sensibly required to rebuild the current codebase at any future point in exactly the same way as it's built today. With custom code, that involves storing everything. With dependencies, that might be a simple as repo+project+version. Yeah, we could say that the external sources might go down, but so could pear.php.net or any of the other things we and other projects depend on for builds. We need to have a certain degree of trust in outside sources, and cover our asses with caching where it makes sense.

Anyhow, originally came here to post this link, which seems relevant:
http://stackoverflow.com/questions/2050450/git-hooks-management

RobLoach’s picture

Drush might be a good first start: #1316322: Add PSR-0 autoloader to drush

Anyhow, originally came here to post this link, which seems relevant:
http://stackoverflow.com/questions/2050450/git-hooks-management

Somewhat related, was working on some documentation for Composer which would allow you to automatically run a Composer update whenever a git checkout was performed:
https://github.com/composer/composer/pull/467/files

Not sure how Symfony rolls, but if they're following some sort of semantic versioning policy

I just asked #symfony, and they said they're using semantic versioning. Composer supports version wildcards to force a specific version branch. The following would retrieve the latest in the 2.1, but would not move to 2.2:
"symfony/finder": "2.1.*"

effulgentsia’s picture

In turn, there'd be no guarantee that everyone works on the same revision of files.

Per #1424924-79: Use Composer for updating Symfony components (without removing Symfony code from repo) and the comments following it, we can and I think should commit a composer.lock file to our repo to address this. Updating a vendor component in turn would require a normal core process of a d.o. issue with a patch file (containing an updated composer.lock) that is tested by bot (confirming no Drupal functional tests break as a result of the vendor update) and committed by a core maintainer.

So whenever I do a git pull you require me to do an additional "php composer --blah blub bleh" on the command line? I don't even know whether my git client would support that (TortoiseGit).

This is a valid concern that I raised in #1424924-85: Use Composer for updating Symfony components (without removing Symfony code from repo), but katbailey pointed me to this issue as the better place to discuss it. I think we need to balance this productivity concern with the productivity concern of people working on core issues that require a vendor component update plus the drupal code they're working on. We've seen on some issues already that posting a patch that includes both results in large patch files that intimidate reviewers. In some cases we've dealt with this by splitting the vendor update in a separate issue, but then there's delays in getting that committed, and meanwhile people are less motivated to work on the drupal issue that's blocked on it. By having a patch file include only a composer.lock (and possibly composer.json) update along with the Drupal code, would that increase number/quality/timeliness of reviews? I hope we can either find a solution that works for people who don't like command lines or a solution to the "big patch on d.o. issue is scary" problem. In the end, we want as many contributors feeling productive as possible.

Also, katbailey makes a good point in #1424924-64: Use Composer for updating Symfony components (without removing Symfony code from repo) about helping site developers and distribution builders manage their builds.

Crell’s picture

As a data point, right now I have a branch in the WSCCI repo to fix error handling tests (and change the way the database handles error reporting) that I am not merging yet because it's blocked by #1542710: Update to latest Symfony 2.1 code. I don't want to just merge it to the kernel branch now because that's going to result in a lot of fugly merge history when both huge Symfony updates end up in the same merge commit (something a number of developers have objected to with a git-based workflow, rightly or wrongly). Instead I want to wait until I can resynch the kernel branch, then rebase the new feature branch onto it.

If I could just include a tweaked composer file as part of that, I wouldn't even need to bother updating core when I needed a newer snapshot of Symfony. I could just merge and go and let Git sort out a few 2 line commits later on.

patcon’s picture

Issue tags: +Composer

tagging

Dries’s picture

I feel like this is a bit of a distraction. Let's stay focused on the minimal viable products (e.g. web services in core), even if that means there are some annoyances for the time being. It feels like this issue could easily distract some of our best people for a couple of weeks. I'd prefer not to do that.

patcon’s picture

For anyone interested, there's a summary of the composer-related issues here:
#1424924-91: Use Composer for updating Symfony components (without removing Symfony code from repo)

Also, here are all issues tagged "composer".

That other issue is generally for the step #0 that could be executed very painlessly: adding composer.json and composer.lock to repo
#1424924: Use Composer for updating Symfony components (without removing Symfony code from repo)

I respect @dries' rationale for postponing this larger issue, and anything I post is for later consideration. @sun and @katbailey made some relevant comments in the other issue that belong here:

@sun said

Introducing Composer for updating these libraries in core more easily is one thing. I can get behind that idea.

But removing these libraries entirely and completely relying on Composer sounds like a disaster.

Regardless of whether you're using a UI or the command line, this is not guaranteed to work:

git checkout 8.x-my-issue-dev-branch

Every checkout will have to be followed by a php composer whatever command whenever you are switching branches, because you cannot be sure anymore whether the files you have are complete, correct, and compatible.

While you CLI coders might be OK with that and will write a git alias or shell script, I'm seriously not. I'm using a git GUI for development, and I want to get the proper files when switching branches.

Pretty please don't make contributing to Drupal even harder than it is already.

@katbailey said

If no git GUI had existed for Windows back when we were switching to git, would that have been a good enough reason for us to stick with CVS? (I wasn't involved in the discussion so I have absolutely no idea whether such considerations factored into it, but I feel strongly that the answer to this question should be a resounding "NO!").

Changes like this don't affect the end user in the slightest and they affect developers only insofar as they force them to embrace the tools and practices that the wider PHP community has adopted in order to more effectively manage projects. That, to my mind, can only be a good thing.

I'd imagine that the vast majority of pulls from upstream when working on D8 will not include a change to composer.json (or composer.lock), and so no extra step will be required. Does your GUI client show you which files have changed when it merges changes from upstream? Is it possible to add git hooks in the GUI? If not, then I'd say it has some catching up to do, but that shouldn't hold up a change to Drupal that otherwise makes a lot of sense.

don't make contributing to Drupal even harder than it is already

I honestly see this change as having the opposite effect!

Crell’s picture

Dries: I'm unclear which "best people" would be tied up on this. Right now, it's a pain in the neck to "chase master" on Symfony. The more we want to push improvements upstream to Symfony (and we want to, and have a few times now), the more we'll want to stay up to date.

Unblocking this means we can get better velocity on Symfony-related stuff. Plus, the main tasks that need to be done to make it happen are testbot and infra tasks; those people are always busy, but mostly not on core. Plus, the tasks on the surface seem really easy: Just add an extra command that gets run before the packager and testbot do their final step. I don't see how that would "distract our best people for weeks". (Not to say that the infra people are not best people, mind; they do yeoman's work. I mean that it wouldn't be pulling people off of new-feature development for D8 to do this and make new-feature development for D8 easier.)

RobLoach’s picture

sun’s picture

Using Composer to update the libraries in a simple way makes sense.

However, removing the libraries entirely from Drupal core results in more negative consequences than it actually helps. The benefits of doing so have single, one-time implications only. The benefit only affects the initial addition. Contrary to that, the negative consequences are permanent and affect everyone who is trying to hack on, develop for, and contribute to Drupal core in any way.

I understand that this has caused some troubles for D8 core development, but that should be considered as a side-effect of a large transition process. Post-release, i.e. once all these new libraries have been added, it is quite predictable and reasonable to expect that our main issue will revolve around updating the libraries. (which still means update != add)

I also understand that "others" are using Composer in this way, but at the same time, those others have an entirely different understanding of the "product" they are developing and delivering, which is not comparable to the Drupal core product at all. The vast majority of them consider themselves as developer libraries or frameworks that can be and are actually supposed to be manually mixed, adjusted, and changed in order to produce an actual end-product that works. Contrary to that, Drupal core is a solidly defined set of code, subsystems, and libraries, which not only needs to work by itself, but also requires that solid definition of what's contained in order to allow contributed projects as well as its entire ecosystem to work against a particular major version. The moment you loosen or remove that fixed definition, you are opening the door for a huge range of conflicts and problems that haven't been there before.

What we're talking about is the management of external libraries. However, not just some additional libraries, but instead we're talking about replacing the libraries that are driving the very core of the system. Sorry for being blunt, but my impression is that most people have only thought about the direct benefit for themselves, without thinking about the system-wide, organizational, and technical implications. I'm not sure whether I am in a privileged position, but having spent a lot of time with Libraries API, jQuery, jQuery Update, and jQuery UI modules in contrib, I know that this change proposal has plenty of consequences, which haven't been considered at all in here yet.

(Before anyone is going to ask: No, I won't provide a more concrete list of consequences. The question itself would mean that one wants to change something without understanding the consequences. If I had solutions for most of the consequences, then stuff like Libraries API wouldn't be as crappy is it now. But that's contrib material. And that's where experimental stuff belongs, for which not all consequences are known or resolved yet. The consequences of this change proposal here have an even larger impact than that.)

Next to that there are practical issues. First of all, as the engineers who are developing, building, and delivering a final product, we should not only know the version of some external library we're pulling in. We are also the final line of defense that is responsible for ensuring that the code we're shipping for our core product is sane, contains what we want and expect, and is neither bloated nor too limited. We should know the code we're adding and shipping with core. Just adding a pointer to a package and version that consists of 500k of possibly bloated code and perhaps even nonsense, and basing an entire set of architectural changes on that, heavily increases the chance that almost no one actually studied the code that is going to be packaged before it is added. In fact, we just did something like that via #1465584: Review external library dependencies in core, and even though I created that issue myself, I never really reviewed the code being added. I strongly object to that approach. I agree with the "Not Invented Here"/"Proudly Found Elsewhere" efforts, but the moment we start to add and use functional code without even knowing and fully understanding what we're adding exactly, we're Doing It Wrong™, it couldn't get more wrong. That is, because it is us who are being held responsible for the final product we're delivering. If we do not fully understand the code that we're shipping, or which particular changes exist between a previous and a new version, then we will run into serious maintenance and security issues.

Second, the fact that we pulled in the latest version of external components and committed them allows people to easily hop between core and core sandboxes using alternative versions. A simple git checkout results in exactly the code you need. Presuming that you've fetched your remotes before leaving, everyone is able to work offline, without requiring a working Internet connection to (re-)download the packages you're possibly missing for another branch. As a direct consequence, we would destroy git's built-in/native capability of working offline. There's also no difference or conflict in what exactly "latest version" means, because the files are properly versioned.

Third, the troubles of novice core contributors even begin with getting a git client up and running already. In almost all cases, novice contributors are using a UI-based git client. It is absolutely clear that we need to on-ramp many more contributors in order to meet the demand of the market. While the additional usage of Composer might be clear and trivial to more advanced PHP coders, it is going to be a huge additional barrier for many novices to get involved, especially those who never really worked on the command line.

That said, the practical issues could be resolved in some ways. E.g., 3) by developing a git command/extension/fork that has built-in Composer support, 2) an offline cache for Composer (but which in fact would boil down to git mirrors of the pulled in packages, à la Drush), and 1) by requiring proofs of extensive reviews of code to be pulled in, before it is specified and committed as new Composer package.

However, the other issues I've mentioned earlier would still remain.

Crell’s picture

sun: While you raise some valid points, the one you spend the most time on is simply untrue.

You imply that by using Composer to pull in 3rd party code on-the-fly, we'll be pulling in untested code we've not looked at and vetted and decided was a good thing to do. That is completely false.

A composer file lets specify an exact version of a package you want to download. The composer.lock file, which gets generated and *should* be checked into the repository, specifies the version of a package you want to run down to the precise git commit sha1 hash to use. If we check into the repo a composer.lock file saying to use, for instance, Twig commit 123abc456, then every single person who checks out Drupal and runs composer update will get that precise, exact same version of the code. Yes, I am assuming that we are vetting the code we're talking about before checking in new versions of composer.lock, but that is no more or less safe an assumption than assuming that we're vetting periodic updates to the Symfony components now. I would presume we'd apply the same level of care to such updates that we do now.

The difference is that such patches are then 2 KB in size, not 2000 KB in size. It also means we do not need to first add an unused library to core just to have a vaguely readable patch for our own code; The "put Twig in core and use it" issue can have a single extra line in it to pull in Twig version whatever; Then that version (that precise, specific version on which we have done whatever auditing we feel like) will be downloaded by anyone applying the patch and running composer update. Similarly, in the routing work I'm doing now I won't need to manually add a commit that pulls in another 300 KB of Symfony updates because I need a newer snapshot than what's in core, because Niklas added some new functionality to Symfony's Routing component that I need.

Libraries API, jQuery, jQuery UI, etc. have nothing to do with it. The packager can run composer install/update as well, so people downloading tarballs will never know the difference. The entire "unvetted code" line of thought is FUD. Please stop with the FUD.

Your other points about branch switching while offline are valid; I'm not sure of the solution there, and agree we should discuss that. But let's stick to discussing real issues, not imaginary ones that have already been pointed out to be false multiple times.

patcon’s picture

I'm with Crell. Composer will make it simpler to bring in and roll with outside deps, but it won't unhinge code from common sense. That's still up to us to vet the single-line `composer.lock` patches that will represent large changes.

But you're right @sun about working offline. I'm lucky enough to live in a place where I don't need to worry about that, but it's likely a real issue for those living in areas with less reliable connectivity. Composer caching is in the works, and maybe that's a valid feature to wait on:
https://github.com/composer/composer/issues/54
https://github.com/composer/composer/pull/62

As for GUI users, I'm unsure whether there's a fix for this which would be ideal for the newest of core contributors, but there is this: Adding a default post-checkout hook on their system to run a script (from d.o docs presumeably) on each checkout.
post-checkout hook: http://stackoverflow.com/a/1011745/504018
Git hook templates: http://stackoverflow.com/a/1977655/504018

From now on, it's just:

$ git clone --branch=8.x http://git.drupal.org/project/drupal.git
$ mv drupal/.git/hooks/post-checkout.sample drupal/.git/hooks/post-checkout

We talk a lot of about new core contrib, but I really wish we had more data on who these people are. My thought was always that with the curve of core, it would be rather advanced users who began contributing so deeply in the code.

Seldaek’s picture

@Crell / #22 - just a small correction, you must run composer install, not update. update will ignore the lock file and update dependencies to the latest possible versions. Other than that you're correct.

As for the points mentioned by @sun - the offline thing is valid but hopefully will be mitigated in the future.

Regarding the contributors, I would just say.. why not give it a shot and see what feedback you get from contributors? It really is just a small-ish decision that can be reverted at low cost. Just commit the vendors again if it turns out it creates too much friction. I understand the uncertainty, but in terms of commitment, it's hardly as big as the other leaps that have been taken recently (Sf2 components and whatnot).

patcon’s picture

@Seldaek

update will ignore the lock file and update dependencies to the latest possible versions. Other than that you're correct.

Just to clarify, that's "the latest possible" given the version constraints in composer.json, right?

Crell’s picture

Seldaek: Oops, you're right. I got those two confused. What's the mitigation you're talking about for offline use?

Otherwise, what #24 said. This is being blown way out of proportion.

Seldaek’s picture

@patcon: yes.

@crell: local caching of packages mostly, but proxying via private satis repos might help too if you have some spare disk space, and if we build a CDN of mirrors it might help people in remote areas.

Sylvain Lecoy’s picture

I see no problems with that, in Java world we are using Apache Maven which is composed of a package repository manager.

When you fetch a new project, you get the last version of the pom.xml (here the composer.json) which indicates to maven which depencencies are needed for a given scope (e.g. at build time, run-time, or testing). This has no conflict with a configuration management such as git, they are complementary tools, just like git submodules for instance.

That would imply also that the qa.drupal.org testbot install the required dependecies before runing a test-suit. Composer would become a drupal dependency for the build and testing phases.

Crell’s picture

Sylvain: How does that address the "switched branches on an airplane" problem, where you need a different version of dependencies? That's the only conceptual blocker here.

Sylvain Lecoy’s picture

I am sorry I didn't get the idea, what do you mean by "switched branches on an airplane" ?

I am not english so there is some concept that I may not catch directly or needs some further explainations.

It is the fact of switching from branches to branches ? Well, for maven this is not a big deal, because maven keeps a local repository under a .m2 directory. This local repository is composed of all artifacts that you previously downloaded, meaning a switch of dependency is extra fast, plus is managed by maven itself. When you switch or change the pom.xml, maven automatically "Update Dependencies" for your project.

I am not sure for Composer however, if a local repository is not supported and you have to fetch a new version each time you switch a branch, that may cause heavy problems. Maybe the tool is not mature yet.

Crell’s picture

See #21 and #22.

gagarine’s picture

Composer is certainly going to have a local cache, their is already some functional code https://github.com/composer/composer/pull/915. This is where the offline issue should be fixed not in drupal git.

boombatower’s picture

Yea, definitely seems like correcting this upstream is the correct fix and would be beneficial to other projects.

sun’s picture

So...

I really tried to re-evaluate this proposal with an open mind in the past weeks. And I warmed up to it. To some extent.

However, my main problem still is the practical barriers that this change would cause.

I'm not sure how your reality looks like, but mine looks like this:

- Pull latest origin:8.x
- Hack.
- Switch to wscci:bundles-sun, hack.
- Switch to cmi:config-next, hack.
- Switch to cmi:config-thingie, hack.
- Switch to platform:user-picture, hack.

The important part is in "switch to". EDIT: To clarify, this means to switch to these entirely different branches multiple times per day.

All of these branches have a completely different merge-base and thus a completely different set of vendor files in core (and core was fixed to be compatible with recent vendor updates). Most of the branches have no need to merge in latest origin:8.x/HEAD, since they are the driving force for focused topics/components, so origin:8.x/HEAD only changes when one of the branches was committed/merged into core.

My platform's git UI (TortoiseGit on Windows, which has evolved into an excellent git client in the meantime [compared to other platforms as well as the CLI]) allows me to simply checkout and work on the branch I want to hack on.

Having to bootstrap the command line into the right location in order to switch/update the core dependencies accordingly — even when presuming there's a caching mechanism in Composer that's able to handle git/$VCS commit hashes, which inherently becomes a problem, since Drupal core does not specify exact commit hashes to include, but only min/max dev versions at this point, so Composer would have no idea what to actually switch to between those git branches... which inherently brings me back to my point in #8... — well, yeah, even if that major design flaw was resolved (e.g., by specifying explicit commit hashes in composer.json — which alas, would inherently break the original idea of making vendor updates "trivial" in core), this change would still hinder me in contributing.

That said, I can totally understand the point of this when working off a code base that has dependencies that are not supposed to change. I.e., essentially translating into "the released Drupal core." — though doing that sounds nonsensical if the dependencies are in the repo during development already.

In the end, the negative consequences of this change proposal heavily outweigh the positive ones (btw, what exactly are those, actually? "popular" does not count).

FWIW, in case Composer has a --do-not-touch-existing-vendors option, then I'd totally +1 a change proposal to make the testbots execute Composer with that flag in order to pull in entirely new dependencies being proposed for a particular Drupal core patch.

effulgentsia’s picture

Drupal core does not specify exact commit hashes to include, but only min/max dev versions at this point, so Composer would have no idea what to actually switch to between those git branches

That's not true. Composer.lock specifies the exact version, and if we allowed chasing non-release commits, would specify the exact commit. So, if/when #32 is resolved, then all that will be left is integration with git, such that a git switch (checkout to another branch/commit) of Drupal could automatically trigger Composer to update the vendor code to match what composer.lock specifies. Seems like once that's possible, all objections to this issue are resolved, but I don't know what it will take to get these things resolved.

RobLoach’s picture

Drupal could automatically trigger Composer to update the vendor code to match what composer.lock specifies. Seems like once that's possible, all objections to this issue are resolved, but I don't know what it will take to get these things resolved.

A while ago, I put together a pull request to add documentation about running a composer install after any git checkout: Use Git Hooks with Composer. That PR could probably use a touch up. Whether git hooks work with TortoiseGit is beyond me. TortoiseComposer, anyone?

Crell’s picture

Component: wscci » base system
Issue tags: +WSCCI

I still want to do this, but refiling...

patcon’s picture

Update to #32 gagarine:

Composer is certainly going to have a local cache, their is already some functional code https://github.com/composer/composer/pull/915. This is where the offline issue should be fixed not in drupal git.

First pass at cache support merged in:
https://github.com/composer/composer/pull/1282

gagarine’s picture

Yep local cache works now :).

Sylvain Lecoy’s picture

Good news, I was also wondering if you guys mentioned git submodules but Crell did it already in #5 ;).

patcon’s picture

Oh, and for anyone interested in contributing, @Seldaek (composer maintainer) seems interested in the git clone --mirror/git clone --reference approach that drush takes to caching git repos:
https://github.com/composer/composer/issues/1323

webchick’s picture

FileSize
104.38 KB
103.85 KB

Some folks were asking about this at DrupalCon Sydney, so I decided to pull some numbers to see what exactly the impact of "The Windows Problem" is, so we can throw some data into this discussion of heated opinions. :)

I went into drupal.org's Google Analytics account to segment the URL "project/drupal/git-instructions" URL (since xjm and I couldn't think of a better URL that would help pinpoint "yes this person is most probably a core developer/contributor") with operating system. It turns out, despite what DrupalCon photos might indicate, there's about 60%(!) of our contributor base using Windows:

git-instructions-os.png

In case you think to yourself something snarky like "Well, duh. Windows users are stupid, so of course they use this page more," :P I also checked /project/issues/user, which would catch contributors across projects, and that the numbers still shows a pretty large Windows majority:

 52% Windows, 35% Mac, 15% Linux

Granted, this doesn't let us know if someone's using Windows but nevertheless has Cygwin, VMWare, etc. installed. But I don't think we can exactly poo-poo this concern (nor the one about switching branches and forgetting to update composer).

kim.pepper was working at the sprint to try and determine some more data around the problem space, as well as if there are workarounds. His concern, and I agree with this, is that people from outside Drupal will see we're using Composer, and want to use it in the same way they can use Composer in any other project. However, currently, the way we use Composer for just an update is a "Drupalism," which is the very opposite thing we want to do when we're bringing in external libraries. :P

jhedstrom’s picture

Is the 'Windows problem' that users can't be expected to use the command line when needed? Composer runs on Windows according to the docs, and I don't think we should lump all Windows developers into a category of not being willing to use the command line if need be.

jhedstrom’s picture

I decided to pursue some evidence in terms of the sizing of the repository. For anybody using drush make, every time they build a project that has a dev release of core, the repository is cloned from drupal.org (using --working-copy—otheriwse, drush does have a ref-cache after the first clone). I did the following to see the size of the actual download:

jhedstrom@hyperion:/tmp (@sp)☠ git clone dm:drupal
Cloning into 'drupal'...
remote: Counting objects: 200544, done.
remote: Compressing objects: 100% (48043/48043), done.
remote: Total 200544 (delta 144690), reused 192009 (delta 138125)
Receiving objects: 100% (200544/200544), 55.33 MiB | 592 KiB/s, done.
Resolving deltas: 100% (144690/144690), done.
jhedstrom@hyperion:/tmp (@sp)☠ cd drupal/
jhedstrom@hyperion:/tmp/drupal (8.x) (@sp)☠ git bundle create drupal-with-vendor.bundle --all
Counting objects: 200544, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (41478/41478), done.
Writing objects: 100% (200544/200544), 55.30 MiB, done.
Total 200544 (delta 144690), reused 200544 (delta 144690)
jhedstrom@hyperion:/tmp/drupal (8.x) (@sp)☠ du -sh drupal-with-vendor.bundle 
56M     drupal-with-vendor.bundle

The current clone size is 56M. I then ran a series of commands to completely remove core/vendor from history (which I'd recommend we do if this goes in) on both the 8.x and 9.x branch.

jhedstrom@hyperion:/tmp/drupal (8.x) (@sp)☠ git bundle create drupal-without-vendor.bundle --all
Counting objects: 209319, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (38787/38787), done.
Writing objects: 100% (209319/209319), 35.37 MiB, done.
Total 209319 (delta 155618), reused 209319 (delta 155618)
jhedstrom@hyperion:/tmp/drupal (9.x) (@sp)☠ du -sh drupal-without-vendor.bundle 
36M     drupal-without-vendor.bundle

Without, the clone size is 36M.

For everybody who isn't currently working on 8.x, this is 20M extra download everytime they clone 6.x or 7.x.

Once 8.x is the current stable version, the size of the repo is not as big an issue (in fact, with a new git clone and a composer install, far more code is downloaded, since the history of each vendor project comes down as a git repository).

It has been stated above that using Composer may make contributing to Drupal more difficult. I disagree, but regardless, what the current system does without a doubt is to discourage contribution back to the non-Drupal projects such as Symfony and Twig, since with those checked into the repo, contributors have no easy way to contirbute a pull request upstream to those projects.

gagarine’s picture

It has been stated above that using Composer may make contributing to Drupal more difficult. I disagree, but regardless, what the current system does without a doubt is to discourage contribution back to the non-Drupal projects such as Symfony and Twig, since with those checked into the repo, contributors have no easy way to contirbute a pull request upstream to those projects.

This is a major point.

RobLoach’s picture

I feel like this discussion is getting a bit out of scope. Let's dial it back a bit...

  1. Composer works on Windows
  2. If you have Drush working on Windows, the Drush Composer wrapper works as well
  3. With the addition of Composer, Drupal core has the ability to bring in external libraries without needing to copy/paste. If someone wanted to contribute to Drupal, they could put together PHP package (like msonnabaum's KeyValueStore), and have Drupal's composer.json bring it in. It's not about having people contribute to Drupal, it's about having Drupal work well with others.
  4. The good thing about forcing updates upstream is that it forces updates upstream. We should be contributing fixes to the projects we use, and not forking them locally. Let's teach best practices, not bad habits.
  5. This issue is about removing core/vendor from the git repository and having Composer handle that for us. Benefit is that updates are managed for us (we don't need to submit update patches against core/vendor). Con is that it would need some d.o infrastructure changes. Very doubtful this would happen in Drupal 8, maybe for Drupal 9.
kim.pepper’s picture

I have written a simple patch in #1920666: Allow additional external libraries with composer that provides:

  • An example.composer.json for each Drupal install, which references Drupal cores composer.json for dependencies. This can be then used to add new libraries used in the project
  • Removes core/vendor
  • Allows composer to download vendor libs into the project root /vendor dir
  • Changes Drupal's classloading code to look in /vendor

I agree that the biggest hurdle is infrastructure changes: running composer install when generating the drupal download, and testbot running composer install when running tests. The code changes are very trivial.

Kim

effulgentsia’s picture

what the current system does without a doubt is to discourage contribution back to the non-Drupal projects such as Symfony and Twig, since with those checked into the repo, contributors have no easy way to contirbute a pull request upstream to those projects

I either disagree with or don't understand this claim. We are cloning projects in /core/vendor/, not forking them. I'm not aware of any point in the D8 cycle in which we accepted a change to an upstream project as a Drupal-specific fork. If you want some code in Symfony or Twig to be different, the only way to do that is to submit a pull request to the upstream project. The annoying step is then after that pull request is accepted by the upstream project, we also need a Drupal patch that brings it into Drupal's code base. However, at this point, we're setting composer.json to target tags / commit ids anyway, which means even if we didn't commit the vendor code into our codebase, we would still need a Drupal patch that updates composer.json to the new tag / commit id. For these same reasons, I similarly don't get what #46.3 and .4 are saying.

Is the 'Windows problem' that users can't be expected to use the command line when needed?

I think #34 sums it up nicely. When you switch to a different branch/tag/commit of Drupal's code base, it should automatically update vendor code to match what's in the composer.lock file of the Drupal code you're switching to. Having to require more than 50% of Drupal contributors to manually run an additional command every time they run git checkout seems worse for total core development productivity than having /core/vendor/* in our git repo.

However, #1920666: Allow additional external libraries with composer lists good reasons why pulling out /core/vendor would help contrib and custom site developer productivity, which is certainly worth considering and might tip the scales.

jhedstrom’s picture

I either disagree with or don't understand this claim. We are cloning projects in /core/vendor/, not forking them.

I didn't mean to imply we were forking the vendor projects, nor did I mean that contributing to those would be impossible. I wanted to point out that using composer as it is used elsewhere makes contributing upstream to vendor project much easier since one has all the necessary git repos in their working project.

kim.pepper’s picture

I wanted to point out that using composer as it is used elsewhere makes contributing upstream to vendor project much easier since one has all the necessary git repos in their working project.

We're not currently using the git repository method for pulling in dependencies.

Kim

patcon’s picture

We're not currently using the git repository method for pulling in dependencies.

@ykim.pepper
whenever someone wants to fix an upstream bug:

rm -rf vendor/twig
composer install --prefer-source
# Just twig cloned
kim.pepper’s picture

@patcon Oh nice!

gagarine’s picture

One more argument to remove the component from the repo https://github.com/theodo/TheodoDrupal8Bundle/blob/master/Resources/doc/...

If you want to integrate drupal8 back in symfony (and yes you want to do that if you want to add a CMS to an existing symfony project) you will have to hack the autoloader...

EDIT: At least to resolve the issue on TheodoDrupal8Bundle we need a switch (in settings) to disable the autoloader. Their is an issue for that already?

gagarine’s picture

Issue summary: View changes

Updated issue summary.

larowlan’s picture

All the blockers are in, can this be un-postponed now?

dmouse’s picture

+1 this is necessary.
Maybe create a library "drupal-core" to using this library in any project via composer.
For users without access any terminal or no tecnical users create a package Drupal Standar with all vendors included. like Symfony
{
"require": {
"drupal/drupal-core":"8.x"
}
}

cordoval’s picture

I have started this here https://github.com/cordoval/drupal-diff is experimental but i would like to get early feedback

thanks

dawehner’s picture

Title: Remove symfony components from the core repo and let Composer manage the dependencies instead » Remove external dependencies from the core repo and let Composer manage the dependencies instead

Let's be honest, we do have more dependencies now.

Crell’s picture

We do. And this issue is still important.

The blockers, last time I checked, were "we need to make infra changes" to testbot and the packager for core. Are there issues for those? How can we make those happen?

It really is a PITA to have the components in core when developing; any issue that touches composer is 3x harder than it should be as a result.

Also, we haven't been changing composer libs on a weekly basis in well over a year so the "zomg everyone will have to composer update every time they switch branches" problem is basically non-existent 95% of the time for 99% of developers.

kim.pepper’s picture

drumm’s picture

Does this need to be escalated to the Technical Working Group and/or core maintainers? I'd like to know this will move forward before going ahead with infrastructure changes.

webchick’s picture

I think probably the core maintainers. I've sent around an email to ping them/us with this issue.

drumm’s picture

I should say the infra changes aren't too bad, the packaging issue looks like it could be deployed right away. If semver were a 10, I'd call this a 3 or 4.

However, it requires an update to old testbots. That will be the same sort of straightforward change, which distracts from new testbots. I would consider, but not require, blocking this on new testbots being the primary way of testing.

webchick’s picture

I don't feel like postponing on new testbots is unreasonable at all, given this has already sat here for 2+ years. But I'd love to get a gut-check from the other core maintainers if this is even still on the table for 8.x before spending much more time on it.

catch’s picture

Removing the dependencies from the repo is very simple, it's really the infrastructure and workflow change that takes some effort.

We'd need to be more careful how we pin dependencies in composer.json, which deserves its own issue - right now some libraries will update if you run composer update, which we don't want happening without a conscious decision especially for patch-level releases.

In principle I think we could do this for a minor release if we had to - still gives us a beta/rc run up to iron out kinks, but doing it before 8.0.0 would be preferable. Blocking on new testbots definitely, the core change is policy/composer.json which doesn't depend on the infra, and git rm which is quick.

There's an argument about no-cli core developers getting stuck because composer is CLI only. I don't think that really holds, because if the dependencies are packaged in the tarball, you can download and unpack that, then copy the vendor directory over. It's an extra step, but it's not that much more of an extra step than running composer install.

larowlan’s picture

Also modern ides include composer integration, for those who use a git GUI (which may also be their ide)

Crell’s picture

So the steps to get this to happen would be:

1) Switch composer.json to very very explicit pins, down to the .z release or git hash as appropriate.
2) Update infra: kimb0 already added issues for that.
3) Make the switch in the repo
4) Profit!!1!

Am I missing a step in there? Point 1 sounds like it would be pretty easy to do.

drumm’s picture

2) Update infra: kimb0 already added issues for that.
2a) Update packaging
2b) Update drupalci
2c) Update legacy testbots

For (2c) - Any changes to legacy testbots is what I'd like to avoid, so work can be concentrated on drupalci, to resolve D8 blockers like #1867192: Testbots need to run on 5.4, 5.5, 5.6 and 7.

I'd say "straightforward" is a better word than "easy."

webchick’s picture

Right. So step 2.5 I think is "wait for testbot TNG." That needs to happen before D8 ships anyway.

xjm’s picture

Issue tags: +revisit before release candidate
davidwbarratt’s picture

Crell,

Shouldn't we just rely on the composer.lock file for the exact commit hash rather than pinning it in composer.json? If there is a composer.lock file you can simply run composer install to use the exact commit hash.

Although I suppose what should be in composer.json should be preciously whatever Drupal core requires, nothing more and nothing less.

Am I missing something here?

Crell’s picture

David: In a fully composer-based project, the composer.json file is what you start with; that means different new projects with the same composer.json could get different code versions. Once you check in the composer.lock file any subsequent installs would get the same precise git hashes, but fresh installs could differ in their dependencies in "safe" ways (assuming the vendors are using semver properly).

That's less of an issue if we check in composer.lock, true, but it's valid for us to be more restrictive and precise in our composer.json file than most projects.

RobLoach’s picture

davidwbarratt’s picture

Crell,

Is this not accurate or am I just confused?

If there is a composer.lock file in the current directory, [composer install] will use the exact versions from there instead of resolving them. This ensures that everyone using the library will get the same versions of the dependencies.

If there is no composer.lock file, composer will create one after dependency resolution.

https://getcomposer.org/doc/03-cli.md#install

If I am understanding this correctly, if composer.lock is present (i.e. in the repository) running composer install should result in the exact same dependencies (i.e. whatever is in composer.lock, not what is in composer.json).

I believe running composer update would re-resolve the dependencies and update the composer.lock file.
https://getcomposer.org/doc/03-cli.md#update

I'm sorry if you already knew all of this, perhaps I don't understand your response.

effulgentsia’s picture

Category: Feature request » Task

Given that #1975220: Allow a Composer user to manage Drupal, modules, and PHP dependencies with a custom root composer.json is working on the "and let Composer manage" part of this issue's title, once that's done, this issue will likely become just the "Remove external dependencies" part, which may involve some feature requests against drupal.org, but in terms of this issue, which is filed against core, I think removing code from core is more of a task than a feature, so categorizing accordingly.

I discussed with alexpott as to whether to raise this issue to Major, but he said not yet, but we might depending on how #1975220: Allow a Composer user to manage Drupal, modules, and PHP dependencies with a custom root composer.json gets resolved and the corresponding implications. For similar reasons, he didn't want to prematurely postpone this to 8.1.x either, so leaving the version alone as well.

davidwbarratt’s picture

To add to what I said in #73,

Assuming #1975220: Allow a Composer user to manage Drupal, modules, and PHP dependencies with a custom root composer.json lands and Drupal core becomes a dependency of a Drupal project, whatever is in composer.json will be used, not what is in composer.lock I think this is actually a good thing, but I want to re-iterate why.

Basically, if we fix composer.json to specific versions / commits, it would be the equavalent of writting a contrib module and fixing it to specific versions of dependencies. If you were to do this, it could prevent you from being able to upgrade a module, or Drupal core itself. Or being able to use a different module since the versions conflict. However, most contrib modules do not constrain their modules to specific versions unless there is a reason for doing so. This would be equivalent to * in Composer, or more accurately ~8.0 since the drupal 8 version of each dependency is required.

Let me explain. If I want to use a package that depends on sdboyer/gliph version 0.1.6 then Composer will install 0.1.6 even though we are currently using 0.1.8 because 0.1.6 meets the version requirement in composer.json. However, let's pretend that instead of 0.1.6 we need, we are using a package that requires >=0.6. Even though this is a minor release, Composer will fail to install it because drupal/core requires 0.1.*.

If you'd like another example, suppose you tried to install a package that required symfony/dependency-injection >= 2.6.0-beta2. Right now, Composer would refuse to install it because Drupal cannot run on beta2.

I think drupal should define version ranges or a comma-defined list of compatible versions. Unless there is some reason Drupal will not work with symfony/dependency-injection 2.6.0-beta2 or 2.5.7, etc.

Doing this will help prevent Composer conflicts in the future and allow people to select packages that work well with Drupal and do not cause dependency problems.

As far as testing and packaging is concerned, I think they will always use composer install which will use whatever is in composer.lock and will completely ignore composer.json. It should be a policy that Drupal is only "guaranteed" to run under what is in composer.lock but that there's isn't a reason why it wouldn't run under other "compatible" versions of the dependencies.

If composer update installs the wrong version we want, we can always go into composer.lock and modify the file by hand to be the precise version. For example if we specify >=2.6,<2.8 for the Symfony components, but what we want tested/packaged is beta1, we can simply run composer update (which will install beta2), go into the composer.lock file and change it to beta1, and commit the modified file. Then, running composer install will install beta1 not beta2.

catch’s picture

As far as testing and packaging is concerned, I think they will always use composer install which will use whatever is in composer.lock and will completely ignore composer.json. It should be a policy that Drupal is only "guaranteed" to run under what is in composer.lock but that there's isn't a reason why it wouldn't run under other "compatible" versions of the dependencies.

Yes of the various options I think this is probably the best one. We also need to be a bit more explicit in our composer.json. Main thing is that testing/packaging are 100% guaranteed to be the same all the time, and that any install of core that doesn't intentionally change versions is also guaranteed to be the same. Also that we only change dependency versions via an explicit issue + commit to core.

Mile23’s picture

I'm not sure what this issue is proposing. It needs a summary update.

Are we saying remove core/vendor/ from the repo and then have the tarball maker say composer install as part of packaging? Because yay plus a million on that. We could also move some packages like PHPUnit to "require-dev", and have the packager say composer install --no-dev.

Are we discussing the relative looseness of version constraints in composer.json? Because @davidwbarratt++ on that. Loose requirements plus lots of continuous integration equals finding version incompatibilities before users do. Especially if there's something like a nightly build that says composer update.

catch’s picture

@Mile23 I think it's both of these, with the caveat if we go for version looseness in composer.json we'll have to check composer.lock into the repo.

Automated branch level testing with composer update would be a massive benefit, updating libraries has massive overhead at the moment.

Mile23’s picture

Checking composer.lock into the repo is standard practice, so you can just install without the update phase.

davidwbarratt’s picture

Status: Postponed » Active
Related issues: +#2380389: Use a single vendor directory in the root
davidwbarratt’s picture

Status: Active » Needs review
FileSize
832.38 KB

Attached is a patch that removes the core/vendor directory as well as core/.gitignore and is certain to fail testing. :)

Status: Needs review » Needs work

The last submitted patch, 81: no-vendor-1475510-81.patch, failed testing.

davidwbarratt’s picture

Status: Needs work » Needs review
FileSize
14.31 MB

Perhaps one that actually applies. (learned something new about git diff today)

Status: Needs review » Needs work

The last submitted patch, 83: no-vendor-1475510-83.patch, failed testing.

bforchhammer’s picture

Patch file size 14.31 MB. I knew we had a lot of dependenies, but wow. ;-)

RoSk0’s picture

Test results:

  1. git pull
  2. composer install
  3. drush si --db-url=mysql://root:toor@localhost/drupal8.dev -y --account-pass=admin
  4. ./core/vendor/phpunit/phpunit/phpunit -c ./core/phpunit.xml.dist with exit results:
    Time: 1.61 minutes, Memory: 206.00Mb
    
    OK, but incomplete, skipped, or risky tests!  
    Tests: 6677, Assertions: 36511, Incomplete: 2.
    
  5. php ./core/scripts/run-tests.sh --concurrency 12 --url http://drupal8.dev --all - had a lot of fails and PHP fatals here.
neclimdul’s picture

patch doesn't apply but doing the following, all simpletest passed that pass on my local machine. I've got a handful that fail on HEAD that also failed with this test.

git rm -rf core/.gitignore /core/vendor
cd core
composer install
cd ../
<run all tests>

So really the only thing blocking this imho is:

  1. Figuring out the testbot process.
  2. Figuring out the packaging process(do builds contain include /core/vendor?).
  3. Communicating the change to devs.
Mile23’s picture

Mile23’s picture

Testbot process discussion here: #2315537: Install composer dependencies before running tests

Packaging process:

  • composer install (optionally with --no-dev if we don't want to package e.g. phpunit)
  • Whatever else the tarball maker does

Educating developers:

Mile23’s picture

davidwbarratt’s picture

FYI:

I mentioned this issue in my session at DrupalCon Los Angeles: Using Subtree Splits to spread Drupal into everything

timmillwood’s picture

This all makes so much sense. I believe it would be best not to have the vendor directory committed in Drupal.

The concern would be the smaller hobbiest sites who may not know what composer is, but could we not offer a "Drupal + dependencies" package as a zip / tarball?

To get this patch in we first need to get it to pass the tests, which means getting test bot to run `composer install`. There are two ways to do this:
1) Add something like `shell_exec('composer install');` to run-tests.sh
OR
2) Update the script which runs run-tests.sh to first run `composer install`

dawehner’s picture

@timmillwood
Yeah indeed, see #2315537: Install composer dependencies before running tests for more information ... I think though once the new testing infrastructure is in place, it will be trivial to make that change, or maybe it will be even executed automatically, you never know.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
17.92 MB

He's a patch which "should" add composer to the test bot before running the test, just so we can see if removing the vendor folders work.

neclimdul’s picture

can we get a patch without the /vendor changes?

Status: Needs review » Needs work

The last submitted patch, 94: 1475510-94.patch, failed testing.

timmillwood’s picture

@neclimdul: isn't the whole point of this issue the /vendor changes?

Looks like my patch failed, so maybe we'd be best waiting until the new testbot is out.

dawehner’s picture

@neclimdul: isn't the whole point of this issue the /vendor changes?

Well, you change more than the /vendor change, so it would be neat to be able to review that part without opening up 18MB in dreditor, which is not possible, well, maybe
you have a super computer :)

timmillwood’s picture

There were two elements to the patch that weren't removing the vendor directory.

diff --git a/core/.gitignore b/core/.gitignore
index 91bd4cd..bd50526 100644
--- a/core/.gitignore
+++ b/core/.gitignore
@@ -1,16 +1,2 @@
-# SimpleTest breaks with the following files, so avoid adding them.
-vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services1-1.php
-vendor/symfony/class-loader/Symfony/Component/ClassLoader/Tests/Fixtures/php5.4/traits.php
-vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services11.php
-
-# The resources for the Validator component are not required.
-vendor/symfony/validator/Symfony/Component/Validator/Resources
-
-# Symfony Validator depends on Symfony Translation but only requires the
-# TranslatorInterface. Thus, we add only the required interface from Symfony
-# Translation by ignoring everything except the interface.
-vendor/symfony/translation/Symfony/Component/Translation/*
-!vendor/symfony/translation/Symfony/Component/Translation/TranslatorInterface.php
-
-# PHPUnit provides some binary dependencies that are already available.
-vendor/phpunit/phpunit/build/dependencies
+# Vendor should not be included in git
+vendor

This update to /core/.gitignore prevents vendor from being committed again in the future.

diff --git a/core/scripts/run-tests.sh b/core/scripts/run-tests.sh
index 174f14d..0d19525 100755
--- a/core/scripts/run-tests.sh
+++ b/core/scripts/run-tests.sh
@@ -5,6 +5,9 @@
  * This script runs Drupal tests from command line.
  */

+// Install Composer
+shell_exec('curl -sS https://getcomposer.org/installer | php && php composer.phar install');
+
 use Drupal\Component\Utility\Timer;
 use Drupal\Component\Uuid\Php;
 use Drupal\Core\Database\Database;

This was a hacky attempt to get testbot to run `composer install`, which I would hate to see committed to core.

cilefen’s picture

Issue tags: +Needs beta evaluation

This issue needs a beta evaluation. As a normal priority issue that imposes yet another feature request on the testbots, a thoughtful case would have to be made about why we need this now.

timmillwood’s picture

dawehner’s picture

This was a hacky attempt to get testbot to run `composer install`, which I would hate to see committed to core.

Yeah nor would I like to run external code directly as part of run-tests.sh ... I think the only proper way to fix it is to help on the d.o. issues,
get the packaging as well as the testbot parts solved.

timmillwood’s picture

pwolanin’s picture

We should consider putting vendor code outside the docroot by default if possible.

timmillwood’s picture

@pwolanin - this issue here is the users downloading the zip or tarball then uploading to ftp.

The zip / tarball currently *is* the docroot so having vendor outside there isn't possible, also some hosts that just give ftp access only give access to the docroot so again having vendor outside isn't possible.

I believe making the vendor location switchable would be ideal.

davidwbarratt’s picture

#104,

If the dependencies are no longer in the repo, you can configure Composer to put them where ever you would like (you can actually do this now, you just end up with two copies of the vendor directory).

Where should they be by default? I don't know, nor do I care. :)

Mile23’s picture

Status: Needs work » Postponed
Issue tags: +blocked by drupalci

This issue should be postponed until the new DrupalCI testbot is live.

When that's happened, we can work on the deploy-for-test script for D8, which will be able to say composer install by itself. This probably needs its own issue... It might exist, I just can't find it for DrupalCI. Here it is for PIFR: #1923582: Add ability for testbot to run 'composer install' during installation

When that's happened, we can do this: #2315545: Install composer dependencies to D8 when packaging

And then when *that's* happened, we can then remove the vendor directory from the repo. :-)

timmillwood’s picture

Been looking again at this and removed core/vendor, removed core/composer.lock, updated core/.gitignore.
The patch is very large so I won't post here yet, but I've put it up on github so I can test with Travis CI. Pleased to report everything is passing on php 5.4, 5.5 and 5.6 because I am running composer --working-dir=core --prefer-dist install as part of the install.

https://github.com/timmillwood/drupal/commit/2d17569c9f5988bffa70c193dfd...
https://travis-ci.org/timmillwood/drupal/builds/70033645

daffie’s picture

@timmillwood: Can you post a patch with the vendor directory. That should be a lot smaller and we can then review it more easily.

timmillwood’s picture

@daffie - Problem is the whole point of this issue is to remove the vendor directory, so there's little point posting a patch with the vendor directory.

davidwbarratt’s picture

#108,

Sweet! Just an FYI, I would not remove composer.lock from the repo. While I think it's a good idea to allow any dependencies to be installed, for the purpose of packaging, I think it could be a racket for some people not knowing exactly what is about to be released. It's certainly possible that while a release is being prepared, a dependency is updated within our range and what we deploy is not what was intedended.

For testing, I think you should just run composer update and use the --prefer-stable --prefer-lowest option(s), and test it again without those option(s) to get the highest version possible, which is what symfony does.

Also, you can ignore the composer update warnings.

timmillwood’s picture

@davidwbarratt - I'm still on the fence about composer.lock, I see good reasons to keep and good reasons to remove, but kinda wanted to see what would happen if it was removed, and seems like all our tests pass regardless.

I've done some tests with composer update both with and without --prefer-stable --prefer-lowest (with failed).
https://travis-ci.org/timmillwood/drupal/builds/70043117

davidwbarratt’s picture

#112,

Well, my vote is to keep composer.lock. It's ignored when using drupal/core as a dependency and it will make a lot of people in this thread more comfortable to know that the dependencies are locked to a specific commit.

That's really interesting that the tests with the lowest dependencies are failing. I wonder if there are some dependencies that we need to bump up in version.

Also, do you think we should update #1923582: Add ability for testbot to run 'composer install' during installation to run both tests?

timmillwood’s picture

@davidwbarratt - All the failures are to do with mikey179/vfsStream, in composer.json we have 1.*, but in composer.lock using 1.5, so I am looking at setting a minimum in composer.json of 1.5

davidwbarratt’s picture

#114,

Yeah, or just step the version up until the tests pass. :)

davidwbarratt’s picture

Mile23’s picture

composer.lock gives us a known test-pass state for distribution, and also speeds up the process of packing for distribution (since composer can then just get the locked packages, rather than determining the best solution over and over).

That, my friends, was a run-on sentence.

Anyway, there's not really a good reason to remove composer.lock. If calling composer install causes problems or failures of any sort, then the fault is in our version requirements, not the lock file.

davidwbarratt’s picture

whaaatttt Mile23 and I agree on something? What is this world coming to? ;)

Mile23’s picture

I'm sure we'll figure out how to disrupt this awkward alliance somehow. :-)

Crell’s picture

I agree, let's leave composer.lock in place for now. We can punt deciding to remove it to a future task. The big win here is getting vendor out of the repo.

webchick’s picture

Version: 8.0.x-dev » 8.1.x-dev
Priority: Normal » Major
Issue summary: View changes
Issue tags: -revisit before release candidate

Summary from core committer discussion: everyone is on board with this change happening. However, it should not block D8's release.

Whenever this gets fixed, the tarball will be exactly the same, so this doesn't present an API change. Git users will need to take care, but they're able to use Composer anyway.

timmillwood’s picture

Status: Postponed » Needs work

awesome news @webchick

Time to get testbot ready!!!!

Squeee!

cweagans’s picture

Re #121 - Just to clarify, you're saying that we can do this before 8.0.0, but if it doesn't happen, it doesn't happen? Or are you saying that we're going to wait till 8.1.x? Little confused by the version change.

Crell’s picture

Isn't this issue a blocker for making composer not a second-class-citizen for module developers? Are we really going to wait for 8.1 (and the disruption therein) to make composer-using modules actually work properly? Given the number of tier-1 modules that plan to leverage composer libraries that seems not-good...

Mile23’s picture

Version: 8.1.x-dev » 8.0.x-dev
Issue summary: View changes
Issue tags: -Needs beta evaluation, -blocked by drupalci

Right, so time to review #2315545: Install composer dependencies to D8 when packaging and #2315537: Install composer dependencies before running tests, then we can patch here to pull vendor out of the repo.

This isn't the issue that blocks composer first-class citizenship. It's a step in the right direction towards allowing it, however.

I respect that @webchick changed the version to 8.1.x in #121, but there's no useful way to patch against 8.1.x for the current dev cycle.

So therefore, I'm setting it to 8.0.x-dev. Obviously someone can switch it back if I'm out of line.

If this gets in for 8.0.0 then yay and no problem. If it doesn't, then we leave this issue on 8.0.x-dev if it's considered a minor update (8.0.1), or bump it to 8.1.x.

Also, it has a beta evaluation and is no longer blocked by drupalci, so I'm removing those tags. Updated the issue to clarify what I understand to be the position.

Mile23’s picture

Here's a patch. It just removes core/vendor/ and modifies example.gitignore to point the user away from committing core/vendor/.

Note that this leaves us without core/vendor/.htaccess, but due to Drupal\Core\Composer\Composer::ensureHtaccess(), it is autogenerated for us.

Obviously the patch will fail, but we can retest as the dependencies happen.

Mile23’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 126: 1475510_125_fail.patch, failed testing.

timmillwood’s picture

Crell’s picture

Issue tags: -WSCCI

This isn't really WSCCI anymore.

timmillwood’s picture

Once we remove external dependencies from the core repo we will need the packager to add the dependencies at packaging.
The latest patch in #2315545: Install composer dependencies to D8 when packaging is ready and waiting to be committed. It has been tested on a test d.o instance.

The last submitted patch, 126: 1475510_125_fail.patch, failed testing.

drumm’s picture

Re-testing on the old testbots won't help. Please be sure to click "Add test" to add a re-test (and/or add another environment).

If #2315537: Install composer dependencies before running tests is in production, I expect it to run in the regular production jobs.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
17.28 MB

Status: Needs review » Needs work

The last submitted patch, 135: remove_external-1475510-135.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
1.64 KB
17.39 MB

I am hoping this patch would apply. I created the patch with --binary flag.

I have also attached a smaller patch for review. The changes in the bigger (17 MiB) patch is basically apply the smaller patch and rm -rf core/vendor.

Of course, I am not sure if the patch would pass the tests. That depends on #2315537: Install composer dependencies before running tests.

Status: Needs review » Needs work

The last submitted patch, 137: remove_external-1475510-137.patch, failed testing.

timmillwood’s picture

@hussainweb - Thanks for the re-roll

It looks like #2315537: Install composer dependencies before running tests is not ready as @drumm thought it might.

Mixologic’s picture

It is deployed to the drupal ci bots, and will probably not be deployed to the qa.drupal.org bots as they are being deprecated soon. Very, very soon. no new features are being added to them.

hussainweb’s picture

@Mixologic: Does that mean that if I click on 'Add test', it would run composer install?

timmillwood’s picture

Status: Needs work » Reviewed & tested by the community

ok, I didn't understand the old and new testbot were displaying in the same output.

RTBC'ing the patch in #137, although it might be best to wait until #2534132: Disable Legacy Testbots and use drupalCI as our testing infrastructure to to save the same confusion I had.

catch’s picture

Status: Reviewed & tested by the community » Needs work

FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: failed during invocation of run-tests.sh --clean.

catch’s picture

Or postponed, but this can't go in until that change is made.

timmillwood’s picture

Status: Needs work » Postponed

@catch - As @Mixologic pointed out to me, that's qa.drupal.org, you'll see it's passing Drupal CI.

PHP 5.5 & MySQL 5.5 13,697 pass

Let's go with postponed until #2534132: Disable Legacy Testbots and use drupalCI as our testing infrastructure.

catch’s picture

@timmillwood - I'm well aware of that, qa.drupal.org is still the canonical testing environment for another few days at least.

Also this needs manual testing to ensure that the versions we get from say 8.0.0-beta15/dev tarball are exactly the same as the versions we get from composer install.

And for #2315545: Install composer dependencies to D8 when packaging to be deployed. For example had this been committed four hours ago, beta15 wouldn't have had a working tarball.

Mixologic’s picture

With the advent of #2380389: Use a single vendor directory in the root we can re address this issue.

However, we still need to keep it postponed until both packaging and testbots are ready - github oauth tokens and some patches need to be in place inorder for those to proceed.

Mile23’s picture

Which patches?

Mixologic’s picture

catch’s picture

timmillwood’s picture

Looks like we're pretty much there with #2573687: Setup GitHub OAuth for Composer on Testbots and #2315537: Install composer dependencies before running tests, so once they are deployed we can revisit #2315545: Install composer dependencies to D8 when packaging.

Is this something we're going to need to wait for 8.1.x for? or can we do it in a patch release? (I'm thinking 8.1.x, right?)

catch’s picture

It's not impossible that it could happen with patch releases, but I think it's more likely we'd try to start doing it with 8.1.0 betas/rcs since it affects workflows where people are deploying sites based on git checkouts of core.

timmillwood’s picture

Version: 8.0.x-dev » 8.1.x-dev

My thoughts exactly!

Mile23’s picture

bojanz’s picture

Status: Postponed » Active
hussainweb’s picture

Looking back at the edits only patch in #137, I don't think we need those changes anymore. Assuming the packaging will put in vendor, it is not a good idea to ignore vendor by default in example.gitignore. There is already a line to ignore vendor which is commented. If someone is using the composer workflow, he will know to uncomment it.

OTOH, core/.gitignore is not necessary anymore. I am just removing it. The patch is much smaller than before (but still rather large). These are the two commands for the changes in the patch.

$ git rm core/.gitignore
$ git rm -rf vendor/
hussainweb’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 156: remove_external-1475510-156.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
12.15 MB

Here is the correct patch with --binary. Also, I filed #2635584: Move core/.gitignore policies to composer events/packaging scripts to discuss removing core/.gitignore. We could at least get rid of that before we fix this one.

pfrenssen’s picture

#2315545: Install composer dependencies to D8 when packaging is in now. Is there anything else left that is blocking this?

pfrenssen’s picture

Here is an updated patch against 8.x-1.x.


$ git rm core/.gitignore && git rm -rf vendor/ && git diff --cached --binary

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

As long as the test passes (not sure why it wouldn't) I don't see any reason why this shouldn't be committed to 8.1.x

pfrenssen’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Postponed
Related issues: +#2635584: Move core/.gitignore policies to composer events/packaging scripts

In here we are obliterating the .gitignore file, while in #2635584: Move core/.gitignore policies to composer events/packaging scripts it is being dissected meticulously. We can best wait for that to land first. This also needs documentation updates.

Mile23’s picture

Issue tags: +Needs documentation
hussainweb’s picture

Status: Postponed » Needs review
FileSize
11.34 MB

#2635584: Move core/.gitignore policies to composer events/packaging scripts is in. Setting it back to RTBC tentatively. Should be needs work for documentation once the tests run. What kind of documentation updates are you thinking?

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

I've taken an initial stab at a change notice for this: https://www.drupal.org/node/2648064

Not sure if it needs more documentation than that?

dawehner’s picture

Well, this would mean that we need to update the documentation which explains how to work on Drupal to also include the composer install step, so also some link to install composer etc.

timmillwood’s picture

There is no installation documentation relating to downloading via git (https://www.drupal.org/documentation/install/download), all the documented workflows won't change with this commit.

catch’s picture

That's because right now it's no different to getting a tarball, it will be different after this patch, so will need documentation.

timmillwood’s picture

Added documentation for a basic git workflow https://www.drupal.org/documentation/install/download#git

mradcliffe’s picture

Maybe a section for Composer or getting dependencies specifically for Drupal >8.1.x.

It looks like the main mentor doc page links to https://drupal.org/dev-env, which does link off to the install page so that checks out. The contributor task pages may also need to be updated such as https://www.drupal.org/contributor-tasks/manual-testing, https://www.drupal.org/node/157602 (Local Server Setup), and several of the programmer tasks. We should create a follow-up in the Mentoring issue queue to work on that.

https://www.drupal.org/node/147789 probably doesn't need a specific mention because it's in the installation instructions.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +8.1.0 release notes

Based on discussions with @xjm and @catch, I think we need to address #171 before we commit this. It is important that contributors are still able to contribute by following instructions. Also I think we should draft something for the 8.1.0 release notes. We can also use this text for a g.d.o core announcement when this is committed as it is likely the many build processes will be broken by this.

I think it is fantastic that we're going to be able to do this for 8.1.0 - great work everyone!

alexpott’s picture

Also @xjm pointed out that we should add this to CHANGELOG.txt since this is something people should know about when upgrading to 8.1.x

timmillwood’s picture

Adding a changelog update:

Drupal 8.1.0, xxxx-xx-xx (development version)
------------------------
- Removed vendor from the root directory
    * Drupal.org packager adds vendor to zip / tarball
    * Composer install must be run to get dependencies when not using
      zip / tarball version

Will work on doc suggestions from #171 now.

timmillwood’s picture

timmillwood’s picture

Status: Needs work » Needs review
kim.pepper’s picture

Go @timmillwood!!

Mile23’s picture

+1 on all this. So close to RTBC...

I'd change this:

Drupal 8.1.0, xxxx-xx-xx (development version)
------------------------
- Removed vendor from the root directory
    * Drupal.org packager adds vendor to zip / tarball
    * Composer install must be run to get dependencies when not using
      zip / tarball version

To this:

Drupal 8.1.0, xxxx-xx-xx (development version)
------------------------
- Removed Composer-managed vendor directory from the git repo
    * Drupal.org packager adds dependencies to zip / tarball, use as-is
    * Composer install must be run to get dependencies when not using
      zip / tarball version
    * See https://www.drupal.org/documentation/install/download#git
      for instructions
Mile23’s picture

Status: Needs review » Needs work
timmillwood’s picture

Status: Needs work » Needs review
FileSize
11.34 MB

Updated based on #178.

hussainweb’s picture

Thanks @Mile23, @timmillwood. I suggest a few more edits to make it easier to understand.

Drupal 8.1.0, xxxx-xx-xx (development version)
------------------------
- Removed Composer-managed vendor directory from the git repo
    * Drupal.org packager adds dependencies to zip and tar files. This can be used without any further steps.
    * When not using zip / tar files, e.g. when using a git clone, run composer install to get dependencies.
    * See https://www.drupal.org/documentation/install/download#git
      for instructions

EDIT: The above is not on 80 character boundary. It is just a suggestion for text. Please break where appropriate.

timmillwood’s picture

claudiu.cristea’s picture

This is a patch only for review purposes, obtained with git diff -D.

Mile23’s picture

Status: Needs review » Reviewed & tested by the community

Good new release notes. Thanks @claudiu.cristea for the readable patch.

Diggit.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Okay let's do this well before 8.1.0 is released so we can ensure that d.o works as expected. I'll add a post to g.d.o/core to notify everyone.

I've credited everyone who has commented at least 3 times since most of the work here has been done in other issues and discussing how we're going to get this done.

Committed c6c6913 and pushed to 8.1.x. Thanks!

alexpott’s picture

  • alexpott committed c6c6913 on 8.1.x
    Issue #1475510 by timmillwood, hussainweb, davidwbarratt, Mile23,...
Wim Leers’s picture

Status: Fixed » Active

It looks like this broke testbot?

See e.g. https://dispatcher.drupalci.org/job/default/76431/console (for the super trivial #2602536: Unused local variable in testSerialization()):

00:00:00.778 find: cannot delete `/var/lib/drupalci/web/jenkins-default-76418/vendor/phpspec/prophecy/spec/Prophecy/Exception': Directory not empty
00:00:00.786 find: cannot delete `/var/lib/drupalci/web/jenkins-default-76418/vendor/phpspec/prophecy/spec': Directory not empty
00:00:00.789 find: cannot delete `/var/lib/drupalci/web/jenkins-default-76418/vendor/phpspec/prophecy/src': Directory not empty
…
00:00:01.005 find: cannot delete `/var/lib/drupalci/web/jenkins-default-76418/vendor/composer/installers/tests/Composer/Installers': Directory not empty
00:00:01.009 find: cannot delete `/var/lib/drupalci/web/jenkins-default-76418/vendor/composer/installers/tests/Composer': Directory not empty
00:00:01.159 Build step 'Execute shell' marked build as failure

Another with similar problems: the equally trivial #2641540-11: Replace deprecated usage of entity_create('entity_test_mulrev') with a direct call to EntityTestMulRev::create().

Mixologic’s picture

Status: Active » Fixed

There seemed to be three tests in quick succession that hit the same testbot that was failing, however all other running tests indicate that this is working in general, and may have just been an artifact of running multiple styles of tests on the same bot (some without composer, and some with).

Its a minor permissions issue on the bots if the problem resurfaces, but so far it seems as though we're not continuing to have issues.

We'll look into making sure that perms problem doesnt happen again.

Wim Leers’s picture

Thanks!

nod_’s picture

Status: Fixed » Needs work

After updating my repo and trying to run composer install:

Could not fetch https://api.github.com/repos/sebastianbergmann/php-file-iterator/zipball..., please create a GitHub OAuth token to go over the API rate limit
Head to https://github.com/settings/tokens/new?scopes=repo&description=XXX+2016-...
to retrieve a token. It will be stored in "/home/theodore/.composer/auth.json" for future use by Composer.

I don't use composer at all so I don't think my config is to blame (maybe a lack of config is to blame). Anyway it seems that to contribute to core one would now need a github account.

  • catch committed 3ad9f73 on
    Revert "Issue #1475510 by timmillwood, hussainweb, davidwbarratt, Mile23...
catch’s picture

We've seen the test fail since #18 I think.

nod_'s issue in #191 is worse than a bit of instability though. The github dependency is one thing, requiring people to mess around with auth tokens because composer is inefficient is another. I think we need a new issue for that, and postpone this until it's resolved. Rolling back for now.

catch’s picture

Here's docs:
https://getcomposer.org/doc/articles/troubleshooting.md#api-rate-limit-a...

That is annoying to expect 3,000+ people to do as a a pre-requisite to rolling Drupal core patches. And I don't think we can require a github registration without a policy change. You're expected to put that in your composer.json - but we already have a project composer.json and putting a github auth token in there would defeat the point.

https://github.com/composer/composer/pull/4685 is the upstream issue that would help with this a bit - but caching doesn't help if the first request fails.

Checked with nod_ in irc and this was the first ever core composer install on his system "first time I ran it got this after the 64 or 65th download", so yeah the caching issue isn't enough.

nod_’s picture

From github: "Unauthenticated requests will be limited to 60 per hour unless you include your OAuth client and secret." https://developer.github.com/changes/2012-10-14-rate-limit-changes/

davidwbarratt’s picture

You can also use the --prefer-source option and you will avoid the GitHub authentication.
https://getcomposer.org/doc/03-cli.md#install

The reason you hit the API is that you're getting the dist which is a tar.gz or zip file.

davidwbarratt’s picture

I think the testbot should use --prefer-source as well so we don't hit the GitHub api. :)

neclimdul’s picture

--prefer-source has its own problems. larger, slower downloads and entire git repos laying around.

Do we have any ideas or criteria for a path forward to re-commiting this? I don't think maintaining /vendor is a sustainable proposition.

davidwbarratt’s picture

I mean your choices are either to use source (with the problems you pointed out) or use dist. Since dist comes from GitHub and GitHub requires an API key, then you are back to that problem.

The only other way around this that I can see is that we could have our own packagist (or a mini packagist, i.e. just a json file) that is hosted on d.o. It would only need drupal core's dependencies and we could point the dist to a zip/tar.gz that is also hosted on d.o.
https://getcomposer.org/doc/05-repositories.md#hosting-your-own

This is basically what we do for drupal contrib:
https://packagist.drupal-composer.org/

catch’s picture

The only other way around this that I can see is that we could have our own packagist (or a mini packagist, i.e. just a json file) that is hosted on d.o. It would only need drupal core's dependencies and we could point the dist to a zip/tar.gz that is also hosted on d.o.

I think that's well worth looking into. It would also brings us down to a single point of failure instead of, um, two single points of failure.

I just tested this locally and it's very easy to reproduce. Another way to mitigate it would be to use composer install--no-dev - it looks like a lot of the packages are little pieces of phpunit. In an hour I'll try again with that ;) But that doesn't help if you want to do actual development of core.

klausi’s picture

Status: Needs work » Needs review

Please update your composer version, it will automatically download with git clone if it runs into the github api limit.

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

IMHO this is a non-issue.

Drupal 8 has all been about getting off the island and embracing the wider PHP community. The wider PHP community all work with composer. All our dependencies (which as a community we have committed back to) are hosted on github, so requiring a github token is rather trivial.

There are many work arounds, but why? what's it going to solve? nothing! it will just create more Drupalisms and get us back to the D7 days.

It's not the first time this step of setting up a token has come up. Drupal.org has a token for the packager. DrupalCI has a token for testbot. I have a token for creating the patch in #182.

I really don't understand the issue.

Also to the point in #194, the github token goes in your auth.json, not in your composer.json. We could even ship drupal with an example auth.json, but ideally you'd put it globally in ~/.composer/auth.json.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Well I just tried that and it half works.

You still get prompted for the token, and there's no indication that it will download from source when you don't give it a token.

I pressed enter to see what would happen, and it did indeed download one project from source. But then it prompts you again, so you have to sit there hitting enter for each project one at a time.

This is what it looks like:

Could not fetch https://api.github.com/repos/sebastianbergmann/recursion-context/zipball/994d4a811bafe801fb06dccbee797863ba2792ba, please create a GitHub OAuth token to go over the API rate limit
Head to https://github.com/settings/tokens/new?scopes=repo
to retrieve a token. It will be stored in "~/.composer/auth.json" for future use by Composer.
Token (hidden): 
No token given, aborting.
You can also add it manually later by using "composer config github-oauth.github.com <token>"
    Failed to download sebastian/recursion-context from dist: Could not authenticate against github.com
    Now trying to download from source
  - Installing sebastian/recursion-context (1.0.1)
    Cloning 994d4a811bafe801fb06dccbee797863ba2792ba

> Drupal\Core\Composer\Composer::vendorTestCodeCleanup
  - Installing sebastian/exporter (1.2.1)
    Downloading: Connecting...
Could not fetch https://api.github.com/repos/sebastianbergmann/exporter/zipball/7ae5513327cb536431847bcc0c10edba2701064e, please create a GitHub OAuth token to go over the API rate limit
Head to https://github.com/settings/tokens/new?scopes
to retrieve a token. It will be stored in "~/.composer/auth.json" for future use by Composer.
Token (hidden): 
No token given, aborting.
You can also add it manually later by using "composer config github-oauth.github.com <token>"
    Failed to download sebastian/exporter from dist: Could not authenticate against github.com
    Now trying to download from source
  - Installing sebastian/exporter (1.2.1)
    Cloning 7ae5513327cb536431847bcc0c10edba2701064e

> Drupal\Core\Composer\Composer::vendorTestCodeCleanup
  - Installing sebastian/environment (1.3.2)
    Downloading: Connecting...
Could not fetch https://api.github.com/repos/sebastianbergmann/environment/zipball/6324c907ce7a52478eeeaede764f48733ef5ae44, please create a GitHub OAuth token to go over the API rate limit
Head to https://github.com/settings/tokens/new?scopes=repo&description=
to retrieve a token. It will be stored in "~/.composer/auth.json" for future use by Composer.

That's better than getting completely blocked, but still pretty bad.

catch’s picture

It's not the first time this step of setting up a token has come up. Drupal.org has a token for the packager. DrupalCI has a token for testbot. I have a token for creating the patch in #182.

That's fourthree tokens. There are 3000+ core contributors.

I've worked on 7.x projects using composer a fair bit, but presumably because they go through drupal packagist for tarballs this hasn't come up.

Similarly composer update on core doesn't run into this either.

So this is specifically a new requirement for every core contributor, that was not properly discussed before this patch went in. If me and nod_ can hit it within a few hours of the patch landing, many more people will.

Just saying 'get off the island' isn't very helpful when the boat is leaky and the mainland is being stingy with passports.

neclimdul’s picture

So this seems to be mostly an issue for development and tracking /vendor with HEAD of any particular core branch. If we make that very explicit assumption then I think we can simplify the requirements rather then tracking every release for a large set of projects. The solution would be we could have our packaging script build and store a parallel -dev-vendor.zip that could download the entire vendor directory. We could then add a script in /core/scripts that could download that for people that have cloned the source.

This doesn't get rid of the possibility that you'll need to use the API though, this just significantly limits the effect. Bisecting and updating will still be affected for example because it won't track older or newer releases, but depending on what you're doing you may only be updating a handful of libraries which hopefully wouldn't hit the limit. If you're going back deep into the history you might just have to use github's API though.

Even if you have the API setup though, this actually might be handy for speeding up composer. ;)

alexpott’s picture

I like the idea of #205 I guess we need to have each minor and patch available...

So we can get vendor at 8.1.1, 8.1.0, 8.1.x, 8.2.0 and 8.2.x (for example) as these might all be different. More work for the DA though...

alexpott’s picture

I guess one issue with #205 is that the DA is still then having the additional bandwidth costs. It does decouple us from github but in reality we are coupled to github because that's where are dependencies are stored.

I'm sympathetic to both opinions... composer tells people what to do and falls back to cloning but OTOH this does introduce and extra setup for people that has not been documented on the g.d.o post or in the expected release notes.

neclimdul’s picture

My hope was it was /less/ work then then building a github mirror even if its for a subset of packages. ;)

That said, releases already have vendor built in so I'm not sure we need the vendor bootstrap for minor/patch releases. If its not a lot more work someone might find it useful but I don't think its necessary.

klausi’s picture

Sigh, composer must have a hidden feature that it falls back to installation from source (git clone) but only in non-interactive shells. That's why I have never seen the github token auth prompt when using composer from Travis CI. It simply reported "Failed to download doctrine/lexer from dist: Could not authenticate against github.com. Now trying to download from source". This should always be done even for interactive shells, I'm going to file an issue against composer.

I can now reproduce locally when clearing my ~/.cache directory and running composer install.

klausi’s picture

Eric_A’s picture

The only other way around this that I can see is that we could have our own packagist (or a mini packagist, i.e. just a json file) that is hosted on d.o.

Are we talking mirror/proxy with Satis/ Toran Proxy?

Crell’s picture

This does seem like the exact case that Satis is useful for. I've played with it a bit, but not completely. I believe mikey_p has it up and running at his employer. If you have Satis mirror a remote project then it will download a tarball of every release of the project ever and save it locally. That's not the most efficient approach for Satis, but it does mean any further downloads from it are just plain static HTTP GET on a file, kthxbye.

Mirroring the packages that core uses seems like a reasonable and low-effort solution. It wouldn't help with people downloading dependencies for contrib modules, but at that point it's out of our scope anyway so that's fine.

chx’s picture

The whole debate misses the elephant in the room: composer, at this stage, is unusuably slow for this many packages. For any operation it will ping every package to make sure the cache is up to date; but even just reading the cache is painful to watch. Running a composer update drupal/somemodule takes several magnitudes more time than cd modules/somemodule;git pull;cd ../.. Try composer -vvv to see. All these years noone benchmarked whether this is a viable solution (hint: nope).

This would not be the first time the "PHP Renaissance" utterly fails at the scale of Drupal.

Mile23’s picture

I was going to time some stuff but ran into this: #2664274: Combination of --prefer-dist and .gitattributes confuses our vendor test cleanup

And you'll have to admit: Using a tool to install dependencies, and having it not only tell you when there is a dependency mismatch but also when the file structure changes is worth a couple minutes. :-)

Mixologic’s picture

barryvdh’s picture

So a few summary points:

- Drupal can still provide releases with the vendor dir included, to prevent these limits for most users
- Developers can build from source if they don't want to register with Github
- Developers can register with Github if they want faster downloads

Imho, we shouldn't look at this as 'Drupal developers' but 'PHP developers'. Yes, Composer/Github can be a bit confusing/intimidating the first time, but we shouldn't need to hide this for all developers. If we expect Drupal developers to also contribute to other projects, they will need a Github account anyways. And there is a very big change developers are using Composer (+github auth) anyways.

The issues with Github tokens can probably be mostly fixed by providing some clear message in the install/developer guide and also in Composer itself (as suggested in the linked issue).

With solutions like own mirrors/caches, you're probably making it unnecessary hard for yourself..

xjm’s picture

Issue tags: -8.1.0 release notes

(Untagging since it is not in 8.1.x.)

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

barryvdh’s picture

According to the linked Composer issue, Github now removed the API limit for archive downloads, so this should speed up all installs and stop asking for OAUth credentials.

https://github.com/composer/composer/issues/4884#issuecomment-195229989

We've spent some time digging in, considering all the options to resolve your issues, weighing your needs against infrastructure strain and availability.

I'm happy to report that our Infrastructure team believes that due to their work on our Git backend since these APIs were introduced, we're now able to drop these rate limits on the API side. We deployed those changes a couple of hours ago. Getting an archive link [1] via the API will no longer count against your hourly rate limit (authenticated or not). This should make Composer installs happy.

catch’s picture

Status: Needs work » Needs review

github removing the API limits makes this feasible again.

timmillwood’s picture

Very exciting news, are we definitely too late to recommit this to 8.1.x?

alexpott’s picture

I think we should consider doing this for 8.1.x-beta now github has removed its limits. We've already published a g.d.o/core so people know this is coming and d.o is ready.

Also I think we need to update the example.gitignore patch.

timmillwood’s picture

The Change Record will need re-publishing, I switched to draft when this got reverted not to confuse people using 8.1.x.
[#2648064]

nod_’s picture

Removed my github creds from composer and things worked as expected, no hit on my github api rate limit either (if you want to check for yourself, it's over there: https://api.github.com/rate_limit)

I'm assuming github know what it's doing, but should we wait a bit to make sure they don't revert the change because of unforeseen reasons on their end?

( edit ) or at least wait for their official announcement (if they're going to make one).

The last submitted patch, 182: remove_external-1475510-182.patch, failed testing.

catch’s picture

For me this would be fine to go in during the beta.

@nod_ if we don't do this in the next week or so it's going to get bumped to 8.2.x, would be a shame to do that due to github being slow to update docs.

It would obviously be very annoying if github reverses this decision though of course, but there is also plenty of discussion and two PRs for using the direct download URLs in composer, which if github did reverse their decision I'd expect to be considered more seriously. If we ended up stuck without either of them, we could revert the patch yet again.

The last submitted patch, 183: 1475510-183-for-review-do-not-test.patch, failed testing.

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

It's been merged to master and deployed to production so you can rely on it now.

I'd say we're good to go!

nod_’s picture

They're probably using git in-house, it's not like they can't revert :þ

I did say "should?", if we shouldn't wait that's cool with me, i'm not the one that gets impacted the most.

heddn’s picture

I've opened #2685947: Rewrite git history to remove vendor to continue the conversation after this gets committed.

xjm’s picture

Issue tags: +beta target

Best to do this sooner rather than later. @alexpott and @catch have also agreed with this going in during beta above.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.2.x and 8.1.x, thanks!

Need to keep an eye on DrupalCI and packaging, but hopefully that's it for this issue now.

xjm’s picture

Issue tags: +8.1.0 release notes
pfrenssen’s picture

Awesome!!

timmillwood’s picture

Re-published the change record [#2648064]

  • catch committed 2971d9c on 8.2.x
    Issue #1475510 by timmillwood, hussainweb, davidwbarratt, alexpott,...
wesruv’s picture

Made a ticket for this here: https://www.drupal.org/node/2691003

Here's the issue:

From a developer experience perspective:
- Checked out 8.1.x
- Made my DB
- Made my settings file
- Visited the site... 500 error with no explanation
- Went to apache log, found :
PHP Fatal error: require(): Failed opening required '/var/www/drupal8/vendor/autoload.php' (include_path='.:/usr/share/php:/usr/share/pear') in /var/www/drupal8/autoload.php on line 14

Didn't really understand what I was supposed to do, thankfully I'm at the Midcamp sprint and was told I need to run composer install to get the external deps.

I like that we did this, but think we need to give an error that let's the developer know they should run composer install.

dawehner’s picture

We could do a file_exists in index.php, which at least should never have a caching issue on real sites. Please open up a dedicated new issue to discuss this problem.

Thank you.

geerlingguy’s picture

Status: Fixed » Closed (fixed)

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