Add overrides field to python_sources and python_tests target
#13270
Conversation
# 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]
# 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]
|
Bump @benjyw. Also, I think we should add this to Stretch goal would be getting the old-style macros |
| @@ -670,13 +698,37 @@ class PythonSourcesGeneratingSourcesField(PythonGeneratingSourcesBase): | |||
| ) | |||
|
|
|||
|
|
|||
| class PythonSourcesOverridesField(OverridesField): | |||
| help = ( | |||
| "Override the field values for generated `python_source` targets.\n\n" | |||
There was a problem hiding this comment.
Have you checked how this will render on the docsite?
There was a problem hiding this comment.
I've checked with ./pants help-all and it looks good. I care a ton about how help messages render, which is why I always pester people to add \n\n.
How would I check on the docsite? The dry-run only does markdown, right?
| value_or_default = super().compute_value(raw_value, address) | ||
| if value_or_default is None: | ||
| return None | ||
| invalid_type_exception = InvalidFieldTypeException( |
There was a problem hiding this comment.
It's weird to instantiate an exception you may never use. Maybe have this be a function?
[ci skip-rust] [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
8dda2fa to
8f33458
Compare
Given that Stu is OOO for another day, going to merge. But based on his feedback in #13187 (comment) and in private conversation, it sounds like Stu is more concerned about 1:1:1 rather than the generic
This PR does not change 1:1:1 at all. It actually makes 1:1:1 much more viable (one target generator per directory). I'm holding off on #13187 because we're not certain if we should use 1:1:1 with Go or not, but here, seems uncontroversial for Python. I'm happy to address specific followups Stu has. |
Yep: this all makes sense, and I'm fine with adding The real discussion that needs to continue is around 1:1:1. |
…tobuf_sources` (#13298) See #13270. This does not yet add the field to `java_sources` and `junit_tests`, tracked by #13297. I'm not sure if we should add this mechanism to `files` and `resources`? I'm having a hard time thinking of when you would want to override the metadata for a single file. But, we might want to do it for the sake of consistency? This also does not hook up `python_requirements` and `poetry_requirements` because those use the old CAOF macro system, rather than the Target API. [ci skip-rust]
…o new target generator `python_test_utils`, not `python_tests` (#13299) Part 1 of #13238. ## Why we need to fix this Currently, `python_tests` includes `conftest.py` and `*_test.pyi` in its `sources` field, and it generates `python_test` targets for those. We special case our Pytest code to ignore running tests on those generated targets: https://github.com/pantsbuild/pants/blob/e8654186c11c87e372bd62ab4f82f39ad2f5b208/src/python/pants/backend/python/subsystems/pytest.py#L50-L57 This will not work with lockfiles #13238: `conftest.py` and `*_test.pyi` both must be modeled by a `python_source` target, not `python_test`. A `python_test` target has the field `resolve: str`, whereas `python_source` has `compatible_resolves: list[str]`. The `resolve` says which exact resolve to use, and then we validate the transitive closure is compatible with that one. Because a `conftest.py` can be a dependency of many `python_test` targets, it's important that it participates in this `compatible_resolves` mechanism. ## Solution Add a new `python_test_utils` target generator, which generates `python_source` targets. It behaves identically to the `python_sources` target generator, except that it has the default `sources` of `conftest.py` and `*_test.pyi`. Why a new target that behaves identically to `python_sources`? This modeling reduces the risk of folks unintentionally including test support files like `conftest.py` in production, such as in a `pex_binary` and `python_distribution`. For `python_distribution`, it's very common to add a dependency on a `python_sources` target generator, rather than a generated `python_source` target, i.e. `:lib` instead of `foo.py:lib`. We want to avoid the gotcha of `:lib` including `conftest.py` because that's how the default `sources` work, and we don't want users to have to be aware of this gotcha and to do set arithmetic in the `sources` fields of two `python_sources` to make sure there aren't duplicate owners of the same `conftest.py`. A new target also dramatically minimizes the breaking changes of this PR. Now, the risk is that some files will be unowned, whereas before there was the risk that they'd be owned by a different target generator than before, so might inherit different metadata like `dependencies` and might be included in unexpected places like a `python_distribution` depending on `:lib`. -- This PR does not deprecate the special casing in `pytest.py` that allows for a `python_test` target to be used with `conftest.py` and `*_test.pyi`. That special casing will be deprecated in a followup. ## Alternatives considered ### `python_tests` generates `python_source` targets This is technically feasible, but really clunky and confusing to document. For example, the `conftest.py` `python_source` target would presumably default to the global `compatible_resolves` option (configured in `[python-setup]`). To change it, we'd either need to add a field to `python_tests` like `conftest_compatible_resolves`, or the user would have to know to use the `overrides` mechanism from #13270. Not intuitive! ### Deprecating having a default `sources` field for `python_tests` and `python_sources` Users would have to either explicitly use the new defaults or stick to the old defaults. Once we're confident people aren't using a default, we can safely change it without breaking anyone. However, this would require a ludicrous amount of churn for a problem that most users don't even have. Even if we automated this change with `./pants update-build-files`, it is still extremely disruptive. Also, this isn't very feasible because of how we validate that each glob in the `sources` field matches. When it's the field's `default`, we only require _any_ glob to match. When it's explicitly declared in a BUILD file, we require _all_ globs to match. So, a user could not safely set `["*_test.py", "test_*.py", "tests.py"]` in a BUILD file unless they had all those files, which is unlikely. ### A global option to change the default I don't think this is technically feasible. The Target API is intentionally shielded from the Rules API. There's no way for an option to change the default of a field. It would require a ludicrous amount of special casing in `engine/internals/graph.py` etc to have our Rules code that consumes these fields to change behavior of things like hydrating sources based on the option. ## Impact Our dependency inference rule for `conftest.py` requires that there be an owning target: https://github.com/pantsbuild/pants/blob/a9af04dc94de2f718aae5b8ce488faaf8c39c07e/src/python/pants/backend/python/dependency_inference/rules.py#L231-L235 So,, unless you have explicitly put `conftest.py` into a `python_sources` or `python_tests` target's `sources` field, then you'll get this error when upgrading to Pants 2.8: ``` pants.base.exceptions.ResolveError: No owning targets could be found for the file `src/python/pants/conftest.py`. Please check that there is a BUILD file in the parent directory src/python/pants with a target whose `sources` field includes the file. See https://www.pantsbuild.org/v2.8/docs/targets for more information on target definitions. You may want to run `./pants tailor` to autogenerate your BUILD files. See https://www.pantsbuild.org/v2.8/docs/create-initial-build-files. ``` It violates our deprecation policy to error here, but we don't think there's a way to gracefully deprecate. Hopefully folks will run `./pants tailor` after encountering this and get `python_test_utils` targets added. When a new `python_test_utils` target is added, the user may need to update its metadata like its `dependencies` to work properly. That might not be very intuitive, so we should document it in upgrade notes. Fortunately, the risk is mostly that users' tests will fail: there is far less risk this will break production code. [ci skip-rust] [ci skip-build-wheels]
As explained in the target generator proposal, this allows ergonomically changing the metadata for a single
python_source/python_testtarget without having to split out a newpython_sources/python_teststarget and then doing set arithmetic on thesourcesfield to make sure that only one target owns the file.For example, you can now set: