Improve buildkit installation docs #224

seancolsen · 2017-07-24T03:54:23Z

I installed buildkit on a new machine today and got hung up on some things. I hope these docs improvements will help others avoid the pitfalls I encountered today.

@xurizaemon @seamuslee001 care to take a look at this?

This is a direct copy-paste from one section of the guide to another, without any edits. When someone is installing buildkit, these steps are easy to miss if they're located in the civibuild file.

This was in the general "buildkit" page, and I think it fits better in the specific "civibuild" page.

- General formatting improvements. - Change "Ubuntu 12.04 or 14.04" to "Ubuntu 12.04 or later" because the buildkit install script is clearly designed to work for later versions of Ubuntu. - Relocate troubleshoot steps to after configuration steps (so the reader doesn't stop reading when they see "troubleshooting". - Higlight post-installation steps. - New instructions for manually ading webserver settings after running `amp config`. - More troubleshooting help.

seancolsen · 2017-07-24T03:57:17Z

BTW, one other thing I got hung up on was thinking that I had some sort of problem with MySQL packages when actually the real culprit was that I never ran amp config. I got turned around because of this warning message during the initial installation process:

"Warning: This system will not support php-mysql extension"

I made this PR which attempts to mitigate this pitfall by removing that warning message.

seamuslee001 · 2017-07-24T03:58:33Z

docs/tools/buildkit.md

+
+!!! tip "tips"
+    * Run this as a non-`root` user who has `sudo` permission. This will ensure that new files are owned by a regular user, and (if necessary) it enables `civibuild` to restart Apache or edit `/etc/hosts`.
+    * Pay close attention to any instructions given in the output of this command.  They may involve adding a line to your Apache configuration file.


Should also mention nginx perhaps change to "Pay close attention to any instructions given in the output of this command. They may involving adding a line to your web-server configuration (apache or nginx)

Fixed in bb27c6e

seancolsen · 2017-07-24T04:00:17Z

docs/tools/buildkit.md

+
+    * For nginx:
+
+        Create a new file `/etc/nginx/conf.d/buildkit.conf` with the following contents:


Did I get this path correct @xurizaemon ? /etc/nginx/conf.d/buildkit.conf

"It depends" ... For Debian, the path above is correct assuming they're using the Debian NginX packages yes. But maybe not all ... eg Debian and CentOS differ on Apache (/etc/apache2 vs /etc/httpd), for my OSX with homebrew it's /usr/local/etc/nginx/servers/buildkit.conf, and that's before you get into the madness of things like CPanel and their ilk, or people installing to /usr/local/nginx...

I see. Do you think it's okay to merge this as-is and assume that people will figure it out? Or do you have something better to propose here?

IMHO, you should only document amp with apache. nginx doesn't read .htaccess, and amp doesn't have enough plumbing to handle bespoke nginx config files. Basically, civibuild/amp needs a way to pass through per-application templates for the nginx config.

Without that, it's easier to (a) just tell people to use apache or (b) turn off amp's httpd integration and do that stuff manually.

tschuettler · 2017-07-24T09:46:55Z

docs/tools/buildkit.md

+
+!!! failure "Troubleshooting errors from `amp test`"
+
+    * Try manually adding settings to your sebserver, as described below.


Thanks @tschuettler. Fixed in 3485d72

totten · 2017-07-24T21:23:00Z

docs/tools/buildkit.md

+
+#### Manually adding settings to your webserver {:#amp-webserver}
+
+During one of the steps in the `amp config` process, `amp` will *attempt to* alter the system-wide settings for your local webserver (apache or nginx). On some platforms, `amp` is not able to perform the necessary configuration and you must do it manually by following these steps:


This seems like a misunderstanding -- amp makes no attempt to modify the system-wide httpd configuration. Rather, it tells the user to do it (and tries to give some pointers in the right direction). Of course, half the users ignore those instructions, so it's a recurring pain-point.

Random:

Guess: there's confusion because the vagrant and docker builds do that part automatically.

But as far as amp config and manual setup are concerned, these aren't automated. (And it can't be totally automated because there are too many edge-cases around different builds -- one might not appreciate that from a Debian/Ubuntu perspective, but from a MAMP/XAMPP/Bitnami/brew perspective it's more salient.)

It would be patch-welcome for amp config to put up a better prompt.

It would be patch-welcome for amp config to try to apply the needful interactively.

It would be patch-welcome for civi-download-tools --full to try to apply the needful.

Agree that it doesn't alter apache or nginx it does modify the hostfile which is http related, I suppose the most sensible thing i can think amp doing would be is to write out the include as a new file in the right directory. But as Chris pointed out finding that correct directory maybe tricky

Yup. Currently the amp config wizard just displays a handful of possibilities and moves on to the next step -- https://github.com/amp-cli/amp/blob/master/src/Amp/Command/ConfigCommand.php#L139

In an ideal world, that script would stop and ask the user what to do. Depending on what config files/directories are around, it would present a menu like:

[1] Include amp in /etc/apache2/httpd.conf [2] Include amp in /etc/apache2/conf.d/amp.conf [3] Include amp in /opt/xampp/etc/httpd.conf [4] Skip this step. I'll configure it manually.

If they choose 1-3, it would call sudo and try to update the selected file.

This way, it's harder for a user to ignore the step, but we still rely on their judgment for where/how to make the change.

@totten indeed I was misunderstanding the expected behavior of amp config. I have made changes in 0015e66 to clarify that, indeed, adding the settings to the webserver is mandatory.

seancolsen · 2017-07-24T22:24:24Z

I have made adjustments to this PR which I think addresses feedback from @totten @tschuettler and @seamuslee001. Will merge in 2 days if no further comment.

I recognize that the issue of "whether (and if so, how much) to document amp vis-à-vis nginx" is still somewhat outstanding in this PR. @totten seems to be arguing that we remove the references to nginx. @seamuslee001 has argued for more references to nginx in this same PR. So I'm a bit torn as to how to address this feedback, given that (personally) I don't know much about nginx. As such, I lean towards merging this content as-is and addressing this issue in a separate PR. My eagerness to merge is somewhat higher in this PR than in most because I know that installing buildkit has been super challenging for lots of people, so I want to get this published, even if it's not perfect.

seamuslee001 · 2017-07-24T22:37:21Z

Agree with you Sean, my argument for including nginx is that it seems to me from the line that Tim linked to earlier we are actively potentially supporting nginx for httpd as also evidenced by https://github.com/amp-cli/amp/blob/master/src/Amp/Command/ConfigCommand.php#L329 so to me it feels like we should document how to potentially set that up but noting it is still WIP for nginx but more solid for apache and apache is included in the download tools command

totten · 2017-07-25T05:30:01Z

@seamuslee001 I've been tempted to rip that code out. ;) In my mind, it's harmful/misleading because there is no good way to setup nginx, just a choice between "not working at all" (default) and "not working half the time" (maybe achievable, if you customize the vhost template and configure it to match a specific build type like drupal-demo or wp-demo).

IMHO, it's really a waste of time to document a broken design. If you want to put energy into nginx support, then try these:

Update amp (I've implemented; needs reviewer)
- As discussed in Nginx support amp-cli/amp#21 (comment), amp create should figure out the nginx template using a dot-file (eg MYBUILD/.amp/config.yml or MYBUILD/.amp/nginx-vhost.php).
- I've posted a PR to do this: Allow each amp instance to override the nginx vhost template amp-cli/amp#47 (example build http://think.hm/tmp/amp.phar-pr47) Let me know if itworks.
Update civicrm-buildkit (needs implementer and reviewer)
- Add files like app/config/*/nginx-vhost.php. (These would like https://github.com/amp-cli/amp/blob/master/src/Amp/views/nginx-vhost.php but be tuned to the needs of each CMS+Civi config.)
- Patch civibuild: it should either set an env var (NGINX_VHOST_TPL=$BLDDIR/app/config/*/nginx-vhost.php); or the install.sh scripts should copy over the file to CMS_ROOT/.amp/nginx-vhost.php.

seamuslee001 · 2017-07-25T05:32:38Z

@totten i would be ok with removing the documentation if we also removed the nginx option (could we comment it out) until we get buildkit ready for nginx (for some reason i thought @xurizaemon had done exactly that), Just figured that if it was a live option in the app we should have documentation for it if its really a half arsed set up we should pull it out of the app until its properly sorted IMHO

seancolsen · 2017-07-25T15:22:21Z

I added a "caution" admonition which recommends that users choose Apache over nginx. I will stick to my deadline of merging this PR tomorrow if there are no further concerns. Then, if we get more support nginx we can remove this word of caution. Or if we drop support for nginx altogether we can update the docs to reflect.

seancolsen added 3 commits July 23, 2017 20:00

Buildkit - relocate post-install steps

76c8105

This is a direct copy-paste from one section of the guide to another, without any edits. When someone is installing buildkit, these steps are easy to miss if they're located in the civibuild file.

Buildkit - relocate "Applying a patch" content

eb8e22c

This was in the general "buildkit" page, and I think it fits better in the specific "civibuild" page.

seamuslee001 reviewed Jul 24, 2017

View reviewed changes

seancolsen commented Jul 24, 2017

View reviewed changes

Buildkit - Mention nginx when mentioning Apache

bb27c6e

tschuettler suggested changes Jul 24, 2017

View reviewed changes

totten reviewed Jul 24, 2017

View reviewed changes

seancolsen added 2 commits July 24, 2017 15:44

Fix spelling error

3485d72

Simplify amp config steps

0015e66

Recommend that people use Apache instead of nginx

36b10fb

seancolsen merged commit 97d9082 into civicrm:master Jul 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve buildkit installation docs #224

Improve buildkit installation docs #224

seancolsen commented Jul 24, 2017

seancolsen commented Jul 24, 2017

seamuslee001 Jul 24, 2017

seancolsen Jul 24, 2017

seancolsen Jul 24, 2017

xurizaemon Jul 24, 2017

seancolsen Jul 24, 2017

totten Jul 24, 2017

tschuettler Jul 24, 2017

seancolsen Jul 24, 2017

totten Jul 24, 2017 •

edited

Loading

seamuslee001 Jul 24, 2017

totten Jul 24, 2017

seancolsen Jul 24, 2017

seancolsen commented Jul 24, 2017

seamuslee001 commented Jul 24, 2017

totten commented Jul 25, 2017

seamuslee001 commented Jul 25, 2017

seancolsen commented Jul 25, 2017


		* For nginx:

		Create a new file `/etc/nginx/conf.d/buildkit.conf` with the following contents:


		!!! failure "Troubleshooting errors from `amp test`"

		* Try manually adding settings to your sebserver, as described below.


		#### Manually adding settings to your webserver {:#amp-webserver}

		During one of the steps in the `amp config` process, `amp` will attempt to alter the system-wide settings for your local webserver (apache or nginx). On some platforms, `amp` is not able to perform the necessary configuration and you must do it manually by following these steps:

Improve buildkit installation docs #224

Improve buildkit installation docs #224

Conversation

seancolsen commented Jul 24, 2017

seancolsen commented Jul 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

totten Jul 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seancolsen commented Jul 24, 2017

seamuslee001 commented Jul 24, 2017

totten commented Jul 25, 2017

seamuslee001 commented Jul 25, 2017

seancolsen commented Jul 25, 2017

totten Jul 24, 2017 •

edited

Loading