-
-
Notifications
You must be signed in to change notification settings - Fork 642
Issue #506: Add support for composer based site builds #694
Conversation
drupal_major_version: 8 | ||
drupal_core_path: "/var/www/drupalvm/drupal" | ||
drupal_core_path: "{{ drupal_composer_path }}/web" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of the box this should still work for makefiles. It's a bit weird having the otherwise empty drupal/
directory though. Not sure we even need to suggest changing it though. The recommended way is composer
and if it still works for makefiles, that's good enough in my opinion.
This is a rather significant change though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As everyone has their config.yml
configured already, it's technically not a breaking change though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I generally see projects use docroot
rather than web
for the actual drupal directory... could we do that if we use composer.json
and install it in the proper directory?
@geerlingguy if you like this approach and travis passes, tell me and I'll start working on updating the docs (I also need to manually test that everything works with a custom |
# used instead of the 'drupal_composer_package'. | ||
build_composer: true | ||
drupal_composer_path: "/var/www/drupalvm/drupal" | ||
drupal_composer_package: "drupal-composer/drupal-project:8.x-dev" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking we could have an example.composer.json
and build from that, just like using drupal.make.yml
; this approach isn't necessarily wrong, but I feel like it would be more intuitive to have a composer.json and build from that rather than building the composer install using a CLI command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might be correct, drupal-project
adds some complexity. However, with this approach we don't need to maintain a composer.json
at all, and it also removes some steps from the quick start guide.
But if you prefer, I can add a basic example.composer.json
and maybe add the create-project
command installation as an option. build_composer_project: true
or something.
Some context to drupal-project and web/ vs docroot/ - drupal-composer/drupal-project#64 There is a little work being done there to make specifying the docroot easier. |
In case we want to support All 3 setups are being tested on travis. Note: docs are entirely out of sync. |
The CI fail is unrelated, I'm assuming it works but will test it locally as well. @geerlingguy want me to keep support for |
Erm just realized it's a bit chaotic with I guess we need to commit |
I don't really like placing the @geerlingguy thoughts? Personally I prefer the first approach more to be honest. Just default to |
@oxyc - Let's do the first and use drupal-project. We can pivot at some point if needed, and it makes more sense to do that and simplify overall setup, I think. People who are deep enough into the composer workflow should/will use Drupal VM as a dependency anyways, so no need to support only composer.json builds out of the box. |
Sorry for creating the bit of rework, but I'm coming around to what you originally proposed. Will need to poke and prod locally in a bit. |
copy: | ||
src: "{{ drupal_composer_json }}" | ||
dest: "{{ drupal_composer_install_dir }}/composer.json" | ||
when: drupal_composer_json is defined and not drupal_site.stat.exists |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't needed for the usual setups (BLT or projects where Drupal VM is in vendor), but I added it as it could be useful in some scenarios. Eg. provisioning production environments (same logic as for makefiles).
If everything looks okay, I'll get started on adjusting the docs and running some more tests in a bit. |
Testing locally. |
@oxyc - CentOS 7 build failed with:
|
We'll see how test run 777 goes. |
@oxyc is the |
file: | ||
path: "{{ drupal_core_path }}" | ||
state: directory | ||
recurse: yes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, what was the reason behind this being recursive?
Hmm not sure... I'll see if I can reach them on twitter. |
See also: drupal-composer/drupal-project#137 (comment) — but yeah, this is kind of a deal breaker imo, as it's been about 10 minutes (still killing my CPU currently), and I'm on a 1.7 Ghz i7 on VMware with an SSD, and a 100 Mbps internet connection. I can't even begin to imagine how many hours it will take a poor soul stuck on an old PC with an Atom processor and a 2 Mbps internet connection :P |
Everything installed successfully, so there's that :) |
This run had all 3 installs:
Yeah that's really bad. I'm on a 1.1ghz Macbook and it only takes a few minutes for me though. Haven't even noticed it's slower than the makefiles. |
Maybe we could still default to the makefile but support composer.json, then... I'm going to test again on a rebuild and see if it's any quicker. |
Some stats from my end:
|
@oxyc could we use https://github.com/hirak/prestissimo somehow? |
Interesting! I'll give it a try once I get these silly tests to pass. |
Updated my stats—6 min vs 22 minutes is a definite deal-breaker :( We could default to makefile since it's waaaay faster, and then allow people to use composer if they'd like. Unless we can figure out a way to get it at least less than 2x slower than makefiles. Maybe prestissimo will work. |
Grr... this thread has some great data in it though—we can do a follow-up issue to look at how to make things faster. Maybe there's a trick someone has up their sleeve to supercharge the install. |
We could look for |
@oxyc you could try syncing the host
I assume the local cache is why it's not such a major issue for most ppl not to mention pulling projects with such a huge history like drupal.. Shallow clones would be a big help! -- composer/composer#4961 |
For comparison, try vagrant-cachier. It supports composer - http://fgrehm.viewdocs.io/vagrant-cachier/buckets/composer/ |
@thom8 yeah we should probably add a section on how to improve performance. That would be one way, and A lot of back and forth here. Now I'm back to drush makefile being the default, and having |
a142bd8
to
838524f
Compare
Tested locally:
|
drupal_major_version: 8 | ||
drupal_core_path: "/var/www/drupalvm/drupal" | ||
drupal_core_path: "{{ drupal_composer_install_dir }}/docroot" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can also keep the old behaviour as makefiles are still the default. Just need to mention that this needs to be changed in the composer docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, let's stick to the old for now, since we're not defaulting to composer.
I'll see if it works using the generic cache bucket. |
Not sure why, but For reference I was trying a config.cache.auto_detect = false
config.cache.enable :generic, { :cache_dir => '/home/vagrant/.composer/cache' } as per http://fgrehm.viewdocs.io/vagrant-cachier/buckets/generic/ Even with auto_detect on, nothing gets cached though. Not sure if this is just happening on my laptop. I have some WIP docs but as I can't actually get the shared cache to work I wont add it. https://gist.github.com/oxyc/7c768feaa3c16a9f52c1196689f3a0a7 (edit: I missed that the scope setting was required, it works now so I'm running a benchmark). |
I think if we get in the other two changes mentioned above, I'm happy to merge this and get the 3.1.0 release tagged. Hopefully we can find a way to get performance to the point where composer is the default by .2 or .3! |
drupal_major_version: 8 | ||
drupal_core_path: "/var/www/drupalvm/drupal" | ||
drupal_core_path: "{{ drupal_composer_install_dir }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave this as the default /var/www/drupalvm/drupal
(the composer
in the var might confuse people who aren't using composer, which at this point is likely the majority of users).
I'm ready to merge once that conflict is resolved. A long road to this point, but it's well worth it. This will be an awesome improvement! |
Whops I somehow managed to get in the commit from my other PR. Getting late.... Want me to squash these? A lot of back and forth going on... |
If it's quick, you can... otherwise I can live with the history. It's not all bad to have some of the pivots as part of the history, though it makes my release note creation a little more fun :D |
Looks good. Once test is green, merge button will be pressed 👍 |
Issue #506. This is still WIP, just checking travis' opinion.