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.

Sign up for GitHub

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

Add global option process_extra_env for every Process call #21501

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Add global option process_extra_env for every Process call #21501

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8afcc71
Add global option process_extra_env
grihabor Oct 4, 2024
52bdd3b
Format imports
grihabor Oct 5, 2024
fadd90d
Slightly improve perfomance
grihabor Oct 5, 2024
2b936b2
Add notes
grihabor Oct 5, 2024
a6326e1
Fix isort
grihabor Oct 6, 2024
519ee3b
Fix black
grihabor Oct 6, 2024
6402e32
Fix mypy
grihabor Oct 6, 2024
f5c6b81
Use existing env_vars option
grihabor Oct 21, 2024
afb3db9
Fix notes
grihabor Oct 21, 2024
26f69bf
Try use subproc_env: SubprocessEnvironment.EnvironmentAware,
grihabor Oct 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/notes/2.24.x.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ We offer [formal sponsorship tiers for companies](https://www.pantsbuild.org/spo

* Fixed bug where `pants peek --include-additional-info` was not actually displaying the additional info ([#21399](https://github.com/pantsbuild/pants/pull/21399)).

* Fixed the `[subprocess-environment].env_vars` option to set arbitrary env variables for every pants Process call.

### New Options System

This release switches the Pants [options system](https://www.pantsbuild.org/2.22/docs/using-pants/key-concepts/options) to use the new "native" implementation written in Rust first introduced in the 2.22.x series.
Expand Down
14 changes: 13 additions & 1 deletion src/python/pants/engine/intrinsics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@
# Licensed under the Apache License, Version 2.0 (see LICENSE).

from __future__ import annotations
from pants.core.util_rules.subprocess_environment import SubprocessEnvironment, SubprocessEnvironmentVars
from pants.core.util_rules import subprocess_environment

import dataclasses
import itertools
import logging

from pants.engine.environment import EnvironmentName
Expand Down Expand Up @@ -39,7 +43,9 @@
ProcessExecutionEnvironment,
)
from pants.engine.rules import _uncacheable_rule, collect_rules, implicitly, rule
from pants.option.global_options import GlobalOptions
from pants.util.docutil import git_url
from pants.util.frozendict import FrozenDict


@rule
Expand Down Expand Up @@ -102,8 +108,13 @@ async def add_prefix(add_prefix: AddPrefix) -> Digest:

@rule
async def execute_process(
process: Process, process_execution_environment: ProcessExecutionEnvironment
process: Process,
process_execution_environment: ProcessExecutionEnvironment,
env_vars: SubprocessEnvironmentVars,
Copy link
Contributor

Choose a reason for hiding this comment

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

Your rule needs to request a SubprocessEnvironment.EnvironmentAware I believe. That is the trick by which different environments can set different env vars (and more generally is the trick by which different environments can have different values for a subsystem).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't work, because I have to Get(EnvironmentVariables) inside to get the actual values, and it still fails with a missing rule. 26f69bf

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, sorry, yes, you were doing the right thing to begin with, I read SubprocessEnvironmentVars as SubprocessEnvironment.

Copy link
Contributor

Choose a reason for hiding this comment

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

The get_subprocess_environment rule takes care of going from SubprocessEnvironment.EnvironmentAware to actual dict of env vars.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll take a closer look in a bit

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh boy, yes, this is more complicated than I first appreciated.

However, the complication is justified, and adding a new option won't solve it, unless the new option is not EnvironmentAware, which seems simply incorrect.

I will noodle on this a bit more.

) -> FallibleProcessResult:
if env_vars.vars:
items = itertools.chain(process.env.items(), env_vars.vars.items())
process = dataclasses.replace(process, env=FrozenDict(items))
return await native_engine.execute_process(process, process_execution_environment)


Expand Down Expand Up @@ -207,4 +218,5 @@ async def path_metadata_request(request: PathMetadataRequest) -> PathMetadataRes
def rules():
return [
*collect_rules(),
*subprocess_environment.rules(),
]
Loading