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

Fix hermetic environment issues with V2 tasks #7721

Merged
merged 11 commits into from May 20, 2019

Conversation

Projects
None yet
4 participants
@Eric-Arellano
Copy link
Contributor

commented May 14, 2019

Problem

Specific test failures

Several of the util tests were failing due to issues with using a completely hermetic environment, as follows:

  • test_contextutil.py assumes that $USER is defined in the env, which is not the case in V2.
  • test_dirutil.py defaulted to using the ASCII codec for its encoding when writing via Python's open(), because the environment does not define LC_ALL or LANG.
  • Any test that uses test_base.py ends up needing the cryptography wheel. This could not be built properly on macOS environments because the OpenSSL headers were not set up properly.

Way to specify environment variables

The naive solution to these problems would simply grab the local environment variables for LC_ALL / LANG and CPPFLAGS / LDFLAGS. This works during local execution, but presents issues for remote execution where we want to have fine-grained control over the environment.

So, we must have some way to hardcode these environment variables via options, while providing sensible defaults that just work in the average case.

The greater design question is tracked by #7735.

Solution

Introduce a new idiom of using a Subsystem to allow users to have fine-grained control over defining each env var, and then add datatypes and rules that compose the individual options into a meaningful set of options that are all required together to achieve the desired behavior.

For example, to fix the encoding problem, we need to surface both LC_ALL and LANG to the ExecuteProcessRequest, so, we want a simple way for the code to access those two variables together; the abstraction should be at the level of behavior, not the individual env vars. We also want users to be able to define each individual env var separately, so the external user API should be at the abstraction of an individual env var.

This does not close out #7735, as this design is not yet general enough for what we will likely need for future cases. Instead, we try to solve this problem with the minimum diff possible, while getting us closer to the final design.

@Eric-Arellano Eric-Arellano requested a review from illicitonion May 14, 2019

@Eric-Arellano Eric-Arellano force-pushed the Eric-Arellano:v2-pytest-fix-util branch from 4a63021 to e81ef9d May 14, 2019

@Eric-Arellano Eric-Arellano marked this pull request as ready for review May 14, 2019

@cosmicexplorer
Copy link
Contributor

left a comment

This is a great solution, seemingly with lots of work underneath it! I would love to at least have an idea of how we would handle generalizing the solutions here (such as setting LC_ALL and CPPFLAGS) to other rules who may want to use them in the future before merging.

@Eric-Arellano Eric-Arellano changed the title Fix hermetic environment issues with V2 pytest runner WIP: Fix hermetic environment issues with V2 pytest runner May 16, 2019

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

@cosmicexplorer ah oops, sorry I didn't mark this as WIP!

Daniel, Stu, and I this morning talked about how we likely want to convert this into a Subsystem before making these changes. See #7735. It's still not clear the scope of how much we need to do for the sake of unblocking remoting, and who is going to lead that work, but for the moment this PR is still a WIP.

Nevertheless, great comments on the contextutil fixes - those will be very helpful regardless of how we approach the env for subprocesses.

Eric-Arellano added some commits May 14, 2019

Fix contextutil test
The hermetic environment tests assumed that the environment was already non-hermetic. Because V2 is hermetic by default, this assumption was not true, so the test failed.
@Eric-Arellano
Copy link
Contributor Author

left a comment

Not ready for review until I figure out #7735 (thanks Danny for commenting on that!).

Show resolved Hide resolved tests/python/pants_test/util/test_contextutil.py Outdated
Show resolved Hide resolved tests/python/pants_test/util/test_contextutil.py Outdated
Show resolved Hide resolved tests/python/pants_test/util/test_contextutil.py Outdated
Show resolved Hide resolved src/python/pants/backend/python/rules/python_test_runner.py Outdated

@Eric-Arellano Eric-Arellano force-pushed the Eric-Arellano:v2-pytest-fix-util branch from d87e3f9 to dcc517e May 16, 2019

@Eric-Arellano Eric-Arellano changed the title WIP: Fix hermetic environment issues with V2 pytest runner Fix hermetic environment issues with V2 tasks May 20, 2019

Solution has changed substantially.

@illicitonion
Copy link
Contributor

left a comment

Introducing a single flag for each env var here, when we're immediately going to completely re-work it to have a per-platform flag, seems a little weird, but makes sense as a stepping stone :)

@@ -77,6 +79,7 @@ def run_python_test(test_target, pytest, python_setup, source_root_config):
interpreter_constraint_args = parse_interpreter_constraints(
python_setup, python_target_adaptors=all_targets
)
interpreter_search_paths = text_type(create_path_env_var(python_setup.interpreter_search_paths))

This comment has been minimized.

Copy link
@illicitonion

illicitonion May 20, 2019

Contributor

Add a TODO that this won't work when executing remotely, because we'll need to do path resolution on the remote system or establish known paths.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano May 20, 2019

Author Contributor

I don't think anything in source code is going to have to change for this. All we'll have to do is change the options value for interpreter_search_paths, and everything should work!

Because the TODO is just changing config values and not actual src code, I'm going to not add a TODO.

Show resolved Hide resolved src/python/pants/backend/python/subsystems/python_native_code.py Outdated
@cosmicexplorer
Copy link
Contributor

left a comment

Great!!

@@ -38,6 +43,10 @@
from pants.goal.task_registrar import TaskRegistrar as task


def global_subsystems():
return (SubprocessEnvironment, PythonNativeCode)

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer May 20, 2019

Contributor

Might want to leave a comment that this is necessary for these subsystems to be recognized as optionable_rule()s, maybe (so that others can copy off of this for further v2 work)? This might change in the future if we allow subscoped dependencies.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano May 20, 2019

Author Contributor

I think it's fine without this comment because it seems to be standard practice to register subsystems this way.

Also, let's say we add a new subsystem that does not at all interact with optionable_rule(), then the comment for this function would become misleading.

Show resolved Hide resolved src/python/pants/backend/python/subsystems/python_native_code.py Outdated
@Eric-Arellano
Copy link
Contributor Author

left a comment

Introducing a single flag for each env var here, when we're immediately going to completely re-work it to have a per-platform flag, seems a little weird, but makes sense as a stepping stone :)

Agreed - this PR is very much focused on unblocking remoting work and not making the final design. Hopefully it helps get us closer to that final design at least.

@@ -77,6 +79,7 @@ def run_python_test(test_target, pytest, python_setup, source_root_config):
interpreter_constraint_args = parse_interpreter_constraints(
python_setup, python_target_adaptors=all_targets
)
interpreter_search_paths = text_type(create_path_env_var(python_setup.interpreter_search_paths))

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano May 20, 2019

Author Contributor

I don't think anything in source code is going to have to change for this. All we'll have to do is change the options value for interpreter_search_paths, and everything should work!

Because the TODO is just changing config values and not actual src code, I'm going to not add a TODO.

@@ -38,6 +43,10 @@
from pants.goal.task_registrar import TaskRegistrar as task


def global_subsystems():
return (SubprocessEnvironment, PythonNativeCode)

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano May 20, 2019

Author Contributor

I think it's fine without this comment because it seems to be standard practice to register subsystems this way.

Also, let's say we add a new subsystem that does not at all interact with optionable_rule(), then the comment for this function would become misleading.

Show resolved Hide resolved src/python/pants/backend/python/subsystems/python_native_code.py Outdated
@stuhood
Copy link
Member

left a comment

Thanks!

help='Override the `LC_ALL` environment variable for any forked subprocesses.')


class SubprocessEncodingEnvironment(datatype([

This comment has been minimized.

Copy link
@stuhood

stuhood May 20, 2019

Member

I'm not sure that the datatype is necessary here (could just consume the Subsystem?) but not a blocker.

@Eric-Arellano Eric-Arellano merged commit aed9ac6 into pantsbuild:master May 20, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:v2-pytest-fix-util branch May 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.