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.

Sign up for GitHub

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

Jump to bottom

Updated setup for newhome #28

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Updated setup for newhome #28

wants to merge 2 commits into from

Conversation

Helios113
Copy link

Description

Better accommodate users who move home dir before installing libraries.

Iacob-Alexandru-Andrei
Iacob-Alexandru-Andrei requested changes Jan 25, 2024
View reviewed changes
Copy link
Contributor

@Iacob-Alexandru-Andrei Iacob-Alexandru-Andrei left a comment

Choose a reason for hiding this comment

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

Good PR, remove the defaults for the pyenv and poetry paths before merge.

setup.sh Outdated
VPOETRY_HOME=""
VPYENV_ROOT=""
USER_NAME=$(echo $USER)
VPOETRY_HOME="/nfs-share/$USER_NAME/.poetry"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not reasonable as a default given the public nature of the template.

Copy link
Contributor

Choose a reason for hiding this comment

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

Add back the user-imputed version.

setup.sh
PYENV_BIN=$PYENV_ROOT/bin/pyenv

eval "$($PYENV_BIN init -)"
echo "export PYENV_ROOT=\"$VPYENV_ROOT\"" >> /home/$USER_NAME/.bashrc
Copy link
Contributor

Choose a reason for hiding this comment

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

Reasonable change, if anyone ever complains, we can change it back to $HOME.

@relogu
Copy link
Collaborator

relogu commented Jan 28, 2024

I'm strongly against hardcoding paths, especially if they are necessary for accessing crucial files such as .bashrc. Also, some users don't use bash but other shells.
I propose to take advantage of the discovered problem/incompatibility to generalise this script a bit more.
It's time for this setup script to receive parameters as input and be smarter. We can choose reasonable defaults and print what's happening to the standard output step by step. We can also take inspiration from other setup scripts of tools like Pyenv, Poetry, Conda, etc. Careless users will be fine. Aware users will be happy to read or to contribute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Iacob-Alexandru-Andrei Iacob-Alexandru-Andrei Iacob-Alexandru-Andrei requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Helios113 @relogu @Iacob-Alexandru-Andrei