Draft: Go modules ignore conditions using go list -m -versions
#3630
+36
−20
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Instead of using our custom Go-based updatechecker (go_modules/helpers/updatechecker/main.go), let's rely on the built-in Go tooling to identify the latest resolvable version of a dependency (i.e., `go list -m --versions <module>`). This also signals a move to performing all of our Dependabot-specific logic in Ruby (which is the norm for most ecosystems), as opposed to splitting it between Ruby and Go. Co-authored-by: David McIntosh <mctofu@github.com>
feelepxyz
reviewed
May 4, 2021
Comment on lines
+70
to
+75
| json, stderr, status = Open3.capture3(env, SharedHelpers.escape_command("go list -m -mod=mod -versions --json #{dependency.name}")) | ||
| if !status.success? | ||
|
|
||
| # TODO: Should we populate an error_context here? | ||
| handle_subprocess_error(SharedHelpers::HelperSubprocessFailed.new(message: stderr, error_context: {})) | ||
| end |
There was a problem hiding this comment.
Suggested change
| json, stderr, status = Open3.capture3(env, SharedHelpers.escape_command("go list -m -mod=mod -versions --json #{dependency.name}")) | |
| if !status.success? | |
| # TODO: Should we populate an error_context here? | |
| handle_subprocess_error(SharedHelpers::HelperSubprocessFailed.new(message: stderr, error_context: {})) | |
| end | |
| json = SharedHelpers.run_shell_command("go list -m -mod=mod -versions --json #{dependency.name}") |
|
Closing in favor of #3631. We'd still like to make use of |
To extend this: both approaches download the target module into the cache. I setup the following sandbox: When I wipe the package cache and invoke the helper, the dependency is cloned: The difference is that module listing fetches the entire module graph (e.g. including transitive), not just the module being queried: Yikes! Also worth a shout, Now that's where it's at - the query time shouldn't scale with the target repository's size! Aside: in the experiment where |
|
@thepwagner Thanks for that insight! I think it's downloading all the transitive deps to create the |
|
Thanks for the deep analysis and reporting the result publicly so that drive-by contributors like me don't try to re-invent the wheel. Note that the transitive deps download may go away (or be greatly reduced in scope) when golang/go#36460 lands. Given the really nice benefits listed above re: gaining retraction, major version support, etc this is probably worth re-investigating once |
|
Now that go 1.17 is out we should give this another shot. Avoiding the need to maintain our own copy of the internal go modules libs would be a great benefit. See #4157. |
|
go 1.17 didn't end up helping but I was able to make progress with an empty |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is an initial exploration of moving go_module latest version selection out of the
updatecheckerhelper and into the ruby code. The goal is to support the upcomingupdate_typeignore conditions.Instead of modifying the
updatecheckerhelper we started down the route of usinggo list -m -versions --json <module>to fetch the list of available versions and then added any extra filtering that was needed to make the tests pass.This appeared to work pretty well and had some benefits:
updatecheckercodeHowever, we hit one snag. While pairing with @jasonrudolph we discovered that his dev shell was out of date and using go 1.15. When I tried this under 1.16 the
go listcommand started to report this error:A workaround I came up with was to change the command to
go list -m -mod=mod -versions --json github.com/dependabot-fixtures/go-modules-lib. This works but takes much more time as it ends up downloading the module into the module cache. Forupdate_checker_spec.rbthis increased the runtime from ~2s to ~10s when run with an empty module cache. In practice this change would mean we need to download every dependency we check for updates instead of only downloading dependencies that need to be updated.Because of the potential performance impact I'm not feeling good about rolling out the
go listchange as part of supporting ignore conditions but wanted to put this out for feedback in case I missed something. I'm also going to put up a draft of an alternate approach which modifies theupdatecheckerhelper to behave more likego listinstead of switching togo list. That could make it easier to swap out the implementation in the future.Alternate: #3631