Skip to content

Conversation

@fpacifici
Copy link
Collaborator

We had a number of issues in managing the development environment.
This collects all the learnings

@fpacifici fpacifici requested a review from a team as a code owner June 27, 2025 23:57
Copy link
Member

@untitaker untitaker left a 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.

Comment on lines +43 to +47
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.
Copy link
Member

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

Copy link
Collaborator Author

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
Copy link
Member

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

Comment on lines +77 to +89
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>`_
Copy link
Member

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

Copy link
Collaborator Author

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.

Copy link
Member

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?

Copy link
Collaborator Author

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.

@fpacifici
Copy link
Collaborator Author

i recommend against documenting N workarounds for things that should already just work

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.
Would you agree that we need both the incentive to fix the environment and enough shared knowledge to understand what is not working so people know where to start from to fix the environment?

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.

@untitaker
Copy link
Member

untitaker commented Jun 30, 2025

We have a problem to solve. Multiple people needed hours of pairing to get their environment working.

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.

The absence of documentation explaining how things worked did not prompt people to fix them but to skip tests. Obviously this is not acceptable.

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.

Copy link
Member

@untitaker untitaker left a 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
Copy link
Member

@untitaker untitaker Jun 30, 2025

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
Copy link
Member

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.

@fpacifici
Copy link
Collaborator Author

fpacifici commented Jun 30, 2025

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.

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.
Fair: I need to remove the reference to make install-dev. Will do that.

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

Comment on lines +77 to +89
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>`_
Copy link
Member

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?

Copy link
Member

@evanh evanh left a 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
Copy link
Member

Choose a reason for hiding this comment

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants