-
-
Notifications
You must be signed in to change notification settings - Fork 631
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
[internal] Fix go_binary
's main
field inference to work with generated targets
#13163
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -134,7 +134,7 @@ class GoInternalPackageDependenciesField(Dependencies): | |
pass | ||
|
||
|
||
class GoInternalPackageSubpathField(StringField): | ||
class GoInternalPackageSubpathField(StringField, AsyncFieldMixin): | ||
alias = "subpath" | ||
help = ( | ||
"The path from the owning `go.mod` to this package's directory, e.g. `subdir`.\n\n" | ||
|
@@ -144,6 +144,19 @@ class GoInternalPackageSubpathField(StringField): | |
required = True | ||
value: str | ||
|
||
@property | ||
def full_dir_path(self) -> str: | ||
"""The full path to this package's directory, relative to the build root.""" | ||
# NB: Assumes that the `spec_path` points to the ancestor `go.mod`, which will be the | ||
# case when `go_mod` targets generate. | ||
if not self.address.is_generated_target: | ||
# TODO: Make this error more eager via target validation. | ||
raise AssertionError( | ||
f"Target was manually created, but expected to be generated: {self.address}" | ||
) | ||
Comment on lines
+152
to
+156
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tdyas I've been thinking a lot the past few days about whether to keep The downside is they won't show up in I think the answer is to make them public, but use eager validation that they are never explicitly created. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I share your concern with that downside. Letting users understand the magic even a little is probably beneficial, even if the targets should be auto-generated in the standard case. Should the first-party target then be called There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it should still be |
||
go_mod_path = self.address.spec_path | ||
return os.path.join(go_mod_path, self.value) | ||
|
||
|
||
class GoInternalPackageTarget(Target): | ||
alias = "_go_internal_package" | ||
|
@@ -203,9 +216,9 @@ class GoExternalPackageTarget(Target): | |
class GoBinaryMainPackageField(StringField, AsyncFieldMixin): | ||
alias = "main" | ||
help = ( | ||
"Address of the `go_package` with the `main` for this binary.\n\n" | ||
"If not specified, will default to the `go_package` in the same directory as this target's " | ||
"BUILD file." | ||
"Address of the `_go_internal_package` with the `main` for this binary.\n\n" | ||
"If not specified, will default to the `_go_internal_package` for the same " | ||
"directory as this target's BUILD file." | ||
) | ||
value: str | ||
|
||
|
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 this error explain to the user that
_go_internal_package
is a hidden target type that is auto-generated for them? Some of the other error messages in this PR do mention that fact.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.
Probably. Will audit all the error messages when renaming these targets.