extra_env_vars field for system_binary #20374

ubmarco · 2024-01-07T21:04:11Z

Fixes #20373

benjyw

Nice so far! See my one comment about the proposed semantics (the "fingerprinting" is more of a detail than most users will understand or care about).

As for testing - should be easy to test by running the system binary env with some contrived env var and checking that the value shows up in its output!

benjyw · 2024-01-07T21:16:34Z

src/python/pants/backend/adhoc/target_types.py

+    help = help_text(
+        """
+        Additional environment variables to provide to the system binary during fingerprinting.
+        This has no effect on the execution of the binary in the scope of an `adhoc_tool` or


Hmm, I think these env vars should always be set, not just for fingerprinting (although the ad-hoc tool values should override them if specified). In your case it would mean you could set DISPLAY in just one place.

Thanks for the feedback. Need clarification.

Let's assume an adhoc_tool has multiple runnable_dependencies. One of them sets extra_env_vars in the system_binary. That means the adhoc_tool invocation would set the vars for the whole invocation, means for the runnable and all other binaries. Is this a safe assumption?

If yes, I would combine all extra_env_vars of all runnable_dependencies. This may also conflict, e.g. one sets an env var to take it over from pants env, the other binary defines it explicitly. How to deal with this?

If, however, the adhoc_tool defines extra_env_vars I would only consider this field.

Is shell_command also affected?

I think it comes down to what we think the user wants.

I thought about that again and I think you're right.
If a variable is needed for fingerprinting, it most certainly is also needed for running the tool. So I will merge all env var requests of binaries.

Yeah, I think merge them in order of specificity, so the more specific targets overrides the less specific one.

So this documentation needs updating.

ubmarco · 2024-01-10T16:11:22Z

I implemented the extra_env_vars inheritance feature.
Conflicts are resolved as follows:

adhoc_tool -> extra_env_vars always wins over adhoc_tool -> runnable_dependencies -> extra_env_vars
adhoc_tool -> runnable_dependencies -> extra_env_vars are injected in the order as they appear in the BUILD file, so last wins
There are, however, no warnings in case a conflict appears. Is that needed?
A test case is still missing.

benjyw · 2024-01-10T17:42:56Z

I implemented the extra_env_vars inheritance feature. Conflicts are resolved as follows:

adhoc_tool -> extra_env_vars always wins over adhoc_tool -> runnable_dependencies -> extra_env_vars

adhoc_tool -> runnable_dependencies -> extra_env_vars are injected in the order as they appear in the BUILD file, so last wins

This is what I meant by order of specificity, yes.

There are, however, no warnings in case a conflict appears. Is that needed?

I don't think so, you likely want to override on purpose.

A test case is still missing.

benjyw

Thanks! See comments inline.

Going to add @chrisjrn to the reviewers, since he has a better handle on the env vars stuff here than I do

benjyw · 2024-01-10T18:28:26Z

src/python/pants/backend/adhoc/run_system_binary.py

@@ -87,6 +92,9 @@ async def _find_binary(
        env.update(**(rds.extra_env or {}))
        append_only_caches = rds.append_only_caches
        immutable_input_digests = rds.immutable_input_digests
+    if extra_env_vars:
+        extra_env = await Get(EnvironmentVars, EnvironmentVarsRequest(extra_env_vars))
+        env.update(extra_env)


This could stomp the PATH or _PANTS_SHIM_ROOT, no? Seems like the update order should be reversed.

I reversed the order

benjyw · 2024-01-10T18:29:07Z

src/python/pants/backend/adhoc/target_types.py

+    help = help_text(
+        """
+        Additional environment variables to provide to the system binary during fingerprinting.
+        This has no effect on the execution of the binary in the scope of an `adhoc_tool` or


So this documentation needs updating.

benjyw · 2024-01-10T18:48:58Z

src/python/pants/core/util_rules/adhoc_process_support.py

@@ -266,14 +277,16 @@ async def _resolve_runnable_dependencies(
    shim_digest_path = f"_runnable_dependency_shims_{shim_digest.fingerprint}"
    immutable_input_digests = {shim_digest_path: shim_digest}
    _safe_update(immutable_input_digests, merged_extras.immutable_input_digests)
+    environment = {"_PANTS_SHIM_ROOT": "{chroot}"}
+    environment.update(runnable_extra_env_vars)


We should switch the update order here, we never want to stomp _PANTS_SHIM_ROOT (although it's highly unlikely)

Reversed the order

ubmarco · 2024-01-15T23:31:34Z

I addressed the comments. Unfortunately I feel a bit lost on the implementation of the overall feature.
This simple example is still not covered:

system_binary(
    name="printenv",
    binary_name="printenv",
    extra_env_vars=["FOO=BAR"],
)

adhoc_tool(
    name="adhoc",
    runnable=":printenv",
    log_output=True,
    stdout="stdout",
    # extra_env_vars=["FOO=BAR"],
)

then running it as ./pants_from_sources.sh export-codegen docs:adhoc does not show the FOO variable.
In this case the system_binary is not used as runnable_dependencies but as runnable directly.
Also shell commands need to be cared for.
I'm having a hard time finding out where to ideally put the code to avoid duplication.
Also understanding the rule execution order is difficult as there is no call tree in the debugger.
I read the docs but don't feel home yet :-/
Is it possible to generate a (maybe graphical) rule execution tree?

Any advice?

benjyw · 2024-01-16T22:14:15Z

I addressed the comments. Unfortunately I feel a bit lost on the implementation of the overall feature. This simple example is still not covered:
system_binary(
    name="printenv",
    binary_name="printenv",
    extra_env_vars=["FOO=BAR"],
)

adhoc_tool(
    name="adhoc",
    runnable=":printenv",
    log_output=True,
    stdout="stdout",
    # extra_env_vars=["FOO=BAR"],
)
then running it as ./pants_from_sources.sh export-codegen docs:adhoc does not show the FOO variable. In this case the system_binary is not used as runnable_dependencies but as runnable directly. Also shell commands need to be cared for. I'm having a hard time finding out where to ideally put the code to avoid duplication. Also understanding the rule execution order is difficult as there is no call tree in the debugger. I read the docs but don't feel home yet :-/ Is it possible to generate a (maybe graphical) rule execution tree?

Any advice?

Hmm, I'll dive a little deeper but I would like to hear from @chrisjrn , who has the most context here.

chrisjrn · 2024-01-23T16:13:49Z

I'm a bit rusty on this code and would gladly do a pairing session (ask me on Slack!), but my recollection is that the env for the system_binary target eventually ends up getting set in _runnable_dependency_shim and I don't think I see anything set there. Have you caught anything in that code path?

huonw · 2024-03-07T22:14:02Z

Heya, just checking in. What's the status of this PR? Is it ready for a re-review or is it waiting on something else?

ubmarco · 2024-03-13T21:23:21Z

What works:

extra_env_vars for system_binary are used for fingerprinting and in case the system binary is used as runnable_dependencies (my use case)

Pending issues:

extra_env_vars do not work if the system_binary is used as runnable directly in the scope of an adhoc_tool.
test cases are missing
shell scripts may also use the system_binary

Last time I worked on the PR I felt a little lost with the code base and went out of time.

ubmarco marked this pull request as draft January 7, 2024 21:12

benjyw reviewed Jan 7, 2024

View reviewed changes

ubmarco requested a review from benjyw January 10, 2024 16:12

tdyas added the category:new feature label Jan 10, 2024

benjyw reviewed Jan 10, 2024

View reviewed changes

benjyw requested a review from chrisjrn January 10, 2024 18:50

ubmarco requested a review from benjyw January 15, 2024 23:42

ubmarco added 5 commits January 31, 2024 20:09

extra_env_vars field for system_binary

e0a4c93

Inherit extra_env_vars from runnable_deps

21d1e70

Reverse extra_env_vars order

e991cb1

Update docs

3ec49b6

Fix lint issues

db8ef72

ubmarco force-pushed the mh-sys-bin-extra-env-vars branch from 80f3d41 to db8ef72 Compare January 31, 2024 19:09

ubmarco added 2 commits January 31, 2024 20:33

Type fixes

ab8da1d

Fix mypy issues

be2af90

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

extra_env_vars field for system_binary #20374

extra_env_vars field for system_binary #20374

ubmarco commented Jan 7, 2024

benjyw left a comment

benjyw Jan 7, 2024

ubmarco Jan 9, 2024 •

edited

Loading

ubmarco Jan 10, 2024

benjyw Jan 10, 2024

benjyw Jan 10, 2024

ubmarco commented Jan 10, 2024

benjyw commented Jan 10, 2024

benjyw left a comment

benjyw Jan 10, 2024

ubmarco Jan 15, 2024

benjyw Jan 10, 2024

benjyw Jan 10, 2024

ubmarco Jan 15, 2024

ubmarco commented Jan 15, 2024 •

edited

Loading

benjyw commented Jan 16, 2024

chrisjrn commented Jan 23, 2024

huonw commented Mar 7, 2024

ubmarco commented Mar 13, 2024

extra_env_vars field for system_binary #20374

Are you sure you want to change the base?

extra_env_vars field for system_binary #20374

Conversation

ubmarco commented Jan 7, 2024

benjyw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ubmarco Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ubmarco commented Jan 10, 2024

benjyw commented Jan 10, 2024

benjyw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ubmarco commented Jan 15, 2024 • edited Loading

benjyw commented Jan 16, 2024

chrisjrn commented Jan 23, 2024

huonw commented Mar 7, 2024

ubmarco commented Mar 13, 2024

ubmarco Jan 9, 2024 •

edited

Loading

ubmarco commented Jan 15, 2024 •

edited

Loading