-
-
Notifications
You must be signed in to change notification settings - Fork 0
Add some troubleshooting notes for the development environment #170
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: main
Are you sure you want to change the base?
Conversation
untitaker
left a comment
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 recommend against documenting N workarounds for things that should already just work. if we document it, people will work around the broken automation instead of raising those issues and fixing them.
| a. MacOs note: There are multiple ways to install the python environment on MacOS. | ||
| We tested successfully the installation via `brew`. Other ways, like `pyenv` and | ||
| `uv`, should work as well but we saw incompatibilities with the way `maturin` expects | ||
| environment variables to be set. See below for more details if you want to use | ||
| `pyenv` or `uv` to install python. |
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 should be handled by the envrc and direnv allow. if 3.11 does not exist uv will download it. if that leads to problems let's treat it like any other bug
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.
if that leads to problems let's treat it like any other bug
We are treating it as any other bug. https://linear.app/getsentry/issue/STREAM-284/automate-all-the-validation-step-in-the-dev-env-guide . Still till that is not fixed people should at least know a way to make it work otherwise nobody would try to fix the issue.
| environment variables to be set. See below for more details if you want to use | ||
| `pyenv` or `uv` to install python. | ||
|
|
||
| 4. Run `make install-dev` from the root of the repo. This will instal `uv` and build |
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 done by direnv allow
| Some environment variables are needed to successfully build the library. These are set by direnv. | ||
| They are explained here to give a better understanding in case of issues. | ||
|
|
||
| - `CMAKE_POLICY_VERSION_MINIMUM=3.5`` - This is needed to build librdkafka on | ||
| newer versions of cmake | ||
|
|
||
| Needed environment variables to allow Rust to start a Python interpreter properly: | ||
|
|
||
| - `PYTHONPATH=.` - This is needed in order to make the Python interpreter started by | ||
| Rust find the virtual environment. | ||
|
|
||
| - `PYO3_PYTHON` - This is needed to make the Python interpreter find the right `site-packages` | ||
| directory when started by the Rust code. See `rust-envvars <https://github.com/getsentry/streams/blob/main/scripts/rust-envvars>`_ |
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.
all of this is set by the envrc
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.
Yes, this paragraph is not part of the happy path. You should only land here if something went wrong. If something goes wrong knowing what variables are needed for what is needed to allow people to try to fix the issue.
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.
if this just "neutrally" documents what the envrc does, shouldn't it be possible to document it next to the code that sets those envvars, i.e. as code comments?
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.
That makes sense to me. I'd move the explanation in the code and reference that from here.
We have a problem to solve. Multiple people needed hours of pairing to get their environment working. The absence of documentation explaining how things worked did not prompt people to fix them but to skip tests. Obviously this is not acceptable. The first (incentive to fix) is both tracked https://linear.app/getsentry/issue/STREAM-284/automate-all-the-validation-step-in-the-dev-env-guide. I doubt we can expect people to fix the environment if the way it works is not clear. |
and now it does, right? i mean most of the envvars you are suggesting in this guide are already set by envrc. i don't think we should suggest to set them explicitly. if they are missing, it's an indication that their direnv setup is broken, and this is the part that should be fixed.
my concern is that we end up with dev environments that drift further away from the golden path. because everybody applied manual fixes on top. |
untitaker
left a comment
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.
don't want to block this too much. maybe this though: instead of "set this envvar" i'd like to rather suggest "figure out why this envvar isn't being set"
|
|
||
| Needed environment variables to allow Rust to start a Python interpreter properly: | ||
|
|
||
| - `PYTHONPATH=.` - This is needed in order to make the Python interpreter started by |
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 envvar is already set (STREAMS_TEST_PYTHONPATH)
specifically overwriting this envvar like this will fix one bug while re-introducing another. i think if PYTHONPATH needs to be set for cargo test, this points to an issue with direnv. otherwise it's a new bug to be investigated and should be discussed.
also since we now overwrite sys.path within the rust process i'm no longer sure PYTHONPATH has any effect at all
| Some environment variables are needed to successfully build the library. These are set by direnv. | ||
| They are explained here to give a better understanding in case of issues. | ||
|
|
||
| - `CMAKE_POLICY_VERSION_MINIMUM=3.5`` - This is needed to build librdkafka on |
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 also definitely set by envrc.
Suggesting to set environment variables manually was not my intent. The happy path is only this https://github.com/getsentry/streams/pull/170/files#diff-a5fb029817e5f0c5efdcb0de0545265c5c80aefe309baab2141c676f9c26849eR32-R56. The rest is all about how to understand the process when things go wrong. I'd be happy to clarify better what is the happy path vs what is troubleshooting. What if I explained that explicitly at the beginning of this section https://github.com/getsentry/streams/pull/170/files#diff-a5fb029817e5f0c5efdcb0de0545265c5c80aefe309baab2141c676f9c26849eR32 |
| Some environment variables are needed to successfully build the library. These are set by direnv. | ||
| They are explained here to give a better understanding in case of issues. | ||
|
|
||
| - `CMAKE_POLICY_VERSION_MINIMUM=3.5`` - This is needed to build librdkafka on | ||
| newer versions of cmake | ||
|
|
||
| Needed environment variables to allow Rust to start a Python interpreter properly: | ||
|
|
||
| - `PYTHONPATH=.` - This is needed in order to make the Python interpreter started by | ||
| Rust find the virtual environment. | ||
|
|
||
| - `PYO3_PYTHON` - This is needed to make the Python interpreter find the right `site-packages` | ||
| directory when started by the Rust code. See `rust-envvars <https://github.com/getsentry/streams/blob/main/scripts/rust-envvars>`_ |
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.
if this just "neutrally" documents what the envrc does, shouldn't it be possible to document it next to the code that sets those envvars, i.e. as code comments?
evanh
left a comment
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 think this is great!
| environment variables to be set. See below for more details if you want to use | ||
| `pyenv` or `uv` to install python. | ||
|
|
||
| 4. Run `make install-dev` from the root of the repo. This will instal `uv` and build |
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.
| 4. Run `make install-dev` from the root of the repo. This will instal `uv` and build | |
| 4. Run `make install-dev` from the root of the repo. This will install `uv` and build |
We had a number of issues in managing the development environment.
This collects all the learnings