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

(Attempt to) Deprecate/Remove BuildFileAddress #6657

Closed
stuhood opened this issue Oct 18, 2018 · 2 comments · Fixed by #9100
Closed

(Attempt to) Deprecate/Remove BuildFileAddress #6657

stuhood opened this issue Oct 18, 2018 · 2 comments · Fixed by #9100

Comments

@stuhood
Copy link
Member

stuhood commented Oct 18, 2018 •
edited
Loading

BuildFileAddress is (likely) only a remnant of now-removed code from the previous BuildFile parsing infrastructure. We should attempt to deprecate it and remove all usages.

"Actual" usages would currently all be using BuildFileAddress.rel_path, since the build_file field is no longer populated. So:

$ rg '\.rel_path' | grep -i 'address'
tests/python/pants_test/build_graph/test_build_file_parser.py:                     {address.rel_path for address in addresses})
tests/python/pants_test/build_graph/test_build_file_address_mapper.py:    self.assertEqual('BUILD', address.rel_path)
src/python/pants/build_graph/address.py:    self.rel_path = rel_path
src/python/pants/build_graph/address.py:            .format(rel_path=self.rel_path, target_name=self.target_name))
src/python/pants/build_graph/build_file_address_mapper.py:    return address.rel_path == file_path
src/python/pants/build_graph/build_file_address_mapper.py:      len({address.rel_path for address in addresses}) == 1):
src/python/pants/build_graph/build_file_address_mapper.py:      specs = [':{} (from {})'.format(address.target_name, os.path.basename(address.rel_path))
src/python/pants/engine/addressable.py:    normalized_address = BuildFileAddress(rel_path=address.rel_path, target_name=target_name)
src/python/pants/build_graph/intermediate_target_factory.py:    address = Address.parse(address, self._parse_context.rel_path)
src/python/pants/build_graph/source_mapper.py:        self._source_to_address[address.rel_path].add(address)
src/python/pants/build_graph/build_file_parser.py:                      addressable_file=address.rel_path,
src/python/pants/backend/python/pants_requirement.py:      target = Address(spec_path=self._parse_context.rel_path, target_name=name)
src/python/pants/engine/legacy/address_mapper.py:    return {bfa.rel_path for bfa in build_file_addresses}
src/python/pants/engine/legacy/address_mapper.py:      return address.rel_path in file_paths
src/python/pants/backend/project_info/tasks/filedeps.py:      files.add(self._full_path(target.address.rel_path))
contrib/go/src/python/pants/contrib/go/targets/go_remote_library.py:      raise TargetDefinitionException(Address(parse_context.rel_path, kwargs['name']),
contrib/go/src/python/pants/contrib/go/targets/go_remote_library.py:      raise TargetDefinitionException(Address(parse_context.rel_path, name),
contrib/go/src/python/pants/contrib/go/targets/go_local_source.py:      raise TargetDefinitionException(Address(parse_context.rel_path, kwargs['name']).spec,
contrib/go/src/python/pants/contrib/go/targets/go_local_source.py:      raise TargetDefinitionException(Address(parse_context.rel_path, name).spec,
@stuhood stuhood mentioned this issue Nov 1, 2018
Closed
@stuhood stuhood mentioned this issue Apr 10, 2019
Merged
@stuhood stuhood removed the P3 - M4 label May 2, 2019
@stuhood stuhood mentioned this issue Oct 7, 2019
Merged
@stuhood stuhood mentioned this issue Feb 6, 2020
Merged
@Eric-Arellano
Copy link
Contributor

To clarify, what is the alternative? Should we be using Address everywhere instead of BuildFileAddress?

@stuhood
Copy link
Member Author

stuhood commented Feb 6, 2020

Yea, I think so. And then the (very rare) cases that actually need to know precisely which BUILD file something came from can ask for an AddressFamily directly.

Eric-Arellano added a commit that referenced this issue Feb 6, 2020
@Eric-Arellano
Rename ProvenancedBuildFileAddress to AddressWithOrigin (#9078)
e085d45 
Several `@goal_rule`s will soon start requesting `ProvenancedBuildFileAddresses`, rather than `BuildFileAddresses`, so that they may preserve the original `FilesystemSpec` to be more precise in what files they run over. (For example, `./pants fmt foo.py` only formatting `foo.py` rather than its whole owning target.). So, this type is going to start showing up in a lot of places.

We rename `ProvenancedBuildFileAddress` to `AddressWithOrigin` because:
1) Generally, we want to stop using `BuildFileAddress` per #6657. So, the name should not mention `BuildFile`.
2) While `provenance` is a valid technical term, it is unfamiliar for even many native English speakers (including myself - I had to look it up). Generally, we should avoid using inaccessible words when there is a better alternative. In contrast, `origin` is familiar to most English speakers.
3) Conceptually, this type refers to an `Address` that is _augmented with_ additional information from where that `Address` first comes from. `AddressWithOrigin` makes clear that the `WithOrigin` part is some extra information being preserved.
@Eric-Arellano Eric-Arellano mentioned this issue Feb 6, 2020
Merged
Eric-Arellano added a commit that referenced this issue Feb 7, 2020
@Eric-Arellano
Use Addresses instead of BuildFileAddresses in most rules (#9083)
1af739d 
Per #6657, there is little reason for `BuildFileAddress` to exist anymore. It is a clunky API, such as being verbose and subclassing `Address`.

This allows us to start using `Addresses` instead of `BuildFileAddresses` in most goals. There are still some awkward parts, like `HydratedTarget.address` being a `BuildFileAddress`, but those will be cleaned up in followup PRs.
@Eric-Arellano Eric-Arellano mentioned this issue Feb 11, 2020
Merged
Eric-Arellano added a commit that referenced this issue Feb 11, 2020
@Eric-Arellano
Replace BuildFileAddress with Address for HydratedTarget and `T…
aa3126b 
…argetAdaptor` (#9100)

Closes #6657. Now, the engine always uses `Address` by default, unless a rule author explicitly calls `await Get[BuildFileAddress](Address)`.

To keep things working with V1, we introduce `LegacyHydratedTarget` so that the V1 code still uses `BuildFileAddress`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Replace BuildFileAddress with Address for HydratedTarget and TargetAdaptor Eric-Arellano/pants
2 participants
@stuhood @Eric-Arellano