-
Notifications
You must be signed in to change notification settings - Fork 6
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
Changes from 5 commits
0aaab17
2640e93
124dad5
ac7a81e
3c8fbd8
5d549c6
31291ef
69ca2ab
809ce4c
3089600
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
|
||
#### Global install | ||
|
||
This is by far the easiest and most convenient method. Simply having all the tools available system wide. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 @brammittendorff might have a suggestion? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 😞 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not use:
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Potherca This is also a way of doing it. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @frenck even better There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Potherca @brammittendorff added GPG. |
||
``` | ||
|
||
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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure we should advise people to edit their There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 **": "*" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would suggest using There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
}, | ||
"require-dev": { | ||
"dealerdirect/qa-tools": "*" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
#!/usr/bin/env bash | ||
set -eu | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer seeing the more verbose There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Agreed |
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even though this script does not pipe anything, I would recommend adding
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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!" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is an error it should be redirected to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
exit 1 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
fi | ||
|
||
# Ensure Composer is installed/updated | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ***" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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');" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)" = \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not entirly sure about the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎱 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It has been decided:
|
||
rm composer-setup.php | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be more robust to add the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These two |
||
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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could also just run the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even better 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer to see an explicit |
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.
Sentence is crooked. Either
or
It might also be useful to list the "different ways" before going into detail. Makes it easier for people to scan ahead.