-
-
Notifications
You must be signed in to change notification settings - Fork 652
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
Represent generated subtarget addresses as file names #10338
Represent generated subtarget addresses as file names #10338
Conversation
[ci skip-rust-tests]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
For alignment with target addresses and the sources field, we will plan to allow relative references to generated subtargets. foo.py will be relative to the BUILD file, and you need //foo.py to mean relative to the build root. This does not work with subdirectories. subdir/a.py is relative to the build root.
As it stands, the only way to get a relative address is by prefixing with :target_name
, so depending on how the "ambiguous owners" syntax shakes out, it might be more consistent to have a relative file be :file.py
. Unclear.
@@ -121,6 +122,10 @@ class Address: | |||
def parse(cls, spec: str, relative_to: str = "", subproject_roots=None) -> "Address": | |||
"""Parses an address from its serialized form. | |||
|
|||
Note that this does not work properly with generated subtargets, e.g. the address | |||
`helloworld/app.py`. We would not be able to calculate the `generated_base_target_name`, so | |||
we treat this like a normal target address. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should maybe open a followup ticket with a short blurb describing the open question here (and referencing the doc for more info).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is necessary. I'm actively iterating on this part of the codebase the next day or two. It's possible that we adapt Address.parse()
to be able to parse file names, given a hint for generated_base_target_name
. But it's also possible that we want that logic to live somewhere else, like the dependency inference rule. Unclear, but will be answered very soon.
@@ -181,8 +179,9 @@ def __init__( | |||
:param generated_base_target_name: If this Address refers to a generated subtarget, this | |||
stores the target_name of the original base target. | |||
""" | |||
self._spec_path = self.sanitize_path(spec_path) | |||
self.validate_path(spec_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably remove the return value from this method too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I meant to do that. Thanks!
@@ -225,14 +228,19 @@ def relative_spec(self) -> str: | |||
""" | |||
:API: public | |||
""" | |||
return f":{self._target_name}" | |||
# NB: It's possible for the target name of a generated subtarget to be in a subdirectory, | |||
# e.g. 'subdir/f.txt'. When this happens, the relative spec is not safe. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should reference the ticket for the ambiguity syntax here as well probably.
Agreed, but this would also introduce a new ambiguity between target addresses vs. generated subtargets. It's possible to have a traditional file named I wonder if there's another symbol we could use? I was thinking |
# Rust tests will be skipped. Delete if not intended. [ci skip-rust-tests]
…ubtargets-spec [ci skip-rust-tests]
Generally that's addressed via |
Before:
After:
This change solves several key questions with file args, as explained in https://docs.google.com/document/d/14MAst0Q0HpoLqysiN7KoKDkA6-Ga_KhjICTl0EC16uI/edit#heading=h.y495n7exlx1x:
!
in the dependencies field of BUILD files.For alignment with target addresses and the
sources
field, we will plan to allow relative references to generated subtargets.foo.py
will be relative to the BUILD file, and you need//foo.py
to mean relative to the build root. This does not work with subdirectories.subdir/a.py
is relative to the build root.Multiple owners
If there are multiple owners of a target, and we generate a subtarget over the same file for each owner, then the resulting addresses would all "look" the same to the user because they would have the same
Address.spec
. This is confusing to the user.For now, we defer this problem by never using generated subtargets when there are multiple owners. In the feature, we could add a syntax to refer to the original owner when there's ambiguity, e.g.
helloworld/app.py@:original_target
. This is safe because of the following semantics with multiple owners:[ci skip-rust-tests]