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

go: analyze package metadata #12429

Merged
merged 14 commits into from
Jul 28, 2021
Merged

Conversation

tdyas
Copy link
Contributor

@tdyas tdyas commented Jul 27, 2021

Analyze package metadata and report it via ResolvedGoPackage type. This provides the data that will be used for dependency inference and builds of packages.

Also refactors the code that finds the owning go_module into its own rules via FindNearestGoModuleRequest.

Tom Dyas added 3 commits July 26, 2021 21:50
[ci skip-rust]

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

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

[ci skip-build-wheels]
@tdyas

This comment has been minimized.

module_address: Optional[Address]
package_name: str
imported_import_paths: Tuple[str]
dependency_import_paths: Tuple[str]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This field is imported paths plus what appears to be transitive dependencies.



@rule
async def find_nearest_go_module(request: FindOwningGoModuleRequest) -> ResolvedOwningGoModule:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was refactored out of the target_type_rules to allow reuse. Probably can be a separate PR.

@tdyas
Copy link
Contributor Author

tdyas commented Jul 27, 2021

Am I missing an obvious mistake in my code? Can class variables on target types "leak" onto sibling or ancestor fields?

The exception is being thrown from GoModuleSources. I was requesting the wrong field type. 🤦

Comment on lines +194 to +195
# Obtain unexpanded targets and ensure file targets are filtered out. Unlike Python, file targets do not
# make sense semantically for Go source since Go builds entire packages at a time. The filtering is
Copy link
Member

Choose a reason for hiding this comment

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

Just a note: this is somewhat true of Java as well: the entire JVM package is implicitly available without an import statement, but separate compilation is still supported.

I expect that @patricklaw will have the inference implementation for the JVM infer a dependency on all files in the package (which will cause it to automatically be coarsened to a single CoarsenedTarget) rather than taking the approach you're taking here. Part of the reason for that is that the JVM allows import cycles between files, and so he already needs to be able to support cycles and thus coarsening.

The equivalent case for go would be if multiple packages had cyclic imports for one another: but it looks like that isn't possible: https://jogendra.dev/import-cycles-in-golang-and-how-to-deal-with-them

So yea: this seems like the way to go for go.

Tom Dyas added 6 commits July 27, 2021 15:58
[ci skip-rust]

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

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

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

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

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

[ci skip-build-wheels]
@tdyas tdyas requested a review from stuhood July 27, 2021 21:06
@tdyas tdyas marked this pull request as ready for review July 27, 2021 21:06
@tdyas tdyas requested review from Eric-Arellano and benjyw July 27, 2021 21:06
Copy link
Contributor

@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.

Great!

address: Address


def error_to_string(d: dict) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

Should this be dict[str, str], or is this trickier than that?

Tom Dyas added 5 commits July 28, 2021 11:44
[ci skip-rust]

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

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

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

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

[ci skip-build-wheels]
@tdyas tdyas merged commit 75a5421 into pantsbuild:main Jul 28, 2021
@tdyas tdyas deleted the golang_resolve_package_metadata branch July 28, 2021 16:36
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.

3 participants