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

Modifying the install script to default to /opt/pret/ #63

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

furrysalamander
Copy link

This will allow for the decompilations to default to a system wide installation.

@mid-kid
Copy link
Member

mid-kid commented Mar 13, 2023

Consider checking whether a makefile is present and install into tools only then. Or have a flag to disable the implicit tools/ addition.

@mid-kid
Copy link
Member

mid-kid commented Mar 13, 2023

for reference, a previous attempt at tackling the install script lives here: #45

@luckytyphlosion
Copy link
Member

Make a separate install.sh. Any sort of backwards incompatible script has to be separate.

@furrysalamander
Copy link
Author

furrysalamander commented Mar 14, 2023

@luckytyphlosion this is backwards compatible. The only change in behavior is that now running the script without args installs to a default location instead of printing an error message.

Comment on lines +23 to +34
mkdir -p $INSTALL_DIRECTORY/agbcc
mkdir -p $INSTALL_DIRECTORY/agbcc/bin
mkdir -p $INSTALL_DIRECTORY/agbcc/include
mkdir -p $INSTALL_DIRECTORY/agbcc/lib
cp agbcc $INSTALL_DIRECTORY/agbcc/bin/
cp old_agbcc $INSTALL_DIRECTORY/agbcc/bin/
cp agbcc_arm $INSTALL_DIRECTORY/agbcc/bin/
cp -R libc/include $INSTALL_DIRECTORY/agbcc/ #drop include, because we don't want include/include
cp ginclude/* $INSTALL_DIRECTORY/agbcc/include/
cp libgcc.a $INSTALL_DIRECTORY/agbcc/lib/
cp libc.a $INSTALL_DIRECTORY/agbcc/lib/
echo "agbcc successfully installed!"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
mkdir -p $INSTALL_DIRECTORY/agbcc
mkdir -p $INSTALL_DIRECTORY/agbcc/bin
mkdir -p $INSTALL_DIRECTORY/agbcc/include
mkdir -p $INSTALL_DIRECTORY/agbcc/lib
cp agbcc $INSTALL_DIRECTORY/agbcc/bin/
cp old_agbcc $INSTALL_DIRECTORY/agbcc/bin/
cp agbcc_arm $INSTALL_DIRECTORY/agbcc/bin/
cp -R libc/include $INSTALL_DIRECTORY/agbcc/ #drop include, because we don't want include/include
cp ginclude/* $INSTALL_DIRECTORY/agbcc/include/
cp libgcc.a $INSTALL_DIRECTORY/agbcc/lib/
cp libc.a $INSTALL_DIRECTORY/agbcc/lib/
echo "agbcc successfully installed!"
install -v -Dm755 -t "$INSTALL_DIRECTORY/bin" agbcc old_agbcc agbcc_arm
install -v -Dm644 -t "$INSTALL_DIRECTORY/lib" libgcc.a libc.a
install -v -Dm644 -t "$INSTALL_DIRECTORY/include" ginclude/*
cp -vR libc/include "$INSTALL_DIRECTORY/" #drop include, because we don't want include/include
echo "agbcc successfully installed!"

More verbosity, and explicit file permissions

cp libc.a $1/tools/agbcc/lib/
echo "agbcc successfully installed!"

if [ "$1" = "-h" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if [ "$1" = "-h" ]; then
if [ "$1" = -h -o "$1" = --help ]; then

When parsing arguments as flags, it's appropriate to have a -- flag to end the list of argument flags, after which the rest is interpreted as positional arguments, and fail when an unknown flag is passed. That said, this is only for -h and I doubt anyone would install to "--help", but keep it in mind and consider using getopt in the future.


# The BASE_INSTALL_DIRECTORY nomenclature is so that the check for the existence of the
# directory doesn't fail just because there's no tools directory yet.
if [ -d "$BASE_INSTALL_DIRECTORY" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

This will fail when /opt/pret doesn't exist, forcing people to mkdir /opt/pret before running the script.
This is a bit of a significant caveat, and I'd rather the script just create necessary directories itself.

Copy link
Member

@mid-kid mid-kid Mar 14, 2023

Choose a reason for hiding this comment

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

If it's imperative to check that a target directory exists, add a -p flag or similar to enable implicit directory creation.
Personally, I think it's good form for an installer to create directories automatically, and at worst provide a helpful message when the user doesn't have permissions to create a directory, and a useful message telling the user the location where the files were installed.

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.

3 participants