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

Jump to bottom

Make more option values environment-sensitive #16984

Merged
merged 10 commits into from
Sep 26, 2022
Merged

Make more option values environment-sensitive #16984

merged 10 commits into from
Sep 26, 2022

Conversation

chrisjrn
Copy link
Contributor

@chrisjrn chrisjrn commented Sep 23, 2022 •
edited
Loading

This migrates most of the options identified in the design document for environment configuration to the new Subsystem.EnvironmentAware mechanism.

This includes:

  • [apache-thrift]
  • [python-native-code]
  • [shell-setup]
  • [subprocess-environment]
  • [twine]
  • [pex]
  • [docker]
  • [test] goal
  • [jvm]

Please let me know if more options in your subsystem should have support for per-environment option settings

Unaddressed from the design doc:

Currently [golang] is missing, because there's a few fields beyond those in the design doc that may need to be migrated, and I am awaiting confirmation. If those questions are resolved before this is ready for merge, I'll add them.

There's also some discussion needed about field values on targets, which are currently not part of the environment configuration system, it is not clear that we need to support these.

Christopher Neugebauer added 10 commits September 23, 2022 11:41
[apache-thrift].thrift_search_paths
359f80e 
# 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]
[python-native-code]
a853c4e 
# 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]
[shell-setup]
db0772b 
# 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]
[subprocess-environment]
33c4ce6 
# 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]
[twine]
3906711 
# 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]
[pex]
57a90c9 
# 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]
[jvm]
26a95f8 
# 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]
[test]
4fbbcac 
# 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]
Add support for ShellStrListOption
7ac4dfc 
# 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]
[docker]
2465f7d 
# 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]
@chrisjrn chrisjrn marked this pull request as ready for review September 23, 2022 18:52
@Eric-Arellano
Copy link
Contributor

Is this PR as cut-and-dry as it seems? In the subsystem definition files, no changes beyond moves?

Also this is probably an internal only change. It doesn't impact existing users in any way. Only gives them a new feature, which we aren't yet publicizing

@chrisjrn
Copy link
Contributor Author

@Eric-Arellano It is absolutely as cut-and-dry as it seems. Options just need to be moved into EnvironmentAware, and if you need to consume the object, you just add that to the set of objects you inject into the rule. As far as I can tell, everything Just Works.™

@chrisjrn chrisjrn added category:internal CI, fixes for not-yet-released features, etc. and removed category:user api change labels Sep 23, 2022
Eric-Arellano
Eric-Arellano approved these changes Sep 23, 2022
View reviewed changes
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.

Beautiful <3 None of my comments are blocking, they're more notes for future follow ups we need to do

src/python/pants/backend/python/subsystems/python_native_code.py
@@ -13,21 +13,22 @@ class PythonNativeCodeSubsystem(Subsystem):
options_scope = "python-native-code"
Copy link
Contributor

Choose a reason for hiding this comment

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

For posterity: this subsystem has several problems, including #14612

I don't think it blocks this PR, given that environment targets are still experimental. But we should probably fix that ticket before 2.15

src/python/pants/backend/python/subsystems/twine.py

# The certs file will typically not be in the repo, so we can't digest it via a PathGlobs.
# Instead we manually create a FileContent for it.
ca_certs_content = Path(ca_certs_path).read_bytes()
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not safe, but can be fixed in a follow up #16987

Copy link
Contributor Author

@chrisjrn chrisjrn Sep 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It's also fundamentally not correct :)

Happy to leave it as and fix in follow-up

src/python/pants/core/util_rules/subprocess_environment.py
@@ -20,25 +20,26 @@ class SubprocessEnvironment(Subsystem):
options_scope = "subprocess-environment"
Copy link
Contributor

Choose a reason for hiding this comment

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

For posterity, this is a really bad subsystem name. I think we only use it in Python.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, we could probably use the deprecation mechanism here if we have a better name.

kaos
kaos approved these changes Sep 25, 2022
View reviewed changes
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

@chrisjrn chrisjrn merged commit 3bdbb7d into pantsbuild:main Sep 26, 2022
@chrisjrn chrisjrn deleted the chrisjrn/make_fields_environment_sensitive branch September 26, 2022 15:22
@wisechengyi wisechengyi mentioned this pull request Oct 2, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Eric-Arellano Eric-Arellano Eric-Arellano approved these changes

@kaos kaos kaos approved these changes

@stuhood stuhood Awaiting requested review from stuhood

@thejcannon thejcannon Awaiting requested review from thejcannon

@jsirois jsirois Awaiting requested review from jsirois

Assignees
No one assigned
Labels
category:internal CI, fixes for not-yet-released features, etc.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chrisjrn @Eric-Arellano @kaos