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.

Sign up for GitHub

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

Switch to letting go get mutate go.mod #3590

Closed
wants to merge 16 commits into from
Closed

Switch to letting go get mutate go.mod #3590

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
9551b73
Switch to letting `go get` mutate `go.mod`
jeffwidman Apr 29, 2021
df0497a
drop comment that's out of scope for this PR
jeffwidman Apr 29, 2021
5ef6efb
pass multiple deps to go get
jeffwidman Apr 29, 2021
acce03d
Merge remote-tracking branch 'upstream/main' into switch-to-letting-g…
jeffwidman Apr 29, 2021
05cb135
use an unfrozen string
jeffwidman Apr 29, 2021
8cb409c
Fix two of the tests
jeffwidman Apr 29, 2021
a13eeee
Fix more tests
jeffwidman Apr 29, 2021
7c75b11
`go get -d <dep>@<version> no longer raises an error on rename
jeffwidman Apr 29, 2021
433a4f1
document the remaining two failures
jeffwidman Apr 30, 2021
8aff846
Merge remote-tracking branch 'upstream/main' into switch-to-letting-g…
jeffwidman May 6, 2021
478dc2c
Update go_modules/lib/dependabot/go_modules/file_updater/go_mod_updat…
jeffwidman May 17, 2021
475b919
Merge remote-tracking branch 'upstream/main' into switch-to-letting-g…
jeffwidman May 17, 2021
73cecb7
Remove no-longer applicable comment.
jeffwidman May 17, 2021
b466bb1
shorten lines to please the linter
jeffwidman May 18, 2021
4233836
hmm...
jeffwidman May 18, 2021
caaf083
Merge remote-tracking branch 'upstream/main' into switch-to-letting-g…
jeffwidman Aug 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions go_modules/helpers/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (

"github.com/dependabot/dependabot-core/go_modules/helpers/importresolver"
"github.com/dependabot/dependabot-core/go_modules/helpers/updatechecker"
"github.com/dependabot/dependabot-core/go_modules/helpers/updater"
)

type HelperParams struct {
Expand Down Expand Up @@ -37,10 +36,6 @@ func main() {
var args updatechecker.Args
parseArgs(helperParams.Args, &args)
funcOut, funcErr = updatechecker.GetVersions(&args)
case "updateDependencyFile":
var args updater.Args
parseArgs(helperParams.Args, &args)
funcOut, funcErr = updater.UpdateDependencyFile(&args)
case "getVcsRemoteForImport":
var args importresolver.Args
parseArgs(helperParams.Args, &args)
Expand Down
65 changes: 0 additions & 65 deletions go_modules/helpers/updater/helpers.go
View file
Open in desktop

This file was deleted.

50 changes: 0 additions & 50 deletions go_modules/helpers/updater/main.go
View file
Open in desktop

This file was deleted.

52 changes: 24 additions & 28 deletions go_modules/lib/dependabot/go_modules/file_updater/go_mod_updater.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ class GoModUpdater
RESOLVABILITY_ERROR_REGEXES = [
# The checksum in go.sum does not match the downloaded content
/verifying .*: checksum mismatch/.freeze,
/go: .*: go.mod has post-v\d+ module path/
/go (?:get)?: .*: go.mod has post-v\d+ module path/
].freeze

REPO_RESOLVABILITY_ERROR_REGEXES = [
Expand Down Expand Up @@ -91,16 +91,14 @@ def update_files # rubocop:disable Metrics/AbcSize, Metrics/PerceivedComplexity
# Replace full paths with path hashes in the go.mod
substitute_all(substitutions)

# Set the stubbed replace directives
update_go_mod(dependencies)

# Then run `go get` to pick up other changes to the file caused by
# the upgrade
run_go_get
# `go get` will generate the required go.mod/go.sum updates for the updated deps
run_go_get(dependencies)

# If we stubbed modules, don't run `go mod {tidy,vendor}` as
# dependencies are incomplete
if substitutions.empty?
# go mod tidy should run before go mod vendor to ensure any
# dependencies removed by go mod tidy are also removed from vendors.
run_go_mod_tidy
run_go_vendor
else
Expand Down Expand Up @@ -151,26 +149,7 @@ def run_go_vendor
handle_subprocess_error(stderr) unless status.success?
end

def update_go_mod(dependencies)
deps = dependencies.map do |dep|
{
name: dep.name,
version: "v" + dep.version.sub(/^v/i, ""),
indirect: dep.requirements.empty?
}
end

body = SharedHelpers.run_helper_subprocess(
command: NativeHelpers.helper_path,
env: ENVIRONMENT,
function: "updateDependencyFile",
args: { dependencies: deps }
)

write_go_mod(body)
end

def run_go_get
def run_go_get(dependencies)
tmp_go_file = "#{SecureRandom.hex}.go"

package = Dir.glob("[^\._]*.go").any? do |path|
Expand All @@ -179,7 +158,24 @@ def run_go_get

File.write(tmp_go_file, "package dummypkg\n") unless package

_, stderr, status = Open3.capture3(ENVIRONMENT, "go get -d")
# TODO: go 1.18 will make `-d` the default behavior, so remove the flag then
command = +"go get -d"
# `go get` accepts multiple packages, each separated by a space
dependencies.each do |dep|
# Use version pinning rather than `latest` just in case
# a new version gets released in the middle of our run.
version = "v" + dep.version.sub(/^v/i, "")
command << " #{dep.name}@#{version}"
end
_, stderr, status = Open3.capture3(ENVIRONMENT, command)
Copy link
Member

@jurre jurre May 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

spec/dependabot/go_modules/file_updater/go_mod_updater_spec.rb:245 is failing because we now no longer run go get -d on all the modules, so basically we're only running the code for update_go_mod (but now using go get instead of our custom helper), we still need to run a bare go get -d to verify that the packages we have downloaded have the module that we expect to find etc.

I verified the tests pass after adding a go get -d after this command, but what might be nice is keeping the original def run_go_get method, and moving the contents of this method into def update_go_mod

I think the tests should pass after that

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, so that was intentional...

I'd thought the generic go get -d wasn't needed because we now rely on go get lib@version to be smart enough to know how to upgrade a particular library and if some other part of the dep tree is hosed, then we let go get -d lib@version decide whether that is relevant or not. I don't expect (or want) dependabot to validate my existing dep tree, I only expect it to validate that version bumps don't put the dep tree in a worse state.

I still think that actually.

So let's do this, I'll break this PR in two. The first will keep this generic go get -d command, and simply swap out the custom helper for go get and then the second will be a discussion of whether this generic command is within the scope of what Dependabot should be doing.

That should keep the majority of this PR in a simple, non-controversial PR, and then the second PR (which will be more controversial) can be much more easily reasoned about/researched/discussed.

I'm also reasonably certain this first PR will not solve #3526 which was my original impetus for all this work. That'd require the second PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I get what you're saying, however:

I only expect it to validate that version bumps don't put the dep tree in a worse state.

I think that running the generic go get -d still protects against this, for the case the failing test was validating, it prevents updating to a version when the package has been renamed, right?

Maybe it is an edge-case we should not worry about, assume peoples CI will fail and they'll figure out what the issue is and update the import statements etc manually, but I definitely appreciate you pulling these out into separate PRs so we can discuss more focussed 🙇

Copy link
Member Author

@jeffwidman jeffwidman May 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Also, a few notes to myself when I get time to dig into this:

  1. IIRC, the other reason this was originally here was because we'd manually munged go.mod w/o touching go.sum, so we needed this go get to pickup those go.sum changes. (This could have also been done via go mod tidy, but go get was handling it well enough). This is why I originally thought this was no longer needed.
  2. The transitory resolving stuff will likely get cleaned up a lot more as part of lazy loading in go 1.17.
  3. There's still the open question though of should dependabot fail when something deep upstream fails... eg, this started failing yesterday 4 steps up our dep chain: tencentcloud dependency seems to be gone hashicorp/go-discover#172. I can see valid arguments both pro/con on this.
  4. it prevents updating to a version when the package has been renamed, right ...double-check whether this test checks a dep we are actually updating, or a different dep that's also present in go.mod, but not the one we are updating. However, regardless whether found in Dependabot or the user's CI, this is still an issue. Unlike Unfixable failure when the dep tree includes a dependency circle and one of those deps at one time referenced a broken module #3526 which is spurious because go get simply doesn't handle cyclical deps very well, and doesn't actually need handling.

handle_subprocess_error(stderr) unless status.success?

# Hmm... I'm still unclear/digging to understand why we'd need a blank `go get -d`
# possibly re-jigger func defs
# https://github.com/dependabot/dependabot-core/pull/3590#discussion_r632456405
# TODO: go 1.18 will make `-d` the default behavior, so remove the flag then
command = "go get -d"
_, stderr, status = Open3.capture3(ENVIRONMENT, command)
handle_subprocess_error(stderr) unless status.success?
ensure
File.delete(tmp_go_file) if File.exist?(tmp_go_file)
Expand Down
26 changes: 19 additions & 7 deletions go_modules/spec/dependabot/go_modules/file_updater/go_mod_updater_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,12 +236,23 @@
# OpenAPIV2 has been renamed to openapiv2 in this version
let(:dependency_version) { "v0.5.1" }

it "raises a DependencyFileNotResolvable error" do
error_class = Dependabot::DependencyFileNotResolvable
# NOTE: We explicitly don't want to raise a resolvability error from `go mod tidy`
it "does not raises a DependencyFileNotResolvable error" do
expect { updater.updated_go_sum_content }.
to raise_error(error_class) do |error|
expect(error.message).to include("googleapis/gnostic/OpenAPIv2")
end
to_not raise_error
end

it "updates the go.mod" do
# this is failing, but I'm not sure why.
# The code runs `go get -d github.com/googleapis/gnostic@v0.5.1`
# and then later runs `go mod tidy -e`.
# So I manually ran those in the test fixture, and it resulted in
# this line appearing. But when the test executes, this line is missing.
# I'm not sure why, and not sure how to run the Ruby debugger
# to step through it.
Copy link
Member Author

@jeffwidman jeffwidman Apr 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The test was failing. The backstory is pretty complicated:

  • aa67f7d removed catching the error, because go mod tidy didn't flag it.
  • Upgrade golang to v1.16 #3233 put it back because in go 1.16, go get -d started flagging the error again.
  • Now that we're switching from go get -d to go get -d <dep>@<version> this error is no longer thrown. Instead the go.mod file gets updated.

I manually replicated the go get -d github.com/googleapis/gnostic@v0.5.1 followed by go mod tidy -e and then updated the test to match what I'm seeing in the go.mod file.

Surprisingly though, the test is still failing complaining that the line isn't present.

So now I'm unclear if the test is wrong somehow, or if there's some wrong code, or what. I'm hesitant to "fix" the test to something that doesn't match the output from when I manually ran the steps.

Anyone else want to take a stab at it?

expect(updater.updated_go_mod_content).to include(
%(github.com/googleapis/gnostic v0.5.1\n)
)
end
end
end
Expand Down Expand Up @@ -301,7 +312,8 @@

before do
allow(Open3).to receive(:capture3).and_call_original
allow(Open3).to receive(:capture3).with(anything, "go get -d").and_return(["", stderr, exit_status])
cmd = "go get -d github.com/spf13/viper@v1.7.1"
allow(Open3).to receive(:capture3).with(anything, cmd).and_return(["", stderr, exit_status])
end

it { expect { subject }.to raise_error(Dependabot::DependencyFileNotResolvable, /The remote end hung up/) }
Expand Down Expand Up @@ -515,7 +527,7 @@
expect { updater.updated_go_sum_content }.
to raise_error(error_class) do |error|
expect(error.message).to include(
"go: github.com/deislabs/oras@v0.10.0 requires\n"\
"go: github.com/deislabs/oras@v0.9.0 requires\n"\
" github.com/docker/distribution@v0.0.0-00010101000000-000000000000: "\
"invalid version: unknown revision"
)
Expand Down
4 changes: 3 additions & 1 deletion go_modules/spec/dependabot/go_modules/file_updater_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,11 @@ module declares its path as: go.etcd.io/bbolt
before do
exit_status = double(success?: false)
allow(Open3).to receive(:capture3).and_call_original
allow(Open3).to receive(:capture3).with(anything, "go get -d").and_return(["", stderr, exit_status])
cmd = "go get -d github.com/etcd-io/bbolt@v1.3.5"
allow(Open3).to receive(:capture3).with(anything, cmd).and_return(["", stderr, exit_status])
end

# This is failing and I'm not sure why...
it "raises a helpful error" do
expect { updated_files }.to raise_error(Dependabot::GoModulePathMismatch)
end
Expand Down