Allow going from Addresses -> Address
#9097
Conversation
This avoids an upcoming import cycle
This removes graph ambiguity issues.
Eric-Arellano
changed the title
Addresses -> Address
| class MappingError(Exception): | ||
| """Indicates an error mapping addressable objects.""" | ||
|
|
||
|
|
||
| class UnaddressableObjectError(MappingError): | ||
| """Indicates an un-addressable object was found at the top level.""" | ||
|
|
||
|
|
||
| class DuplicateNameError(MappingError): | ||
| """Indicates more than one top-level object was found with the same name.""" | ||
|
|
||
|
|
||
| class ResolveError(MappingError): | ||
| """Indicates an error resolving targets.""" |
There was a problem hiding this comment.
Moved to avoid a cyclic import. These also arguably belonged here originally as they are used in multiple files.
| @rule | ||
| async def find_build_file(address: Address) -> BuildFileAddress: | ||
| address_family = await Get[AddressFamily](Dir(address.spec_path)) | ||
| # NB: `address_family.addressables` is a dictionary of `BuildFileAddress`es and we look it up | ||
| # with an `Address`. This works because `BuildFileAddress` is a subclass, but MyPy warns that it | ||
| # could be a bug. | ||
| struct = address_family.addressables.get(address) # type: ignore[call-overload] | ||
| addresses = address_family.addressables | ||
| if not struct or address not in addresses: | ||
| _raise_did_you_mean(address_family, address.target_name) | ||
| return next(build_address for build_address in addresses if build_address == address) |
There was a problem hiding this comment.
This is prework for the next step of the project: no longer storing BuildFileAddress in HydratedTarget. Once we make that change, we need this rule so that we have a way to go from Address -> BuildFileAddress.
| @@ -222,7 +227,7 @@ def _hydrate(item_type, spec_path, **kwargs): | |||
| address_family_by_directory = {af.namespace: af for af in address_families} | |||
|
|
|||
| matched_addresses = OrderedSet() | |||
| addr_to_origin: Dict[BuildFileAddress, AddressSpec] = {} | |||
| bfaddr_to_origin: Dict[BuildFileAddress, AddressSpec] = {} | |||
There was a problem hiding this comment.
The below changes are only renames to make clear that this is a BFA rather than Address. Will make the follow up easier to understand.
| def _inject_addresses(self, subjects): | ||
| """Injects targets into the graph for each of the given `Address` objects, and then yields them. | ||
|
|
||
| TODO: See #5606 about undoing the split between `_inject_addresses` and `_inject_address_specs`. | ||
| """ | ||
| logger.debug('Injecting addresses to %s: %s', self, subjects) | ||
| with self._resolve_context(): | ||
| addresses = tuple(subjects) | ||
| thts, = self._scheduler.product_request(TransitiveHydratedTargets, | ||
| [BuildFileAddresses(addresses)]) | ||
|
|
||
| self._index(thts.closure) | ||
|
|
||
| yielded_addresses = set() | ||
| for address in subjects: | ||
| if address not in yielded_addresses: | ||
| yielded_addresses.add(address) | ||
| yield address | ||
|
|
There was a problem hiding this comment.
This was never used.
| # NB: The HydratedTargets are not valid HydratedTargets, such as using a str instead of | ||
| # Address for the .address field. This is okay because it makes the tests much easier to work | ||
| # with and the topo_sort rule does not actually care about the HydratedTarget fields. |
There was a problem hiding this comment.
Leftover from prior PR.
| @@ -39,31 +39,26 @@ class Filedeps(Goal): | |||
| @goal_rule | |||
There was a problem hiding this comment.
This file only has some light refactoring. We're going to need to change this file once HydratedTarget.address is no longer a BFA so that we await Get[BuildFileAddress](Address).
Eric-Arellano
force-pushed
the
single-address
branch
from
a31f035
to
851653f
Compare
| @@ -80,6 +81,17 @@ def _raise_did_you_mean(address_family: AddressFamily, name: str, source=None) - | |||
| raise resolve_error from source | |||
| raise resolve_error | |||
|
|
|||
| @rule | |||
| async def find_build_file(address: Address) -> BuildFileAddress: | |||
There was a problem hiding this comment.
So, this works, but we should take a look at how it is being used, and see whether it should be batched. The parsing will be memoized obviously, but each usage is O(N) in the number of targets in a directory.
There was a problem hiding this comment.
Sounds good. For now, it adds no new complexity than what we had before. But, after we finish this project, we'll be able to see if we can improve this. Thanks!
Part of #9083.
This allows us to entirely use
AddressesandAddressin V2 rules, rather thanBuildFileAddresses. Tangibly,runandtestcan now requestAddressesin their signature.The only remaining piece is changing
HydratedTarget,TargetWithOrigin, andTargetAdaptorto storeAddressrather thanBuildFileAddress, which is saved for a followup due to issues with V1 still expectingBuildFileAddress.