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

Workflow; support, but don't require, pyenv #205

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

Workflow; support, but don't require, pyenv #205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d9fe0ed
Workflow; support, but don't require, pyenv
Oct 27, 2020
53814b4
curious
Oct 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .python-version
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
3.7.5
2 changes: 2 additions & 0 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@ language: python
# cache: pip # not enough installed via pip to justify
python:
- '3.6'
- '3.7'
os:
- linux
- osx
before_install:
- pip install -U pip
install:
Expand Down
14 changes: 7 additions & 7 deletions dev/setup_env_osx.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
#!/bin/bash
set -eo pipefail
set -o errexit -o nounset -o pipefail

readonly repo_root="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd -P)"
Copy link

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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. 🤯

Copy link

@DoomGerbil DoomGerbil Oct 27, 2020 •
edited
Loading

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.

Copy link
Contributor

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.

Suggested change
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")"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.

exit 1
}

readonly root="${BASH_SOURCE%/*}/.."

# Setup python virtualenv for running tests
readonly venv_dir="${root}/.dev-venv"
readonly venv_dir="${repo_root}/.dev-venv"

python3 -m pip install virtualenv
python3 -m virtualenv "${venv_dir}"
Expand All @@ -21,8 +21,8 @@ else
venv_bin="${venv_dir}/bin"
fi

"${venv_bin}/python" -m pip install -r "${root}/python/requirements.txt"
"${venv_bin}/python" -m pip install -r "${root}/ci/requirements.txt"
"${venv_bin}/python" -m pip install -r "${repo_root}/python/requirements.txt"
"${venv_bin}/python" -m pip install -r "${repo_root}/ci/requirements.txt"

# Install p4d binary if missing
if ! [[ -x "$(command -v p4d)" ]]; then
Expand Down