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

Require setting description_of_origin with AddressInput and UnparsedAddressInputs #15743

Conversation

Eric-Arellano
Copy link
Contributor

This will allow us to enrich the error messages for #14468, which will be done in a followup.

Some of the callers are not yet implemented, but that's fine because this doesn't update error messages yet except for two cases that should not happen in the wild.

[ci skip-rust]
[ci skip-build-wheels]

…rsedAddressInputs`

[ci skip-rust]

[ci skip-build-wheels]
Copy link
Contributor Author

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

I split this up into a dedicated PR so that
a) we have a dedicated changelog entry for plugin change
b) we can bikeshed the new error messages more easily

@@ -33,6 +33,7 @@ async def inject_docker_dependencies(
UnparsedAddressInputs(
(v for v in dockerfile_info.from_image_build_args.to_dict().values() if v),
owning_address=dockerfile_info.address,
description_of_origin="TODO(#14468)",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can try to figure this out, although @kaos if you have a chance, it would help.

@@ -283,6 +283,7 @@ async def create_docker_build_context(
UnparsedAddressInputs(
dockerfile_build_args.values(),
owning_address=dockerfile_info.address,
description_of_origin="TODO(#14468)",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can try to figure this out, although @kaos if you have a chance, it would help.

UnparsedAddressInputs(
target_refs,
owning_address=address,
description_of_origin=f"TODO(#14468)",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can figure this one out after lunch. The tricky part is it could be from provides field or entry_points field.

Targets, UnparsedAddressInputs([request.pants_address], owning_address=None)
Targets,
UnparsedAddressInputs(
[request.pants_address], owning_address=None, description_of_origin="TODO(#14468)"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Help appreciated @stuhood @tdyas @chrisjrn (possibly in followup PR)

@@ -46,7 +46,7 @@ def address_input(self) -> AddressInput:
f"Unknown URI scheme for BSP BuildTargetIdentifier. Expected scheme `pants`, but URI was: {self.uri}"
)
raw_addr = self.uri[len("pants:") :]
return AddressInput.parse(raw_addr)
return AddressInput.parse(raw_addr, description_of_origin="TODO(#14468)")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Help appreciated @stuhood @tdyas (possibly in a followup). Tom suggested over DM that we refer to the config file the address comes from, although that information is not stored on this type.

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@Eric-Arellano Eric-Arellano enabled auto-merge (squash) June 3, 2022 19:40
@Eric-Arellano Eric-Arellano merged commit 8b91de4 into pantsbuild:main Jun 3, 2022
@Eric-Arellano Eric-Arellano deleted the address-input-description-of-origin branch June 3, 2022 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants