-
-
Notifications
You must be signed in to change notification settings - Fork 606
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
__defaults__(all=...)
is a future-compatibility hazard for adding new field
#20925
Comments
This is a good find, and thanks for the write up, Huon. I think With that, I fully agree better diagnostics is a must, to help identify what failed (in this example, providing an invalid value for Another observation is, that we may want to consider doing less up-front validation of the value during target creation, and post-pone it to when it's actually being used. In this case, the The downside being that Which leads me to conclude that we may want to introduce an intermediate step for creating field values, where the first step is merely structural, and would be used for -- -- __defaults__({("python_source", "pex_binary"): dict(tags=["foo"]}) |
Ah, yes. Strings seems reasonable to solve this use-case (although I note there's the risk that someone makes a typo like
I think the error observed #20894 (comment) was from pex, because it was being passed an incorrect argument. That is, Pants already didn't do much validation, and so the argument was passed through to the underlying
👍 |
👍🏽
Right. I misread the comment thinking the error message came up-front, not when running a goal that invoked With that line of thought, one approach could be to have a flag on the field types to indicate if they're unsuitable for All of the above, to open up the idea of providing defaults per field type, rather than per target field. So instead of __defaults__({python.interpreter_constraints: "==3.10.*"})
# This would be equivalent to
__defaults__(all={"interpreter_constraints": "==3.10.*"})
# except, only apply to interpreter_constraints for targets using the Field from the Python backend. I actually kind of like this, and we could deprecate # Example to set default tags. Being a default field type present on practically all targets.
__defaults__({core.tags: ["tag1", "tag2"]}) |
Ah, yeah, great idea, so scope/group/namespace the fields somehow, and then defaulting within a namespace much reduces the risk here! Nice. |
Describe the bug
The
all
functionality for__default__
is somewhat of a future-compatibility hazard:t1
with a field calledf
(this could be in any backend, even a private in-repo one), and a second built-in target calledt2
without that field.t2
also calledf
.t2
target will now becomet2(name="two", f="abc")
after applying__defaults__
, and this could either:abc
doesn't make sense fort2
'sf
field)With Pants' current behaviour, if we wanted to be strict about not breaking existing code, this means we could never add new fields, because someone could have an existing field of the same name, and use it in
__defaults__(all=...)
. Not ever adding new fields is clearly untenable, so either we have to:all
works (e.g. deprecate and remove it(!), have some notion of "all fields as of a particular release", so people need to bump that )I think we should make an explicit policy decision (even if it's just "users just have to tolerate any breakage due to
all
").For instance, in #20894 (comment), a user had:
In 2.20 and earlier (before #20737), the only built-in target with the
extra_build_args
field wasdocker_image
, and so thisall
was applying that docker-formatted build arg to all.In 2.21 (which includes #20737),
pex_binary
also has this field. It has completely different meaning (a list of CLI args to pass to apex
invocation). Thus, that__default__
configuration starts settingpex_binary(..., extra_build_args=["PYTHON_IMAGE=python:3.11"])
and understandably an invocation likepex ... PYTHON_IMAGE=python:3.11
doesn't work.The fix the user identified is to move the field from
all
todocker_image
:Pants version
All, although specific example is in 2.21
OS
All
Additional info
N/A
The text was updated successfully, but these errors were encountered: