Skip to content
This repository was archived by the owner on Aug 31, 2023. It is now read-only.

Rework the init files and configs#8

Closed
karbowiak wants to merge 7 commits intohhvm:masterfrom
karbowiak:master
Closed

Rework the init files and configs#8
karbowiak wants to merge 7 commits intohhvm:masterfrom
karbowiak:master

Conversation

@karbowiak
Copy link
Contributor

  1. Add hhvm-fastcgi.hdf with some config parameters for running hhvm in fastcgi move
  2. Rework hhvm.fastcgi init to not use the defaults/hhvm-fastcgi file, since you have it all in the hdf anyway
  3. Fix indentation in hhvm-fastcgi init file
  4. Rename server.hdf to hhvm.hdf to better indicate it belongs to the hhvm init
  5. Reapply appropriate chmods, because sublime hates me, and sets it to 644

Overall, a bit easier to work with, even if the PR looks garbled :(

Hopefully @dol is ok with me reworking his init

@dol
Copy link
Contributor

dol commented Jan 10, 2014

Some thoughts on my side.

Add hhvm-fastcgi.hdf with some config parameters for running hhvm in fastcgi move

I agree. I separate file has it advantage. See #9.

Rework hhvm.fastcgi init to not use the defaults/hhvm-fastcgi file, since you have it all in the hdf anyway

Disagree. Settings like 'RUN_AS_USER', 'CONFIG_FILE' and 'ADDITIONAL_ARGS' can not be set/overwritten the hdf conf. This is useful when customizing the startup. Take a look at hhvm -v. hhvm has more args to set on startup.

Fix indentation in hhvm-fastcgi init file

Disagree. You are using tabs, which is not recommend according to https://lists.debian.org/debian-devel/2012/03/msg00052.html

Rename server.hdf to hhvm.hdf to better indicate it belongs to the hhvm init

No sure hhvm.hdf is a clear name. See my recommendation notes below.

Reapply appropriate chmods, because sublime hates me, and sets it to 644

Please squash the commits -> http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html

PS: I recommend to use separate git branches for every PR.

My vote: Don't merge.

Opened issue #9 to discuss the introduction of a separate config file for FastCGI.

@karbowiak
Copy link
Contributor Author

Coolio, i'll close this PR, and look at fixing the issues, then retry and issue a new PR :)

@karbowiak karbowiak closed this Jan 10, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants