[internal] go: rewrite third party package analysis to not use `go list` #14157

tdyas · 2022-01-12T23:13:50Z

Problem

The existing Go backend uses go list to analyze third party modules. Several users encountered a problem where the call to go list was demanding changes to go.mod with this error message:

go: updates to go.mod needed; to update it:
	go mod tidy

Running go mod tidy, however, made no changes to the go.mod, so there was no way to ever satisfy go list.

Solution

Solve the issue by rewriting the Go backend to use the same custom package analyzer used for first-party package analysis. This removes the reliance on go list for package analysis which was the go invocation that was generating the error message. (go list is still used to compute what modules are dependencies.)

ThirdPartyPkgInfo is renamed to ThirdPartyPkgAnalysis to better align with FirstPartyPkgAnalysis.

[ci skip-rust]

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

Eric-Arellano

Thank you for diving into this!

src/python/pants/backend/go/util_rules/third_party_pkg_test.py

src/python/pants/backend/go/util_rules/third_party_pkg.py

src/python/pants/backend/go/util_rules/first_party_pkg_test.py

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

[ci skip-rust]

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

Eric-Arellano

Nice, this looks really great. Thanks so much for fixing this!

Would you be willing to please add comments to both of the downloads=True processes explaining what's going on re: downloads, like that only go.mod is being downloaded? You explained this really well in the PR comment.
Could you please benchmark ./pants --no-local-cache --no-pantsd list ::?

src/python/pants/backend/go/goals/check_test.py

src/python/pants/backend/go/target_type_rules_test.py

src/python/pants/backend/go/util_rules/build_pkg_test.py

src/python/pants/backend/go/util_rules/third_party_pkg_test.py

src/python/pants/backend/go/util_rules/third_party_pkg.py

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

tdyas · 2022-01-14T20:29:41Z

1. Would you be willing to please add comments to both of the `downloads=True` processes explaining what's going on re: downloads, like that only `go.mod` is being downloaded? You explained this really well in the PR comment.

Will do.

2. Could you please benchmark `./pants --no-local-cache --no-pantsd list ::`?

Against pants repo or a entirely Go repo like remote-api-tools?

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

Eric-Arellano · 2022-01-14T20:31:38Z

Against pants repo or a entirely Go repo like remote-api-tools?

Go repo. remote-api-tools sounds good because it's bigger than example-golang.

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

tdyas · 2022-01-14T20:42:08Z

Against pants repo or a entirely Go repo like remote-api-tools?

Go repo. remote-api-tools sounds good because it's bigger than example-golang.

I'm on slow WiFi so the benchmark is dominated by network download time.

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

Internal changes: * upgrade to Rust v1.58.0 ([pantsbuild#14174](pantsbuild#14174)) * [internal] fix typos in codegen registration ([pantsbuild#14172](pantsbuild#14172)) * Remove per-language indirection for formatter plugins. ([pantsbuild#14166](pantsbuild#14166)) * Pulls `Coordinate` and `ArtifactRequirements` out into a separate file to avoid a circuilar import later ([pantsbuild#14164](pantsbuild#14164)) * Factors lockfile metadata code into class that supports multiple languages as well as versions ([pantsbuild#14116](pantsbuild#14116)) * Adds `.env` file to make vscode auto-imports work correctly out of the box ([pantsbuild#14130](pantsbuild#14130)) * [internal] Rename classes for `generate_lockfiles.py` for clarity ([pantsbuild#14218](pantsbuild#14218)) * [internal] Fix bad Find+Replace ([pantsbuild#14213](pantsbuild#14213)) * [internal] Bump libc from 0.2.106 to 0.2.112 in /src/rust/engine ([pantsbuild#14206](pantsbuild#14206)) * [internal] Bump tempfile from 3.2.0 to 3.3.0 in /src/rust/engine ([pantsbuild#14202](pantsbuild#14202)) * [internal] Bump criterion from 0.3.3 to 0.3.5 in /src/rust/engine ([pantsbuild#14205](pantsbuild#14205)) * [internal] Bump walkdir from 2.3.1 to 2.3.2 in /src/rust/engine ([pantsbuild#14204](pantsbuild#14204)) * [internal] Bump generic-array from 0.14.4 to 0.14.5 in /src/rust/engine ([pantsbuild#14203](pantsbuild#14203)) * [internal] Remove the minimum bucket size of batching to improve stability. ([pantsbuild#14210](pantsbuild#14210)) * [internal] Make `JvmLockfileRequest` generic ([pantsbuild#14201](pantsbuild#14201)) * [internal] add comment re clippy issue ([pantsbuild#14188](pantsbuild#14188)) * [internal] rename some codegen scopes to put language first ([pantsbuild#14187](pantsbuild#14187)) * [internal] Check for ambiguity when running `generate-lockfiles` ([pantsbuild#14178](pantsbuild#14178)) * [internal] Change JVM lockfile format ([pantsbuild#14175](pantsbuild#14175)) * [internal] go: rewrite third party package analysis to not use `go list` ([pantsbuild#14157](pantsbuild#14157)) * [internal] Hotfix `fmt` crashing on non-formattable targets ([pantsbuild#14168](pantsbuild#14168)) * [internal] Simplify `core/goals/package.py` filesystem API usage ([pantsbuild#14162](pantsbuild#14162)) * [internal] java/thrift: register union that was not registered ([pantsbuild#14159](pantsbuild#14159)) * [internal] Bump arc-swap from 1.3.0 to 1.5.0 in /src/rust/engine ([pantsbuild#14148](pantsbuild#14148)) * [internals] Bump tokio-rustls from 0.22.0 to 0.23.2 in /src/rust/engine ([pantsbuild#14149](pantsbuild#14149)) * [internal] Bump num_cpus from 1.13.0 to 1.13.1 in /src/rust/engine ([pantsbuild#14150](pantsbuild#14150)) * [internal] Bump tokio-util from 0.6.7 to 0.6.9 in /src/rust/engine ([pantsbuild#14151](pantsbuild#14151)) * [internal] Bump os_pipe from 0.9.2 to 1.0.0 in /src/rust/engine ([pantsbuild#14152](pantsbuild#14152)) * [internal] Introduce new `BuiltinGoal` subsystem type. ([pantsbuild#13991](pantsbuild#13991)) * [internal] Upgrade python type stubs packages ([pantsbuild#14143](pantsbuild#14143)) * [internal] Bump strum_macros from 0.20.1 to 0.23.1 in /src/rust/engine ([pantsbuild#14141](pantsbuild#14141)) * [internal] Make `generate-lockfiles` goal generic ([pantsbuild#14122](pantsbuild#14122)) * [internal] Bump errno from 0.2.7 to 0.2.8 in /src/rust/engine ([pantsbuild#14137](pantsbuild#14137)) * [internal] Bump colored from 1.9.3 to 2.0.0 in /src/rust/engine ([pantsbuild#14138](pantsbuild#14138)) * [internal] Bump crossbeam-channel from 0.4.4 to 0.5.0 in /src/rust/engine ([pantsbuild#14139](pantsbuild#14139)) * [internal] Bump reqwest from 0.11.4 to 0.11.9 in /src/rust/engine ([pantsbuild#14135](pantsbuild#14135)) * [internal] Further tweak dependabot config ([pantsbuild#14132](pantsbuild#14132)) * [internal] python: use immutable_input_digests for protobuf codegen ([pantsbuild#13885](pantsbuild#13885)) * [internal] python: use immutable_input_digests for protobuf codegen ([pantsbuild#13885](pantsbuild#13885)) * [internal] No need for two f-strings/two strings. ([pantsbuild#14119](pantsbuild#14119))

Internal changes: * upgrade to Rust v1.58.0 ([pantsbuild#14174](pantsbuild#14174)) * [internal] fix typos in codegen registration ([pantsbuild#14172](pantsbuild#14172)) * Pulls `Coordinate` and `ArtifactRequirements` out into a separate file to avoid a circuilar import later ([pantsbuild#14164](pantsbuild#14164)) * Factors lockfile metadata code into class that supports multiple languages as well as versions ([pantsbuild#14116](pantsbuild#14116)) * Adds `.env` file to make vscode auto-imports work correctly out of the box ([pantsbuild#14130](pantsbuild#14130)) * [internal] Rename classes for `generate_lockfiles.py` for clarity ([pantsbuild#14218](pantsbuild#14218)) * [internal] Fix bad Find+Replace ([pantsbuild#14213](pantsbuild#14213)) * [internal] Bump libc from 0.2.106 to 0.2.112 in /src/rust/engine ([pantsbuild#14206](pantsbuild#14206)) * [internal] Bump tempfile from 3.2.0 to 3.3.0 in /src/rust/engine ([pantsbuild#14202](pantsbuild#14202)) * [internal] Bump criterion from 0.3.3 to 0.3.5 in /src/rust/engine ([pantsbuild#14205](pantsbuild#14205)) * [internal] Bump walkdir from 2.3.1 to 2.3.2 in /src/rust/engine ([pantsbuild#14204](pantsbuild#14204)) * [internal] Bump generic-array from 0.14.4 to 0.14.5 in /src/rust/engine ([pantsbuild#14203](pantsbuild#14203)) * [internal] Remove the minimum bucket size of batching to improve stability. ([pantsbuild#14210](pantsbuild#14210)) * [internal] Make `JvmLockfileRequest` generic ([pantsbuild#14201](pantsbuild#14201)) * [internal] add comment re clippy issue ([pantsbuild#14188](pantsbuild#14188)) * [internal] rename some codegen scopes to put language first ([pantsbuild#14187](pantsbuild#14187)) * [internal] Check for ambiguity when running `generate-lockfiles` ([pantsbuild#14178](pantsbuild#14178)) * [internal] Change JVM lockfile format ([pantsbuild#14175](pantsbuild#14175)) * [internal] go: rewrite third party package analysis to not use `go list` ([pantsbuild#14157](pantsbuild#14157)) * [internal] Hotfix `fmt` crashing on non-formattable targets ([pantsbuild#14168](pantsbuild#14168)) * [internal] Simplify `core/goals/package.py` filesystem API usage ([pantsbuild#14162](pantsbuild#14162)) * [internal] java/thrift: register union that was not registered ([pantsbuild#14159](pantsbuild#14159)) * [internal] Bump arc-swap from 1.3.0 to 1.5.0 in /src/rust/engine ([pantsbuild#14148](pantsbuild#14148)) * [internals] Bump tokio-rustls from 0.22.0 to 0.23.2 in /src/rust/engine ([pantsbuild#14149](pantsbuild#14149)) * [internal] Bump num_cpus from 1.13.0 to 1.13.1 in /src/rust/engine ([pantsbuild#14150](pantsbuild#14150)) * [internal] Bump tokio-util from 0.6.7 to 0.6.9 in /src/rust/engine ([pantsbuild#14151](pantsbuild#14151)) * [internal] Bump os_pipe from 0.9.2 to 1.0.0 in /src/rust/engine ([pantsbuild#14152](pantsbuild#14152)) * [internal] Introduce new `BuiltinGoal` subsystem type. ([pantsbuild#13991](pantsbuild#13991)) * [internal] Upgrade python type stubs packages ([pantsbuild#14143](pantsbuild#14143)) * [internal] Bump strum_macros from 0.20.1 to 0.23.1 in /src/rust/engine ([pantsbuild#14141](pantsbuild#14141)) * [internal] Make `generate-lockfiles` goal generic ([pantsbuild#14122](pantsbuild#14122)) * [internal] Bump errno from 0.2.7 to 0.2.8 in /src/rust/engine ([pantsbuild#14137](pantsbuild#14137)) * [internal] Bump colored from 1.9.3 to 2.0.0 in /src/rust/engine ([pantsbuild#14138](pantsbuild#14138)) * [internal] Bump crossbeam-channel from 0.4.4 to 0.5.0 in /src/rust/engine ([pantsbuild#14139](pantsbuild#14139)) * [internal] Bump reqwest from 0.11.4 to 0.11.9 in /src/rust/engine ([pantsbuild#14135](pantsbuild#14135)) * [internal] Further tweak dependabot config ([pantsbuild#14132](pantsbuild#14132)) * [internal] python: use immutable_input_digests for protobuf codegen ([pantsbuild#13885](pantsbuild#13885)) * [internal] python: use immutable_input_digests for protobuf codegen ([pantsbuild#13885](pantsbuild#13885)) * [internal] No need for two f-strings/two strings. ([pantsbuild#14119](pantsbuild#14119)) * `LockfileMetadata` - Replace flaky `_header_dict()` mechanism with more robust `header_attrs` mechanism ([pantsbuild#14229](pantsbuild#14229))

Internal changes: * upgrade to Rust v1.58.0 ([#14174](#14174)) * [internal] fix typos in codegen registration ([#14172](#14172)) * Pulls `Coordinate` and `ArtifactRequirements` out into a separate file to avoid a circuilar import later ([#14164](#14164)) * Factors lockfile metadata code into class that supports multiple languages as well as versions ([#14116](#14116)) * Adds `.env` file to make vscode auto-imports work correctly out of the box ([#14130](#14130)) * [internal] Rename classes for `generate_lockfiles.py` for clarity ([#14218](#14218)) * [internal] Fix bad Find+Replace ([#14213](#14213)) * [internal] Bump libc from 0.2.106 to 0.2.112 in /src/rust/engine ([#14206](#14206)) * [internal] Bump tempfile from 3.2.0 to 3.3.0 in /src/rust/engine ([#14202](#14202)) * [internal] Bump criterion from 0.3.3 to 0.3.5 in /src/rust/engine ([#14205](#14205)) * [internal] Bump walkdir from 2.3.1 to 2.3.2 in /src/rust/engine ([#14204](#14204)) * [internal] Bump generic-array from 0.14.4 to 0.14.5 in /src/rust/engine ([#14203](#14203)) * [internal] Remove the minimum bucket size of batching to improve stability. ([#14210](#14210)) * [internal] Make `JvmLockfileRequest` generic ([#14201](#14201)) * [internal] add comment re clippy issue ([#14188](#14188)) * [internal] rename some codegen scopes to put language first ([#14187](#14187)) * [internal] Check for ambiguity when running `generate-lockfiles` ([#14178](#14178)) * [internal] Change JVM lockfile format ([#14175](#14175)) * [internal] go: rewrite third party package analysis to not use `go list` ([#14157](#14157)) * [internal] Hotfix `fmt` crashing on non-formattable targets ([#14168](#14168)) * [internal] Simplify `core/goals/package.py` filesystem API usage ([#14162](#14162)) * [internal] java/thrift: register union that was not registered ([#14159](#14159)) * [internal] Bump arc-swap from 1.3.0 to 1.5.0 in /src/rust/engine ([#14148](#14148)) * [internals] Bump tokio-rustls from 0.22.0 to 0.23.2 in /src/rust/engine ([#14149](#14149)) * [internal] Bump num_cpus from 1.13.0 to 1.13.1 in /src/rust/engine ([#14150](#14150)) * [internal] Bump tokio-util from 0.6.7 to 0.6.9 in /src/rust/engine ([#14151](#14151)) * [internal] Bump os_pipe from 0.9.2 to 1.0.0 in /src/rust/engine ([#14152](#14152)) * [internal] Introduce new `BuiltinGoal` subsystem type. ([#13991](#13991)) * [internal] Upgrade python type stubs packages ([#14143](#14143)) * [internal] Bump strum_macros from 0.20.1 to 0.23.1 in /src/rust/engine ([#14141](#14141)) * [internal] Make `generate-lockfiles` goal generic ([#14122](#14122)) * [internal] Bump errno from 0.2.7 to 0.2.8 in /src/rust/engine ([#14137](#14137)) * [internal] Bump colored from 1.9.3 to 2.0.0 in /src/rust/engine ([#14138](#14138)) * [internal] Bump crossbeam-channel from 0.4.4 to 0.5.0 in /src/rust/engine ([#14139](#14139)) * [internal] Bump reqwest from 0.11.4 to 0.11.9 in /src/rust/engine ([#14135](#14135)) * [internal] Further tweak dependabot config ([#14132](#14132)) * [internal] python: use immutable_input_digests for protobuf codegen ([#13885](#13885)) * [internal] python: use immutable_input_digests for protobuf codegen ([#13885](#13885)) * [internal] No need for two f-strings/two strings. ([#14119](#14119)) * `LockfileMetadata` - Replace flaky `_header_dict()` mechanism with more robust `header_attrs` mechanism ([#14229](#14229))

rewrite third party dep analysis to not use go list

d4f73c6

[ci skip-rust]

tdyas requested review from stuhood and Eric-Arellano January 12, 2022 23:13

tdyas changed the title ~~[internal go: rewrite third party package analysis to not use go list~~ [internal] go: rewrite third party package analysis to not use go list Jan 12, 2022

rename ThirdPartyPkgInfoRequest -> ThirdPartyPkgAnalysisRequest

5280730

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

Eric-Arellano reviewed Jan 13, 2022

View reviewed changes

Tom Dyas added 4 commits January 14, 2022 13:48

put back test

25219ce

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

avoid incorrect use of posixpath

aa5684f

[ci skip-rust]

pass downloaded go.mod's to download step

4f2f61b

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

avoid spamming pkg_analyzer.rules() all over the place

8e63928

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

tdyas requested a review from Eric-Arellano January 14, 2022 15:19

Eric-Arellano approved these changes Jan 14, 2022

View reviewed changes

add comments explaining allow_downloads=True

d3589f6

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

Tom Dyas added 2 commits January 14, 2022 21:30

cleanup more pkg_analyzer strays

15ce45d

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

remove stray print

db7172a

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

remove unused (yet) sum field

d145eca

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

fix working_dir

606f693

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

tdyas merged commit d28b781 into pantsbuild:main Jan 14, 2022

tdyas deleted the golang_rewrite_third_party_dep_analysis branch January 14, 2022 22:14

Eric-Arellano mentioned this pull request Jan 19, 2022

Go: consider redesigning third-party metadata to only be what's used #14071

Closed

illicitonion mentioned this pull request Jan 22, 2022

Prep for 2.10.0.dev2 #14236

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[internal] go: rewrite third party package analysis to not use `go list` #14157

[internal] go: rewrite third party package analysis to not use `go list` #14157

tdyas commented Jan 12, 2022 •

edited

Loading

Eric-Arellano left a comment

Eric-Arellano left a comment

tdyas commented Jan 14, 2022

Eric-Arellano commented Jan 14, 2022

tdyas commented Jan 14, 2022

[internal] go: rewrite third party package analysis to not use go list #14157

[internal] go: rewrite third party package analysis to not use go list #14157

Conversation

tdyas commented Jan 12, 2022 • edited Loading

Problem

Solution

Eric-Arellano left a comment

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

tdyas commented Jan 14, 2022

Eric-Arellano commented Jan 14, 2022

tdyas commented Jan 14, 2022

[internal] go: rewrite third party package analysis to not use `go list` #14157

[internal] go: rewrite third party package analysis to not use `go list` #14157

tdyas commented Jan 12, 2022 •

edited

Loading