-
Notifications
You must be signed in to change notification settings - Fork 10
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
Streamlining Setup: Introducing a Makefile for Effortless Dependency Management and Installation #15
base: master
Are you sure you want to change the base?
Conversation
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.
Hello @ashish-kus
First off I apologize for betting back to this an entire year after your PR. I hope you have had a pretty fun year since you last worked on this!
When I was first thinking about how to best provide un/installation for pokeshell (and in general small shell scripts, like it) make
is definitely a solution that crossed my mind. I don't have much experience with writing Makefile
s directly, and I know Makefile
s can get hairy and complex; but this is a simple enough application that having ubiquitous install commands (that can be "easily" implemented) like make install
, make uninstall
can be valuable. Especially a configurable INSTALL_DIR
from the command line.
I eventually decided on just a quick and clean un/install.sh
scripts. Less code for me to deal with.
This is great work, however, and I see you are doing this correctly (i.e. using .PHONY
). A part of me wants to add this in and another part wants to keep it out. Another idea to keep the flexibility here is to treat the Makefile
as a wrapper. In other words, instead of removing install.sh
and uninstall.sh
, have make
call those scripts in its respective install
and uninstall
targets. This will keep make
as a wrapper layer that can be much easier to maintain, instead of having it replace the existing un/install.sh
scripts.
What do you think about such a solution?
By the way, I just remembered that just
is a thing now that I came across a while back. This seems to be a better solution than make
and is right up the ally for this problem, imo.
@ashish-kus update. Added a justfile
, still need to incorporate checking for dependencies, and perphaps adding a line in ~/.profile
, which appends the install_dir to the $PATH
and remove that line in ~/.profile
during uninstall. By the way take a look at basher.
@echo "updating your $PATH variable" | ||
|
||
@echo -e "\033[1;32m DONE \033[0m pokeshell is now installed" | ||
@echo -e "\033[1;32m RELOAD YOUR SHELL BEFORE USING POKESHELL \033[0m" |
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.
This script is not updating the $PATH as far as I can tell, please remove line 14. Since it is not updating the $PATH, reloading will not do anything. pokeshell will work as long as $(INSTALL_DIR)/bin
is on path.
|
||
@echo "Removing files" | ||
sudo rm -rf $(INSTALL_DIR)/bin/pokeshell | ||
@echo "Removing Complitions" |
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.
Spelling
sudo rm -rf $(INSTALL_DIR)/bin/pokeshell | ||
@echo "Removing Complitions" | ||
sudo rm -rf $(INSTALL_DIR)/share/bash-completion/completions/pokeshell |
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.
the -r
flag is doing nothing here as these are files
the -f
flag is not needed anyway either I think, can you explain where the current invocation will fail (just rm
) compared to rm -f
?
Also I'd like to keep the -v
flag for being transparent with users. Shellscripts shouldn't be trusted willy-nilly from the internet and -v
makes what we are removing on their filesystem clear.
shell completions. | ||
|
||
An uninstall script is also provided: | ||
An uninstalltion is also provided: |
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.
spelling
``` | ||
|
||
If you do not want to install then you can still run pokeshell anywhere | ||
by adding the following lines to your `~/.bashrc`. | ||
by just colning the repo and adding the path to repo to your $PATH variable by adding the following lines to your `~/.bashrc`. |
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.
spelling, use backticks around $PATH.
pokeshell --help | ||
|
||
pokeshell -a random s:pikachu-gmax" | ||
|
||
for more options $ pokeshell --help |
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.
remove the example here, that is what --help
is for.
@@ -0,0 +1,44 @@ | |||
# Define the dependencies | |||
DEPENDENCIES = chafa convert jq curl |
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.
convert
should be imagemagick
I believe.
# Installation instructions | ||
install: check_dependencies | ||
@echo "\033[1;32m Installing... \033[0m" | ||
sudo mkdir -pv $(INSTALL_DIR) |
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.
Are sudo
commands necessary inside the Makefile
? I believe these should be left out and if the INSTALL_DIR
requires admin privileges then the user can call sudo make install/uninstall
, right?
Wanted to contribute to this awesome project. I've replaced the install and uninstall scripts with a MAKEFILE to handle dependency checks and installations, ensuring a smoother user experience and for better management. I've made some updates to the README to reflect the changes I've introduced.
Installation:
make install
Uninstallation:
make uninstall
Please review and share your thoughts. Thanks!