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

Initial documentation for Environments #17096

Merged
merged 3 commits into from
Oct 28, 2022

Conversation

stuhood
Copy link
Member

@stuhood stuhood commented Oct 3, 2022

Adds initial documentation for the Environments feature designed and implemented in #7735 and #13682.

This initial version is meant to capture what will be possible in the 2.15.x release, but it will/should contain some references to future work (some of which should block stabilizing environments).

Fixes #13682, and fixes #16393.

[ci skip-rust]
[ci skip-build-wheels]

@stuhood stuhood force-pushed the stuhood/environment-docs-draft branch from 709a11f to 905fecb Compare October 4, 2022 20:17
@stuhood stuhood self-assigned this Oct 4, 2022
@stuhood stuhood force-pushed the stuhood/environment-docs-draft branch from 905fecb to 8c12c51 Compare October 4, 2022 20:27
@stuhood stuhood marked this pull request as ready for review October 4, 2022 20:31
Comment on lines 187 to 199
_TODO: Like https://github.com/pantsbuild/pants/issues/15764, but for tests._

_TODO: Give an example of using `environment=` vs `runtime_environment=` to use docker only for test execution, but not for building of thirdparty dependencies by using PEX to cross-build. This will require exposing options out of PEX to override (?) the target platform, and figuring out how that doesn't break `check` is an open question._
Copy link
Member Author

Choose a reason for hiding this comment

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

So: I'm pretty sure that we should avoid stabilizing environments until we have figured out exactly how to implement a cross-build like this. Many other very useful workflows are enabled, but this one in particular feels like it requires a bit more design work.

This is additionally covered by #8384.

docs/markdown/Using Pants/environments.md Outdated Show resolved Hide resolved
docs/markdown/Using Pants/environments.md Outdated Show resolved Hide resolved
docs/markdown/Using Pants/environments.md Show resolved Hide resolved
Copy link
Contributor

@chrisjrn chrisjrn left a comment

Choose a reason for hiding this comment

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

Note the following:

  • The subsystem/options namespace is called environments-preview (not environments)
  • The specifier field is called _environment (not environment)

docs/markdown/Using Pants/environments.md Outdated Show resolved Hide resolved
docs/markdown/Using Pants/environments.md Outdated Show resolved Hide resolved
docs/markdown/Using Pants/environments.md Outdated Show resolved Hide resolved

> 🚧 Environment compatibility
>
> Currently, there is no static validation that a target's environment is compatible with its dependencies' environments -- only the implicit validation of the goals that you run successfully against those targets (`check`, `lint`, `test`, `package`, etc).
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we provide static validation of runtime configurations when we're not using environments? If not, then this is probably likely to be more confusing than illuminating.

Copy link
Member Author

Choose a reason for hiding this comment

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

We do for resolves, interpreter_constraints, etc. So yea, sortof.

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Thanks! Probably would be good to update options.md too to mention "environment specific options"

docs/markdown/Using Pants/environments.md Outdated Show resolved Hide resolved
docs/markdown/Using Pants/environments.md Outdated Show resolved Hide resolved
docs/markdown/Using Pants/environments.md Outdated Show resolved Hide resolved
docs/markdown/Using Pants/environments.md Outdated Show resolved Hide resolved

Environment targets have fields (target arguments) which correspond to options which are marked "environment-aware". When an option is environment-aware, the value of the option that will be used in an environment can be overridden by setting the corresponding field value on the environment target for that environment. If an environment target does not set a value, it defaults to the value which is set globally via options values.

For example, the [`[python-bootstrap].search_path` option](doc:reference-python-bootstrap#search_path) is environment-aware, which is indicated in its help. It can be overridden for a particular environment by a corresponding environment target field, such as [the one on `local_environment`](doc:reference-local_environment#codepython_bootstrap_search_pathcode).
Copy link
Contributor

Choose a reason for hiding this comment

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

It would probably help to have a BUILD file example where you set this, and then have a comment in it saying "this overrides the option [python-bootstrap].search_path

People tend to really benefit from examples


## Consuming environments

To declare which environment they should build with, many target types (but particularly "root" targets like tests or binaries) have an `environment=` field: for example, [`python_tests(environment=..)`](doc:reference-python_tests#codeenvironmentcode).
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this sentence going to soon be stale?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unclear: we'll learn more during the alpha and rcs I expect.

docs/markdown/Using Pants/environments.md Show resolved Hide resolved
docs/markdown/Using Pants/environments.md Show resolved Hide resolved
@stuhood stuhood force-pushed the stuhood/environment-docs-draft branch from 8c12c51 to 3521354 Compare October 11, 2022 18:00
@stuhood
Copy link
Member Author

stuhood commented Oct 11, 2022

Applied review feedback, but haven't addressed the TODOs yet.


To declare which environment they should build with, many target types (but particularly "root" targets like tests or binaries) have an `environment=` field: for example, [`python_tests(environment=..)`](doc:reference-python_tests#codeenvironmentcode).

The `environment=` field may either:
Copy link
Member

Choose a reason for hiding this comment

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

Are target field names referred to with the trailing equal sign else where (besides this PR)? It feels like a new (i.e. inconsistent) thing to me..

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess that it is a convention that I'm trying to start.

I'm not really sure that end users know what a "field" is otherwise... most users probably think of the arguments to targets as ... arguments.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we could have a glossary page listing terminology and syntax for how we typically refer to them, such as target field names always being on the form of field_name=, if we also do that in messages from pants that would make it more clear with fewer words in many cases I believe.

docs/markdown/Using Pants/environments.md Outdated Show resolved Hide resolved
docs/markdown/Using Pants/environments.md Show resolved Hide resolved
@stuhood stuhood added this to the 2.15.x milestone Oct 25, 2022
[ci skip-rust]

[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@stuhood stuhood force-pushed the stuhood/environment-docs-draft branch from 3521354 to ff9a75b Compare October 27, 2022 23:00
@stuhood
Copy link
Member Author

stuhood commented Oct 27, 2022

Ok folks: I think that this is ready to go. Please take another look.

Copy link
Member

@kaos kaos left a comment

Choose a reason for hiding this comment

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

lgtm. thanks for this comprehensive write up. :)

@stuhood stuhood changed the title Initial draft of documentation for Environments Initial documentation for Environments Oct 28, 2022
@stuhood stuhood enabled auto-merge (squash) October 28, 2022 16:48
@stuhood stuhood merged commit d7880ce into pantsbuild:main Oct 28, 2022
@stuhood stuhood deleted the stuhood/environment-docs-draft branch October 28, 2022 16:58
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Thanks!

```python BUILD
pex_binary(
name="main",
environment="python_bullseye",
Copy link
Contributor

Choose a reason for hiding this comment

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

This should show the pants.toml imo. Generally, I recommend maybe adding -tgt to the target name for all these env targets in this page. It's way too easy to think that environment="target name" rather than name as defined in pants.toml

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