-
-
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
feat(dev_env): Dev env bootstrap support for two Python versions #28213
Conversation
This change adds supports for bootstrapping two different Python versions. Python 3.8.10 will be used for Apple M1 since we're targetting Python 3.8 for Sentry's next Python version and it is the first one that supports Apple's M1 chipset.
014d44e
to
859f94e
Compare
cd "$(dirname "${BASH_SOURCE[0]}")" | ||
pwd -P | ||
)" | ||
|
||
bold="$(tput bold)" |
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.
Centralizing this code for other scripts to use.
@@ -58,10 +53,6 @@ report_to_sentry() { | |||
rm "$_SENTRY_LOG_FILE" | |||
} | |||
|
|||
require() { |
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.
Moving it to the main library.
@@ -86,6 +77,27 @@ die() { | |||
return 1 | |||
} | |||
|
|||
prompt_python_venv_creation() { |
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.
Moving this code into a function so we can call it from within the .envrc
context in getsentry
(since we source it).
# We depend on a Sentry checkout for this to work.
source ../sentry/.envrc
.envrc
Outdated
esac | ||
} | ||
|
||
commands_info() { |
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 idea as the previous function.
@@ -44,15 +44,16 @@ _append_to_startup_script() { | |||
case "$SHELL" in | |||
*/bash) | |||
# shellcheck disable=SC2016 | |||
echo "Visit https://github.com/pyenv/pyenv#installation on how to fully set up your Bash shell.";; | |||
echo "Visit https://github.com/pyenv/pyenv#installation on how to fully set up your Bash 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.
Auto-formatting.
# We can remove this once we upgrade to newer versions of Python | ||
if query_big_sur; then | ||
if query-apple-m1; then | ||
pyenv install --skip-existing "${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.
This is the core that makes installations on Apple M1 to use Python 3.8.10.
# NOTE: We're dropping support for older pyenv versions | ||
if [[ "$pyenv_version" < 2.0.0 ]]; then | ||
if [[ "$(pyenv -v | awk '{print $2}')" < 2.0.0 ]]; 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.
Since we only use the variable here, might as well just place it here. shellcheck also complains since I make a reference to PYENV_VERSION
down below, thus, it looks like the two could be the same variable (if ignoring casing).
@@ -111,16 +110,14 @@ install_pyenv() { | |||
setup_pyenv() { | |||
configure-sentry-cli | |||
install_pyenv | |||
_startup_script=$(get_shell_startup_script) | |||
append_to_config "$_startup_script" | |||
append_to_config "$(get_shell_startup_script)" |
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.
Merging the two lines.
|
||
# If the script is called with the "dot space right" approach (. ./scripts/pyenv_setup.sh), | ||
# the effects of this will be persistent outside of this script | ||
echo "Activating pyenv and validating 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.
Making less noise in the output of direnv
.
@billyvg @joshuarli this is ready for another review. Thanks! You can ignore the dev env workflow failure. I will fix that in #28265 |
- '.github/workflows/python-deps.yml' | ||
- 'requirements*' | ||
- '.github/workflows/python-deps.yml' | ||
- 'requirements*' |
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.
Ah we should make this run on precommit
I will follow up on any other comments on my next PR. |
* master: (109 commits) ref(js): Add notice to use useApi when applicable (#28365) fix(tests): Temporarily disable test for migration 0223 (#28363) fix(ui): Use theme border for form footer border (#28359) ref(ts): Authenticators have names (#28362) feat(cdc_search): Implement cdc search for `is:unresolved` query. (#28006) ref(rrweb): Upgrade to latest player (#28333) feat(issue-alerts): Add Sentry Apps Alert Rule Action as an Action (#28338) feat(notifications): Remove NotificationSettings when Slack is uninstalled (#28216) py38(django 2.2): bump django-crispy-forms to 1.8.1 (#28344) types(python): Slack Integration Types (#28236) ref(jira ac): Rm fk constraints from JiraTenant (#28349) chore(js): Upgrade storybook from 6.3.6 -> 6.3.7 (#28323) feat(symbol-sources): Redact symbol source secrets from audit logs (#28334) feat(notifications): Add `actor_id` to Analytics Events (#28347) ref(performance): Separate header from content in transaction events (#28346) Adding in new text styles for the codeowners sync CTA in the issues detail. (#28133) fix(django 2.2): rename view method to avoid clashing (#28312) fix(alerts): Project breadcrumb link to project rule list (#28339) ref(performance): Separate header from content in transaction vitals (#28343) feat(dev_env): Dev env bootstrap support for two Python versions (#28213) ...
In #28213 we removed the leniant Python version check and it hit few engineers. This adds it back and better wording for Python 3.8 version check.
In #28213 we removed the leniant Python version check and it hit few engineers. This adds it back and better wording for Python 3.8 version check.
) This change adds supports for bootstrapping two different Python versions. Python 3.8 will be used for Apple M1 since it's the next Python version for Sentry and it is the first one that supports Apple's M1 chip-set. Other miscellaneous fixes and changes included. Co-authored-by: Billy Vong <billyvg@users.noreply.github.com>
In #28213 we removed the leniant Python version check and it hit few engineers. This adds it back and better wording for Python 3.8 version check.
@@ -1 +0,0 @@ | |||
3.6.13 |
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 believe removing this has broken brand new dev environments, since pyenv will just default to the system python version, we never actually have people set their python to 3.8 so they can do python -m venv .venv
We probably can bring this back now?
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.
Now that they have been reunited we can do that, however, if folks are using direnv
, the bootstrap script they would be using the PYENV_VERSION
env variable which pyenv
recognizes.
I can see how using pyenv
directly w/o direnv
guidance will not have PYENV_VERSION
set.
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.
In that case perhaps our develop docs are out of date and should be suggesting more direnv things
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 going back to the .python-version
file approach; simplifying the setup.
This change adds supports for bootstrapping two different Python versions.
Python 3.8.10 will be used for Apple M1 since we're targetting Python 3.8 for Sentry's
next Python version and it is the first one that supports Apple's M1 chipset.
Fixes: #28154