-
Notifications
You must be signed in to change notification settings - Fork 49
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
Add scripts/install/auto-install.sh #824
Conversation
The intent is for the user to execute this command AFTER creating the panoptes user and logging in as the panoptes user: bash <(curl -s https://raw.githubusercontent.com/panoptes/POCS/develop/scripts/install/auto-install.sh) We can later augment this to create the panoptes user and then do the rest of the setup as that user.
Provide two different suggestions for how to download and execute. Allow for overriding of the github repo url and branch, as this will make it easier to test this script.
PAWS is expected by install-dependencies.sh, so installing it will ease the life of the builder.
Codecov Report
@@ Coverage Diff @@
## develop #824 +/- ##
===========================================
+ Coverage 81.24% 81.27% +0.02%
===========================================
Files 66 68 +2
Lines 5513 5516 +3
Branches 758 759 +1
===========================================
+ Hits 4479 4483 +4
- Misses 836 837 +1
+ Partials 198 196 -2
Continue to review full report at Codecov.
|
scripts/install/auto-install.sh
Outdated
|
||
[[ -z "${POCS_GITHUB_USER}" ]] && export POCS_GITHUB_USER="panoptes" | ||
[[ -z "${POCS_GIT_URL}" ]] && export POCS_GIT_URL="https://github.com/${POCS_GITHUB_USER}/POCS.git" | ||
[[ -z "${POCS_BRANCH}" ]] && export POCS_BRANCH="develop" |
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.
I think this is fine, but I have been thinking lately that we should go back to pushing release versions to master
and having non-developer units always pull from master
. Mostly the onus here would be on me to actually be good about making releases.
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.
I think that might be a good idea to use master that way. I.e. on rare occasions, and after testing on multiple units, we declare a new version that non-developers should use.
scripts/install/auto-install.sh
Outdated
echo " | ||
Ensuring that ${PANDIR} is owned by user ${PANUSER} | ||
" | ||
do_sudo chown "${PANUSER}" "${PANDIR}" |
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.
do_sudo chown "${PANUSER}" "${PANDIR}" | |
do_sudo chown -R "${PANUSER}:${PANUSER}" "${PANDIR}" |
I added the recursive option because I was thinking the subdirectories existed at this point but it looks like not but the option shouldn't hurt. Also not sure if wanted the group changed or not, I usually default to changing both.
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.
I tried that a while ago, even had it in this code earlier today, but then I remembered that it was really very slow if the full tree was already there. I can reconsider/retest if you like.
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.
Interesting. It seems like there shouldn't be that many files in the tree. But okay not doing it. That means are assuming if it already exists then the user has already set their permissions accordingly, which I think is fine.
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.
I added it, and it worked fine. I now recall that the slow case was inside of docker.
scripts/install/auto-install.sh
Outdated
do_sudo chown "${PANUSER}" "${PANDIR}" | ||
|
||
clone_or_update "${POCS}" "${POCS_GIT_URL}" "${POCS_BRANCH}" | ||
clone_or_update "${PAWS}" "https://github.com/panoptes/PAWS.git" "develop" |
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.
Could obviously add the ability to clone a forked version of PAWS but I suppose not entirely necessary at this point. Can leave until we overhaul PAWS, which needs to happen soonish.
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.
Exactly.
@@ -574,7 +574,10 @@ function maybe_install_conda() { | |||
function get_installed_astrometry_version() { | |||
local -r solve_field="${ASTROMETRY_DIR}/bin/solve-field" | |||
if [[ -x "${solve_field}" ]] ; then | |||
"${solve_field}" --help|(grep -E '^Revision [0-9.]+,' || /bin/true)|cut -c10-|cut -d, -f1 | |||
("${solve_field}" --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.
I use $SOLVE_FIELD
elsewhere to point to the solve-field
executable. Here it is just checking a revision number, correct? (Also, I keep meaning to submit a PR to astrometry.net to add a simple --version
flag. Very annoying it doesn't have one.)
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.
Yup, just checking version number.
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 looks nice and simple, which is good. It looks like this assumes everything is set up w.r.t. the user, in particular membership in the dialout
and/or plugdev
groups, etc.
@jamessynge what's the plan with this? Should I merge? |
Set prompt correctly (with panoptes-env) in interactive shells.
Move various helper functions from install-dependencies.sh into install-helper-functions.sh. Remove unused functions from install-dependencies.sh.
PTAL. I've tested/adjusted this repeatedly, trying to streamline the process, and improve the experience for folks that aren't Linux experts. For example, it now invokes the set-panoptes-env.sh script twice, once at the start of .bashrc, and again at the end. This ensures that non-interactive shells are configured with the desired environment (e.g. for crontab), and also updates the prompt for interactive shells. |
and for $HOME/.cache. The latter because I found that if it doesn't exist, some install step creates it as owned by root:root, breaking later install steps.
setup.py and pytest at the end of auto-install.sh. Could also consider doing so in install-dependencies.sh.
during install-dependencies.sh (defaults to doing so).
Don't check if stdin is a terminal when attempting to get column width. I think this messes with some of the methods which can probe stdout.
Remove unintended indentation in message.
Don't require user to hit enter during gcloud install.
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.
👍
@@ -22,4 +22,8 @@ scikit_image >= 0.12.3 | |||
scipy >= 0.17.1 | |||
transitions >= 0.4.0 | |||
tweepy | |||
|
|||
# requests 2.21 doesn't like urllib3 >=1.25 | |||
urllib3 >= 1.24, < 1.25 |
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.
I wish we could somehow put a timer on this. urllib3 1.25.x is only about two weeks old, so I expect this will resolve itself shortly.
Of course, there are not really any features in 1.25 that we require so should be okay.
@@ -78,3 +78,6 @@ coreutils | |||
# includes this file). | |||
git | |||
|
|||
# Installing some POCS dependencies (esp. astroscrappy) require a C compiler, |
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.
I think astroscrappy
is required by ccdproc
, which is still in develop
but which I think I have removed from, i.e. #279. We don't really use it for POCS (we might for PIAA).
Not terrible to have build-essential
here, but if we use this script to build Docker images later we will want to minimize this as it causes bloat.
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.
Acknowledged.
################################################################################ | ||
# Clone the POCS repo from github. | ||
|
||
clone_or_update "${POCS}" https://github.com/panoptes/POCS.git upstream develop |
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.
Do you want the upstream
here? Seems like it is not set anywhere above if I'm following correctly. You do set it right below this.
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.
Yes. This is used as the origin name when cloning, so that https://github.com/panoptes/POCS.git
will be the upstream repo, and https://github.com/${POCS_GITHUB_USER}/POCS.git
will be the origin.
fi | ||
|
||
clone_or_update \ | ||
"${PAWS}" "https://github.com/panoptes/PAWS.git" upstream develop |
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.
Same comment about upstream
.
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.
Yup, that is intended.
scripts/install/auto-install.sh
Outdated
# QUESTION: Should we add these lines here: | ||
# source $PANDIR/set-panoptes-env.sh | ||
# cd $POCS | ||
# python setup.py install |
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 should be set to develop
instead of install
.
scripts/install/auto-install.sh
Outdated
${POCS}/scripts/install/install-dependencies.sh | ||
|
||
# QUESTION: Should we add these lines here: | ||
# source $PANDIR/set-panoptes-env.sh |
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.
What is the alternative? Set them manually?
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.
Yes. The install-dependencies.sh script spits out some lines for the user to run by hand, and I'm suggesting that I could do so automatically as part of validating the install.
cd "${REPO_DIR}" | ||
git fetch --all | ||
git checkout "${BRANCH}" | ||
git pull |
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.
If they have already set up the repo they might have origin
set to their repo and this pull might not necessarily be doing what we want. Or they might have files in staging, in which case this will fail.
I think if the dir is there and a repo we just do nothing other than maybe print a message reminding them to update. Thoughts?
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.
Yeah, clearly if there are already files staged then we need to avoid messing with the repo.
Of course, if they have a repo, they don't need this auto-install script, and can run install-dependencies.sh themselves.
I'll adjust the script to stop if there is a POCS or PAWS repo.
@jamessynge I wonder if we also shouldn't copy |
I've added some code for copying pocs.yaml to pocs_local.yaml, and reminding the user to customize it. |
a message indicating how to proceed. Run setup.py install, and pytest after installing dependencies. Clone pocs.yaml, and remind user to modify pocs_local.yaml (the clone).
And fix it so that it actually sets the PATH! Fix install-dependencies.sh so that it is more likely to remove lines it added to .bashrc previously, even if slightly different.
Add script which can be used to install POCS and dependencies via a single command:
Description
The script does the following:
/var/panoptes
with the appropriate ownership.scripts/install/install-dependencies.sh
Related Issue
#531
How Has This Been Tested?
Tested by repeatedly setting up PAN005 for Doug and Christina.
Screenshots (if appropriate):
Types of changes
Checklist: