Skip to content
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

Feature: Add one line install script and instructions to README #594

Merged
merged 8 commits into from
Oct 8, 2024

Conversation

kevincobain2000
Copy link
Contributor

Install cli could be more convinient for for users to not guess b/w arm, x86_64 and aarch etc..
Plus it will provide better installation for dev-ops team as one interface.

Verify

╰─$ cat install.sh| sh -
######################################################################## 100.0%
Installed successfully to: /Users/pulkit.kathuria/git/action-coveritup/app/frankenphp

╰─$ ls -lh frankenphp
-rwxr-xr-x  1 user.name  679754705   105M Feb 25 20:15 frankenphp

@kevincobain2000 kevincobain2000 changed the title Add one line install script and instructions to README Feature: Add one line install script and instructions to README Feb 25, 2024
install.sh Outdated
@@ -0,0 +1,57 @@
#!/bin/sh

BIN_DIR=$(pwd)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be great to only set this if it isn't set, so people can do:

BIN_DIR=/usr/bin curl -sLK https://raw.githubusercontent.com/dunglas/frankenphp/main/install.sh | sh

Copy link
Contributor Author

@kevincobain2000 kevincobain2000 Feb 25, 2024

Choose a reason for hiding this comment

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

Thanks

Added 19c126a and d0962c8 so user can do export BIN_DIR=... however still not setting /usr/bin as default path because it may require sudo as cat install.sh | sudo sh, because not always /usr/bin has the permissions for the user.

And adding extra instructions to set BIN_DIR in the README kind of complicates this simple process. Although added provision so user can install elsewhere too. That is why, pwd keeps things simple and user has a choice to move it to which ever bin dir of user's PATH choice.

Verification as below:

Fail

╰─$ sudo cat install.sh| sh
Password:
Warning: Failed to open the file /usr/local/bin/frankenphp: Permission denied
curl: (23) Failure writing output to destination

Success

╰─$ cat install.sh| sudo sh
Password:
######################################################################## 100.0%
Installed successfully to: /usr/local/bin/frankenphp

Copy link
Collaborator

@withinboredom withinboredom left a comment

Choose a reason for hiding this comment

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

Overall LGTM and I like it.

install.sh Outdated Show resolved Hide resolved
install.sh Show resolved Hide resolved
@withinboredom
Copy link
Collaborator

Looks like some issues with shellcheck in GHA (mostly variables need to be quoted). Can you address that?

@kevincobain2000
Copy link
Contributor Author

Thanks for the review @withinboredom
I have fixed the lint errors on spellchecker, I think rest of the lint errors are un-related to changes in this pull request. May want to re-run the CI.

@kevincobain2000
Copy link
Contributor Author

Ideally it'd be also nice to have this script hosted on https://frankenphp.dev/install.sh instead of raw.github... But that is topic for another day - and may depend on how is the docs pipeline structured.

@withinboredom
Copy link
Collaborator

There are still remaining issues (double quoting to prevent globbing). The paths should be quoted because if my current pwd is /home/withinboredom/bin*2024 it will fail in fun ways trying to download the file.

@kevincobain2000
Copy link
Contributor Author

Squashed commits and lints fixed.

@dunglas
Copy link
Owner

dunglas commented Feb 27, 2024

Is this safe?

Composer uses a checksum for instance: https://getcomposer.org/download/

install.sh Outdated Show resolved Hide resolved
@kevincobain2000
Copy link
Contributor Author

Is this safe?
Composer uses a checksum for instance: https://getcomposer.org/download/

No comments on this.
I mean I guess you could add this extra layer of security and have checksum. At the same time I also don't see the point as it is not a package manager. Homebrew installation doesn't have similar either and not many other curl shell installations.

@withinboredom
Copy link
Collaborator

I also don't see the point

Incomplete downloads, bad actors/broken caches on corporate middle-boxes, and a host of other things can cause checksums to be invalid. It's worth detecting it if we can, if for no other reason than to save people some time when things aren't working as expected.

@kevincobain2000
Copy link
Contributor Author

kevincobain2000 commented Feb 28, 2024

It is worth detecting. Yes. Just for this install script it seems over work. And may not have a simple straightforward command for the user. It’s a trade off.

@dunglas
Copy link
Owner

dunglas commented Feb 28, 2024

Ok, that works for me. That would be nice to have an official brew and maybe APT repositories too (easier to update).

@kevincobain2000
Copy link
Contributor Author

kevincobain2000 commented Feb 28, 2024

Yes. Given the popularity of this tool it’d be at least wise to reserve the names on brew and/or apt get etc. Publications can be delayed when you have time. Suggestion is to at least own the name before some bots started claiming it. It is just a naive thought.

@SirMishaa
Copy link

Hello,

They are remaining tasks to do before merging this PR ? Would be great to have this in the README.md file

@kevincobain2000
Copy link
Contributor Author

I believe this is ready. All changes from my side have been pushed.

install.sh Outdated

THE_ARCH_BIN=""
THIS_PROJECT_OWNER="dunglas"
DEST=$BIN_DIR/frankenphp
Copy link
Owner

Choose a reason for hiding this comment

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

Why not install directly the binary in /usr/local/bin/? This will simplify the one-liner provided in docs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that change makes an assumption that there is a /usr/local/bin to install to or that the user has permissions to do so. I, personally, would be wary of running a random curl'd script with root permissions just to install something.

I think you can probably make the docs more readable by suggesting BIN_DIR:

curl -sLK https://raw.githubusercontent.com/dunglas/frankenphp/main/install.sh | BIN_DIR=/usr/local/bin sh

and modify the script prompt for sudo if it fails to copy.

install.sh Outdated

THE_ARCH_BIN=""
THIS_PROJECT_OWNER="dunglas"
DEST=$BIN_DIR/frankenphp
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that change makes an assumption that there is a /usr/local/bin to install to or that the user has permissions to do so. I, personally, would be wary of running a random curl'd script with root permissions just to install something.

I think you can probably make the docs more readable by suggesting BIN_DIR:

curl -sLK https://raw.githubusercontent.com/dunglas/frankenphp/main/install.sh | BIN_DIR=/usr/local/bin sh

and modify the script prompt for sudo if it fails to copy.

install.sh Outdated Show resolved Hide resolved
@dunglas dunglas merged commit f2d7e21 into dunglas:main Oct 8, 2024
29 of 30 checks passed
@dunglas
Copy link
Owner

dunglas commented Oct 8, 2024

Thank you @kevincobain2000! I refactored a bit the script.

@kevincobain2000
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants