Skip to content
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

Docs: add clarity around environment variables #18382

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

luke321321
Copy link

Add clarity to the docs to say that hermetic execution implies enviroment variables won't be passed in. Also add in the example that the HOME directory won't be able to be found and so global user settings will be ignored.

@lukedyer-peak lukedyer-peak force-pushed the docs/environment-variables branch from b7e4e54 to b78f105 Compare February 28, 2023 16:51
Copy link
Contributor

@jsirois jsirois left a comment

Choose a reason for hiding this comment

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

Thanks for giving this a go. I'll let others sign off on the PR because my judgement on enough / not enough, etc is always off.

@@ -68,6 +68,7 @@ Work is broken down into many small units and kept warm in a daemon so that as l
### Hermetic execution

Pants sandboxes all processes that it executes, ensuring that cache keys are always accurate, and builds are always correct.
This implies, amongst other things, processes can't access your local [environment variables](docs:reference-subprocess-environment) or even setting stored in your home directory (as this uses `HOME`).
Copy link
Contributor

@jsirois jsirois Feb 28, 2023

Choose a reason for hiding this comment

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

The subprocess-environment references are only good for the Python backend today; so they risk sounding authoritative and yet being off-point. I think the best you can do is to encourage looking for where / how to pass environment variables for the context you're in. Maybe give the subprocess-environment as a qualified example of how you can do this for Python. N.B. that its not even always the right thing to do for Python! If the env var is only needed for tests, you can reduce scope with https://www.pantsbuild.org/docs/reference-test#extra_env_vars - and that works for any language stack, including Python.

Which is all just to say the concept is clear and easy to explain, the details are all over the place and very hard to explain without misleading folks / helping X% & confusing the heck out of Y%.

Copy link
Contributor

Choose a reason for hiding this comment

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

To whit on narrowness of subprocess-environment applicability:

$ git grep SubprocessEnvironment
src/python/pants/backend/python/util_rules/pex_environment.py:from pants.core.util_rules.subprocess_environment import SubprocessEnvironmentVars
src/python/pants/backend/python/util_rules/pex_environment.py:    subprocess_env_vars: SubprocessEnvironmentVars,
src/python/pants/core/util_rules/subprocess_environment.py:class SubprocessEnvironment(Subsystem):
src/python/pants/core/util_rules/subprocess_environment.py:class SubprocessEnvironmentVars:
src/python/pants/core/util_rules/subprocess_environment.py:    subproc_env: SubprocessEnvironment.EnvironmentAware,
src/python/pants/core/util_rules/subprocess_environment.py:) -> SubprocessEnvironmentVars:
src/python/pants/core/util_rules/subprocess_environment.py:    return SubprocessEnvironmentVars(

So no hits in java/scala/jvm/go, etc.

Copy link
Member

Choose a reason for hiding this comment

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

Also worth noting is that for the tools Pants integrates with, config files from your HOME do get picked up where applicable without any additional configuration needed.

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

Successfully merging this pull request may close these issues.

4 participants