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.

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

Replace BuildFileAddress with Address for HydratedTarget and TargetAdaptor #9100

Merged
merged 6 commits into from
Feb 11, 2020

Conversation

Eric-Arellano
Copy link
Contributor

@Eric-Arellano Eric-Arellano commented Feb 11, 2020

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.

This fixes some failing tests and means we can delete the type! We keep BuildFileAddress because there are still some uses for it, such as V1.
Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

This also completely removes BuildFileAddresses, as it's no longer needed.

This part of the description needs a refresh I think.

@@ -93,6 +93,12 @@ def _raise_did_you_mean(address_family: AddressFamily, name: str, source=None) -
)


@rule
async def find_build_files(addresses: Addresses) -> BuildFileAddresses:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know which codepath this will still be used in? Will probably want to confirm that we don't see a performance impact.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's used solely for LegacyAddressMapper, which is used by filter I think?

@@ -519,6 +544,29 @@ def owns_any_source(legacy_target: HydratedTarget) -> bool:
return TransitiveHydratedTargets(tuple(tht.root for tht in transitive_hydrated_targets), closure)


@rule
async def legacy_transitive_hydrated_targets(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should make sure we don't see a performance impact on legacy goals for this... things like dependencies, maybe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can we do that? Could we possibly land this for the dev release and use Twitter's performance suite to check if there were any regressions in the dev release?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love to see an upstream performance suite, personally...........

Copy link
Contributor

@cosmicexplorer cosmicexplorer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

To keep things working with V1, we must introduce LegacyHydratedTarget so that the V1 code still uses BuildFileAddress.

Great approach!

@@ -519,6 +544,29 @@ def owns_any_source(legacy_target: HydratedTarget) -> bool:
return TransitiveHydratedTargets(tuple(tht.root for tht in transitive_hydrated_targets), closure)


@rule
async def legacy_transitive_hydrated_targets(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love to see an upstream performance suite, personally...........

Copy link
Contributor

@codealchemy codealchemy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@Eric-Arellano Eric-Arellano merged commit aa3126b into pantsbuild:master Feb 11, 2020
@Eric-Arellano Eric-Arellano deleted the no-more-bfa branch February 11, 2020 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(Attempt to) Deprecate/Remove BuildFileAddress
4 participants