-
Notifications
You must be signed in to change notification settings - Fork 14
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
Workflow; support, but don't require, pyenv #205
base: master
Are you sure you want to change the base?
Conversation
Travis automatically has pyenv + pyenv-virtualenv support, so I expect/hope this to be lovely.
set -eo pipefail | ||
set -o errexit -o nounset -o pipefail | ||
|
||
readonly repo_root="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd -P)" |
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.
Am I misreading it, or does this set $repo_root
to dev/
within the repo? That seems misleading at best.
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.
Not sure why the existing line below readonly root="${BASH_SOURCE%/*}/.."
was deleted - this seems more readable.
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 need the variable within the error message on line 7.
re: readability - settle in... https://stackoverflow.com/questions/59895/how-to-get-the-source-directory-of-a-bash-script-from-within-the-script-itself is a joy. 🤯
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.
@petemounce And I notice that in that case, it's referencing .python-version
at the root of the repo as $repo_root/../.python-version
.
That's my problem here - that repo_root
isn't actually the repo root. It's basically repo_root=ACTUAL_REPO_ROOT/dev
which IMO is definitely not correct.
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.
+1 on @DoomGerbil's point here. This line is not setting the right directory. Am suggestion the cleaned-up version.
readonly repo_root="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd -P)" | |
readonly repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd -P)" |
The pipe to /dev/null
is not required as the cd
should not print anything if successful and we bail-out anyway if there's an error (due to set -e
above) and we would like to see the output in that case.
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'm unsure about the value we're setting for the repo_root
here. Assuming I didn't misread the one-liner, I would prefer that repo_root
actually reference the repo root, and then the rest of the paths using it be relative to that, rather than setting that var to /dev
within the repo and then acting relative to that.
Otherwise LGTM
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: DoomGerbil, PaulSonOfLars The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
FYI @petemounce we have no OWNERS in this repo. We probably wanna address that at some point, though not super urgent. |
set -eo pipefail | ||
set -o errexit -o nounset -o pipefail | ||
|
||
readonly repo_root="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd -P)" |
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.
+1 on @DoomGerbil's point here. This line is not setting the right directory. Am suggestion the cleaned-up version.
readonly repo_root="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd -P)" | |
readonly repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd -P)" |
The pipe to /dev/null
is not required as the cd
should not print anything if successful and we bail-out anyway if there's an error (due to set -e
above) and we would like to see the output in that case.
|
||
command -v "python3" >/dev/null 2>&1 || { | ||
echo >&2 "I require python3 but it's not installed. Officially supported version is 3.7.5" | ||
echo >&2 "I require python3 but it's not installed. Officially supported version is $(cat "${repo_root}/../.python-version")" |
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.
echo >&2 "I require python3 but it's not installed. Officially supported version is $(cat "${repo_root}/../.python-version")" | |
echo >&2 "I require python3 but it's not installed. Officially supported version is $(cat "${repo_root}/.python-version")" |
Fixing this in relationship to the corrected repo_root
above in my suggestion, and to make it consistent with the other uses of ${repo_root}
below in this script.
Travis automatically has pyenv + pyenv-virtualenv support, so I expect/hope this to be lovely.No, it doesn't. Nuts. Still, this enables some workstation pinning.pyenv-virtualenv will, when the shell has had
eval $(pyenv virtualenv-init -)
done, automatically enter a virtualenv if a.python-version
file is found when cd'ing into a directory.https://github.com/pyenv/pyenv-virtualenv
Changes
Verification