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

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

Conversation

grihabor
Copy link
Contributor

@grihabor grihabor commented Oct 5, 2024

Precompiled binaries that were not created for NixOS usually have a so-called link-loader hardcoded into them. On Linux/x86_64 this is for example /lib64/ld-linux-x86-64.so.2. for glibc. NixOS, on the other hand, usually has its dynamic linker in the glibc package in the Nix store and therefore cannot run these binaries.

The solution is to use nix-ld, but it only works if you set NIX_LD env variable for every process call.

This pr adds a global option process_extra_env to set arbitrary env variables for every pants Process call.

grihabor added a commit to grihabor/pants-nix that referenced this pull request Oct 5, 2024
@benjyw
Copy link
Contributor

benjyw commented Oct 16, 2024

Thanks for tackling this!

So we actually already have an option for this: https://www.pantsbuild.org/dev/reference/subsystems/subprocess-environment

The problem is that we only actually apply those env vars in some Python-related processes. But the documentation of that option is very clear that it should apply to all processes. So this is simply broken today.

Therefore, instead of the new option, could you make this existing option apply to all processes, and remove the special handling in the python cases? This would also close #16565 .

@grihabor
Copy link
Contributor Author

@benjyw I'm getting a bunch of rule errors. It looks like SubprocessEnvironment uses EnvironmentAware class which afaik needs an EnvironmentName which means that I can't use it in the -> Process rule, is this correct? If so, shall we create a different option for Process env vars?

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.

@benjyw
Copy link
Contributor

benjyw commented Oct 31, 2024

I've asked @stuhood to take a look at this, as his engine expertise is second to none.

@grihabor
Copy link
Contributor Author

Thank you!

@grihabor
Copy link
Contributor Author

@benjyw @stuhood Any updates on this?

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.

2 participants