-
Notifications
You must be signed in to change notification settings - Fork 103
feat(dev): devenv + uv + pre-commit #5070
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
base: master
Are you sure you want to change the base?
Conversation
# TODO: reactivate when fixed | ||
# - id: check-github-workflows | ||
|
||
- repo: local |
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.
pre-commit now runs in ci and now runs black/flake8/mypy (locked+installed via uv) on python files!
|
||
# Style checking | ||
|
||
style: style-rust style-python ## check code style |
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.
style and lint for python's moved into pre-commit
i decided to leave rust alone for now to keep things simple (will need some work as tools like rustfmt and clippy-driver are needed to accept individual file arguments)
$$RELAY_PYTHON_VERSION -m venv --copies .venv | ||
.venv/bin/pip install -U pip wheel | ||
|
||
.venv/python-requirements-stamp: requirements-dev.txt |
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.
rerunning make setup-venv
will always make sure python stuff is up to date and reexecutes the build - all this complexity can be removed
# Make will re-run 'pip install' if the mtime on requirements-dev.txt is higher again. | ||
touch .venv/python-requirements-stamp | ||
|
||
.git/hooks/pre-commit: |
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 has been rolled up into pre-commit install --install-hooks
in devenv sync
#!/bin/bash | ||
#!/not/executable/bash | ||
|
||
# Our policy is that the .envrc is entirely optional, and a user |
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.
devenv/sync.py
and .pre-commit-config.yaml
are all using .venv/bin
explicitly so this policy you have here is still being respected! although you will need a global install of devenv: https://github.com/getsentry/devenv/?tab=readme-ov-file#install
(chances are and i would hope relay devs already have devenv installed as it is required for sentry development)
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.
actually well it is needed to put .devenv/bin on path so you have access to devenv's uv at .devenv/bin/uv
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.
LGTM but I would appreciate a second opinion
hooks: | ||
- id: check-github-actions | ||
- id: check-github-workflows | ||
# TODO: reactivate when fixed |
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 this referring to? What needs to be fixed?
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.
https://gist.github.com/joshuarli/36587aeb18445bf18f36abcd4b82382e
(upgraded to 0.33.0 since in previous versions output is a lot less readable)
mostly complaining about 'uses' is a required property
which... it is not? lol
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.
Were you able to test this workflow? Any chance we will face surprises next time we want to release the library?
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 good catch, I'll run it and see!
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 it's totally broken haha, will fix
8625a9c
to
38369f3
Compare
) | ||
|
||
if not shutil.which("rustup"): | ||
raise SystemExit("rustup not on PATH. Did you run `direnv allow`?") |
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.
Bug: Rustup Check Fails During Fresh Setup
The rustup
check runs too early in the sync
script. Although brew bundle
installs rustup
earlier, it's not immediately available on PATH until the environment (like direnv
) reloads. This causes the script to exit prematurely on a fresh setup with a misleading error.
This introduces devenv, uv and pre-commit in accordance with these repo standards:
https://www.notion.so/sentry/Standard-Spec-devenv-2238b10e4b5d8072a992d8b67d94be22
https://www.notion.so/sentry/Standard-Spec-python-uv-2248b10e4b5d8045b8fff30f8b8b67ca
https://www.notion.so/sentry/Standard-Spec-pre-commit-2238b10e4b5d8019aa1fe1158e39c38e
The ability to run devenv sync and get a dev environment in any repo is the DX i'm aiming for,
and these standards provide a foundation from which later we can easily push updates to all repos.
You should be able to checkout this branch, run direnv allow and devenv sync (might be prompted to run ~/.local/share/sentry-devenv/bin/devenv update first), which will set up everything needed for development (other than cargo).
Thanks to uv and our internal pypi, setting up the venv is near instant now (excluding the rust build of
py/
of course).Originally I was going to split this into 2 PRs, where the latter would configure py/ for uv, but it's not very good DX since uv sync will erase it as it doesn't know about it.
Note
Introduce devenv + uv for Python envs and pre-commit hooks, update CI to use uv/venv with targeted linting, and replace legacy venv/requirements workflow across repo.
devenv/
(config andsync.py
),Brewfile
, rootpyproject.toml
,py/pyproject.toml
,uv.lock
, and.pre-commit-config.yaml
to standardize setup with uv and pre-commit..envrc
to load.env
, add uv/devenv toPATH
, and requiredevenv
; export.venv
.setup.cfg
forflake8
; removerequirements-dev.txt
andscripts/git-precommit-hook
.actions/setup-python
withastral-sh/setup-uv
+getsentry/action-setup-venv
; install deps viauv sync
.ci.yml
, add changed-file detection and runpre-commit
on modified files; cache pre-commit; adjust Python build/test to use uv; update devservices validation to read version fromuv.lock
.build_library.yml
, trigger on PR, use uv-based venv for macOS and sdist builds; comment out artifact merge step.Makefile
: drop Python style/lint/format targets and legacy venv management;setup-venv
now runsdevenv sync
and installspy/
via uv.README.md
to usedevenv sync
anduv pip
commands.Written by Cursor Bugbot for commit ab5c3b7. This will update automatically on new commits. Configure here.