Skip to content

Added install script & documentation for installing this repository. #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Oct 27, 2016
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,48 @@ The following packages are suggested:
[phpDox]: http://phpdox.de
[Sami]: https://github.com/FriendsOfPHP/sami

## Usage

This is a simple metapackage, but still can be used in different ways.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sentence is crooked. Either

This is a simple metapackage, but it still can be used in different ways.

or

This is a simple metapackage which can be used in different ways.

It might also be useful to list the "different ways" before going into detail. Makes it easier for people to scan ahead.


#### Global install

This is by far the easiest and most convenient method. Simply having all the tools available system wide.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This speaks of a personal preference. I would prefer the tone to convey a neutral point of view


The following script will install a system wide Composer for you, including the QA tools.

```bash
bash <(curl -S https://raw.githubusercontent.com/DealerDirect/php-qa-tools/master/bin/install.sh)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like telling people to pipe/redirect atbitrary code to bash. I also don't have any real alternative.. Other than maybe (which is not stable) or more work than it might be worth.

Maybe @brammittendorff might have a suggestion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally know where you are going here... this discussion is probably older than Netscape v.s. Mosaic...

This was based on how other larger tools do this (e.g. Composer, Homebrew). In my point of view, it's all about trust. We have a review process and the code is opensource. Take a look and decide. I would suggest adding a notice or warning above this instruction.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The alternative would be to tell people to install composer themselves which would kind of defeat the purpose of the script 😞

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that warning people and telling them how to do it "safely" should be sufficient (like expressed here.

It would also help to sign commits and have 2FA required for direct contributors. (As a means for more trust).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice find! Will do a similar thing 👍

I totally agree on the last bit... little out of scope for this pull request. Sound like a 'guide'. Let's discuss this post daily and consider enforcing it.

Copy link
Contributor

@brammittendorff brammittendorff Oct 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use:

curl -o install.sh https://raw.githubusercontent.com/DealerDirect/php-qa-tools/master/bin/install.sh
bash install.sh

Yes you will split it up but you will let the user decide if he or she wants to see the file. You will give the power to the user to check that file for security reasons.

Maby you can add md5sum to check if the md5sum is correct. Yes I know you can fake this but it is a nice way of saying hey this file has the correct file integrity.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Potherca This is also a way of doing it.
https://rvm.io/rvm/security
They offer it as en extra installation method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is about as safe as we can make it without making it too difficult for common users.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@frenck even better

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

```

If you already have a global Composer setup, you can execute this to include the tools.

```bash
composer global require "dealerdirect/qa-tools"
```

#### Project install

The other option is to install this on a per project basis.

```bash
composer require --dev "dealerdirect/qa-tools"
```

Or modify your `composer.json` to include `dealerdirect/qa-tools` in the `require-dev` sections.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we should advise people to edit their composer.json file as this is considered a bad practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since some people still do (including myself... opinions...) I would suggest adding something like: "prefered method" to the first instruction.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with the wording "preferred method".


```json
{
"name": "acme/my-project",
"require": {
"** your requirements **": "*"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest using instead of "** your requirements **": "*".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually changed this one during dev a couple of times. I even used a working symfony example. Not sure what to use. ... is fine by me.

},
"require-dev": {
"dealerdirect/qa-tools": "*"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to add a version number rather than "no constraint"

}
}
```

## Contributing

This is an active open-source project. We are always open to people who want to use the code or contribute to it.
Expand Down
74 changes: 74 additions & 0 deletions bin/install.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
#!/usr/bin/env bash
set -eu
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer seeing the more verbose set -o errexit as that is a bit more understandable than -e (and set -o nounset rather than -u). It would also be helpful to add a comment after each flag for developers less familiar with bash.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Agreed


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though this script does not pipe anything, I would recommend adding

set -o pipefail

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

# Ensure PHP is installed
if ! command -v php > /dev/null 2>&1; then
echo "PHP is not found, installation aborted!"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is an error it should be redirected to STDERR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

exit 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As there is more than one exit scenario I would prefer to see a different exit code than 1

fi

# Ensure Composer is installed/updated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of a comment it might be better to have a function named ensureComposerInstalled

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to keep it simple, top down. But I agree as well... 👍

if ! command -v composer > /dev/null 2>&1; then
echo "*** Installing Composer ***"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having a personal standard for messages I would prefer to see a convention being used. (For instance the Heroku output formats).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah... Since we don't have a convention for it yet... Nevertheless, nice link, will "borrow" some. 😉

php -r "copy('https://getcomposer.org/installer', 'composer-setup.php');"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For various reasons I would advise against using PHP here. Why not just use bash? (Or build the whole thing in PHP).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is taken directly from the official Composer documentation. I do agree, nevertheless, this is how it is documented by Composer (which has changed a lot in the last couple of years).


if [ \
"$(curl -S https://composer.github.io/installer.sig)" = \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

"$(php -r \"echo hash_file\('SHA384', 'composer-setup.php'\);\")" \
]; then
php composer-setup.php --quiet --filename=composer
sudo mv composer /usr/local/bin/composer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not entirly sure about the sudo, then again, chowning would probably also require sudo, so... meh.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎱

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has been decided:

@8BallTweets Should we use `sudo mv`?

— 422 - Unprocessable (@Potherca) October 25, 2016

@potherca - Yes

— Ask the Magic 8 Ball (@8BallTweets) October 25, 2016

rm composer-setup.php
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be more robust to add the rm to a cleanup function called from an EXIT trap.
(This would also avoid having 2 duplicate lines for removing the file).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some catches with using a trap, but in this case, yeah... 👍

else
>&2 echo 'ERROR: Invalid composer installer signature'
rm composer-setup.php
exit 1
fi
fi

echo "*** Updating shell profiles ***"
# Add Composer's Global Bin to ~/.profile path
if [[ ! -f "$HOME/.profile" ]]; then
touch "$HOME/.profile"
fi
if grep -q -F "export COMPOSER_HOME=$HOME/.composer" "$HOME/.profile"; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two ifs belong in an else. If we've just created the file it does not make sense to grep it.

echo "export COMPOSER_HOME=$HOME/.composer" >> "$HOME/.profile"
fi
if grep -q -F "export PATH=$PATH:\$COMPOSER_HOME/vendor/bin" "$HOME/.profile"; then
echo "export PATH=$PATH:\$COMPOSER_HOME/vendor/bin" >> "$HOME/.profile"
fi

# Source the .profile to pick up changes
# shellcheck source=/dev/null
source "$HOME/.profile"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could have unintended side-effects, depending on what the user has (been told by other programs to) put in their .profile

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It "should not", but I like the don't assume approach. Replacing it with a success message at the end including an instruction would make more sense I guess. 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also just run the export COMPOSER_HOME=$HOME/.composer and/or export PATH=$PATH:\$COMPOSER_HOME/vendor/bin in the relevant if to have the same effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even better 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding your last comment... actually, you can't...


# Test if ZSH is installed
if command -v zsh > /dev/null 2>&1; then
# Add Composer's Global Bin to ~/.zprofile path
if [[ ! -f "$HOME/.zprofile" ]]; then
touch "$HOME/.zprofile"
fi
if grep -q -F "export COMPOSER_HOME=$HOME/.composer" "$HOME/.zprofile"; then
echo "export COMPOSER_HOME=$HOME/.composer" >> "$HOME/.zprofile"
fi
if grep -q -F "export PATH=$PATH:$COMPOSER_HOME/vendor/bin" "$HOME/.zprofile"; then
echo "export PATH=$PATH:$COMPOSER_HOME/vendor/bin" >> "$HOME/.zprofile"
fi

# Source the .zprofile to pick up changes
# shellcheck source=/dev/null
source "$HOME/.zprofile"
fi

# Install/Update Dealerdirect QA tools
if ! grep -q -F "dealerdirect/qa-tools" "$HOME/.composer/composer.json" > /dev/null 2>&1; then
echo "*** Installing Composer Prestissimo in order to speed up next steps ***"
composer global require "hirak/prestissimo:^0.3"
echo "*** Installing Dealerdirect PHP QA Tools ***"
composer global require "dealerdirect/qa-tools:@dev"
echo "*** Removing local Prestissimo dependency ***"
composer global remove "hirak/prestissimo"
else
echo "*** Updating Dealerdirect PHP QA Tools ***"
composer global update "dealerdirect/qa-tools:@dev"
fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to see an explicit exit 0;