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

[internal] Generate _go_internal_package targets #13139

Merged
merged 1 commit into from
Oct 6, 2021

Conversation

Eric-Arellano
Copy link
Contributor

@Eric-Arellano Eric-Arellano commented Oct 6, 2021

Closes #13049. This greatly reduces boilerplate and also allows us to make some fields required like import_path and subpath, so that we don't have to calculate those in consuming rules like build_go_pkg.py.

The address format

The generated address looks like project#./dir. @tdyas offered that this is intuitive for Go developers because they have to say go build ./dir already with the leading ./.

This solves how to represent when the _go_internal_package is in the same dir as the go_mod: project#./.

This also makes very clear the difference from external packages like project#rsc.io/quote vs. internal packages like project#./dir.

Improves dependency inference

Now that the import_path field is required for both _go_internal_package and _go_external_package, we can create a global mapping of import_path -> pkg_target. This is necessary for #13114.

This also improves performance. We don't need to call ResolvedGoPackage on all the candidate targets a package might depend on to calculate their import paths. We do still need it when resolving the deps of a particular _go_internal_package, but we can be lazier when we call that codepath in not evaluating all candidate targets.

dependencies benchmark

As expected, there is no difference because we are finding the dependencies of everything, so we still have to call ResolvedGoPackage. The perf gains are only in things sometimes being less eager, which isn't the case here.

Before:

❯ hyperfine -r 5 './pants_from_sources --no-pantsd --no-process-execution-local-cache dependencies ::'
  Time (mean ± σ):     26.501 s ±  1.537 s    [User: 29.554 s, System: 24.115 s]
  Range (min … max):   24.928 s … 28.763 s    5 runs

After:

❯ hyperfine -r 5 './pants_from_sources --no-pantsd --no-process-execution-local-cache dependencies ::'
  Time (mean ± σ):     26.359 s ±  0.526 s    [User: 29.600 s, System: 23.769 s]
  Range (min … max):   25.625 s … 26.993 s    5 runs

package benchmark

Before:

❯ hyperfine -r 5 './pants_from_sources --no-pantsd --no-process-execution-local-cache package ::'
  Time (mean ± σ):     33.777 s ±  0.248 s    [User: 39.221 s, System: 39.389 s]
  Range (min … max):   33.517 s … 34.062 s    5 runs

After:

❯ hyperfine -r 5 './pants_from_sources --no-pantsd --no-process-execution-local-cache package ::'
Benchmark #1: ./pants_from_sources --no-pantsd --no-process-execution-local-cache package ::
  Time (mean ± σ):     31.049 s ±  0.702 s    [User: 40.606 s, System: 40.537 s]
  Range (min … max):   30.512 s … 32.273 s    5 runs

TODO: fix go_binary inference of main field

#13117 added inference of the main field for go_binary, that it defaults to the go_package defined in that directory.

But target generation no longer generates targets actually in each directory. All generated targets are "located" in the BUILD file of the go_mod, i.e. their spec_path is set to that. So it no longer looks to AddressSpecs like there are any targets in each subdirectory, and there are >1 _go_internal_package targets in the go_mod dir.

Instead, we should use the subpath field to determine what directory the targets correspond to.

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

# 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]
"generates `_go_external_package` targets based on the `require` directives in your "
"`go.mod`.\n\n"
"If you have external packages, make sure you have an up-to-date `go.sum`. Run "
"`go mod tidy` directly to update your `go.mod` and `go.sum`."
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI This may need to say go mod download all. See #13136. But fix in a separate PR in any event.

@Eric-Arellano
Copy link
Contributor Author

Performance benchmarks posted. A 2 second speedup for ./pants package ::. I'll take it!

@Eric-Arellano Eric-Arellano enabled auto-merge (squash) October 6, 2021 18:48
@Eric-Arellano Eric-Arellano merged commit 0044d1d into pantsbuild:main Oct 6, 2021
@Eric-Arellano Eric-Arellano deleted the gen-go-pkg-tgt branch October 6, 2021 23:53
Eric-Arellano added a commit that referenced this pull request Oct 8, 2021
…rated targets (#13163)

Restores support for #13117 post-#13139.

[ci skip-rust]
@stuhood stuhood mentioned this pull request Oct 11, 2021
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.

Go: improve modeling of go_package via target generation
2 participants