Skip to content

Commit

Permalink
Replace custom go helper with go list
Browse files Browse the repository at this point in the history
Over time, we've managed to thin out our custom `go` helper in favor of
native package manager tooling via `go list`, `go get`, and `go mod`.

The final remaining usage was for translating package import URLs into
the actual repository URL for fetching metadata.

The original impetus for the `github.com/Masterminds/vcs` library was
that it was a copy/paste of some code in core `go`:

 [Further context](#4448 (comment)):

> I think the code is trying to map a Go import path back to a repository so we can e.g. link to the repo in the Dependabot PR, find the CHANGELOG, link to the diff between versions, etc, etc.
> It’s not totally trivial with Go because:
> 1. they’ve got that whole vanity URL thing going on, where you have to pass `go-get=1` in to make it act like a fancy redirect to a repo
> 2. a Go import path doesn’t necessarily actually match a repo URL. For instance, I might be able to import https://github.com/go-kit/kit/tracing, but that’s not a valid URL on GitHub. The repo is [github.com/go-kit/kit](https://github.com/go-kit/kit), and the package is browsable at https://github.com/go-kit/kit/tree/master/tracing. So Go bakes in some [logic](https://github.com/golang/go/blob/95c125a44ad1a0c3e441c3214160cd7b4483e79c/src/cmd/go/internal/vcs/vcs.go#L1437) to handle that for a series of SCMs, and the Masterminds/vcs package does a [similar thing](https://github.com/Masterminds/vcs/blob/master/vcs_remote_lookup.go) too.
>
> Ideally, we’d just use whatever golang/go does, but IIRC that wasn’t importable so the next best thing was the Masterminds/vcs implementation. The Ruby code you linked to predates our usage of that Go library, and I’d guess is just a really incomplete Ruby implementation of the same thing.
> Perhaps these days a) there’s a good Ruby implementation, or b) we think the VCS list is stable enough we’d be happy to translate it to Ruby, or c) we could somehow codegen the Ruby from the canonical Go implementation? Or we just stick with that one-off Go helper?

However, I discovered after doing some digging in
#6938 that there
_is_ both an upstream copy
(golang/go#18387 (comment)),
which may potentially get deprecated
(golang/go#57051). Furthermore, there _is_
actually a `go list` command that provides enough of what we need:
* golang/go#44742

```
$ go list -m -f '{{.Origin}}' golang.org/x/tools@latest
{git https://go.googlesource.com/tools    refs/tags/v0.3.0 502c634771c4ba335286d55fc24eeded1704f592 }
```

While this doesn't fully resolve the upstream issue:
* golang/go#18387

For our purposes it provides enough of what we need since we already
have the module path extracted from `go.mod` (golang/go#18387 (comment)).

Performing this switch allows us to completely delete the native go
helper altogether. I debated leaving the framework in place in case we
later wish to extend it for other features, but decided for now to rip
it out as it's easy enough to add later. And in general we're trying to
move toward leveraging native package manager tooling whenever possible.
So if we later needed an additional feature, we're probably start with a
discussion with the `go` team to see if they'd build it into the `go`
cmd tooling.
  • Loading branch information
jeffwidman committed Apr 10, 2023
1 parent 2932de6 commit 1fd501c
Show file tree
Hide file tree
Showing 18 changed files with 75 additions and 252 deletions.
6 changes: 1 addition & 5 deletions go_modules/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,6 @@ RUN cd /tmp \
&& tar -xzf go-${TARGETARCH}.tar.gz -C /opt \
&& rm go-${TARGETARCH}.tar.gz

ENV DEPENDABOT_NATIVE_HELPERS_PATH="/opt"

COPY go_modules/helpers /opt/go_modules/helpers
RUN bash /opt/go_modules/helpers/build

USER dependabot
# TODO: can this be deleted? I think no, it needs to stay for writing to the go module cache...
COPY --chown=dependabot:dependabot go_modules /home/dependabot/go_modules
10 changes: 2 additions & 8 deletions go_modules/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,12 @@ Go modules support for [`dependabot-core`][core-repo].

### Running locally

1. Install native helpers
```
$ export DEPENDABOT_NATIVE_HELPERS_PATH=$PWD/helpers/install-dir
$ helpers/build
```

2. Install Ruby dependencies
1. Install Ruby dependencies
```
$ bundle install
```

3. Run tests
2. Run tests
```
$ bundle exec rspec spec
```
Expand Down
2 changes: 1 addition & 1 deletion go_modules/dependabot-go_modules.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -35,5 +35,5 @@ Gem::Specification.new do |spec|

next unless File.exist?("../.gitignore")

spec.files += `git -C #{__dir__} ls-files lib helpers -z`.split("\x0")
spec.files += `git -C #{__dir__} ls-files lib -z`.split("\x0")
end
9 changes: 0 additions & 9 deletions go_modules/helpers/Makefile

This file was deleted.

28 changes: 0 additions & 28 deletions go_modules/helpers/build

This file was deleted.

5 changes: 0 additions & 5 deletions go_modules/helpers/go.mod

This file was deleted.

2 changes: 0 additions & 2 deletions go_modules/helpers/go.sum

This file was deleted.

34 changes: 0 additions & 34 deletions go_modules/helpers/importresolver/main.go

This file was deleted.

67 changes: 0 additions & 67 deletions go_modules/helpers/main.go

This file was deleted.

1 change: 0 additions & 1 deletion go_modules/lib/dependabot/go_modules/file_parser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
require "open3"
require "dependabot/dependency"
require "dependabot/file_parsers/base/dependency_set"
require "dependabot/go_modules/path_converter"
require "dependabot/go_modules/replace_stubber"
require "dependabot/errors"
require "dependabot/file_parsers"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
require "dependabot/errors"
require "dependabot/logger"
require "dependabot/go_modules/file_updater"
require "dependabot/go_modules/native_helpers"
require "dependabot/go_modules/replace_stubber"
require "dependabot/go_modules/resolvability_errors"

Expand Down
31 changes: 29 additions & 2 deletions go_modules/lib/dependabot/go_modules/metadata_finder.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,44 @@

require "dependabot/metadata_finders"
require "dependabot/metadata_finders/base"
require "dependabot/go_modules/path_converter"

module Dependabot
module GoModules
class MetadataFinder < Dependabot::MetadataFinders::Base
private

def look_up_source
url = Dependabot::GoModules::PathConverter.git_url_for_path(dependency.name)
look_up_source_using_go_list(dependency.name)
end

# TODO should this func be moved to lib/dependabot/go_modules/metadata_finder/ subdir?
# I'm unclear when we organize things into a subfolder rather than inlining...
def look_up_source_using_go_list(path)
debugger
# Save a query by manually converting golang.org/x names
import_path = path.gsub(%r{^golang\.org/x}, "github.com/golang")

# TODO WIP, this command isn't quite right I'm reading though `go help list` to grok it
command = + "go list -m -f '{{.Origin}}' " + import_path
command = SharedHelpers.escape_command(command)

stdout, stderr, status = Open3.capture3(environment, command)
handle_subprocess_error(stderr) unless status.success?

debugger

url = stdout # TODO extract URL for returned result
Source.from_url(url) if url
end

def handle_subprocess_error(stderr)
# As we discover errors custom to go list, add handling for them here.
# See go_mod_updater.rb#handle_subprocess_error for example

# We don't know what happened so we raise a generic error
msg = stderr.lines.last(10).join.strip
raise Dependabot::DependabotError, msg
end
end
end
end
Expand Down
20 changes: 0 additions & 20 deletions go_modules/lib/dependabot/go_modules/native_helpers.rb

This file was deleted.

20 changes: 0 additions & 20 deletions go_modules/lib/dependabot/go_modules/path_converter.rb

This file was deleted.

1 change: 0 additions & 1 deletion go_modules/lib/dependabot/go_modules/update_checker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
require "dependabot/update_checkers/base"
require "dependabot/shared_helpers"
require "dependabot/errors"
require "dependabot/go_modules/native_helpers"
require "dependabot/go_modules/version"

module Dependabot
Expand Down
42 changes: 42 additions & 0 deletions go_modules/spec/dependabot/go_modules/metadata_finder_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -63,4 +63,46 @@
it { is_expected.to eq("https://github.com/satori/go.uuid") }
end
end

describe "#look_up_source_using_go_list" do
subject { described_class.look_up_source_using_go_list(path) }

let(:path) { "gopkg.in/guregu/null.v3" }

context "with a path that is immediately recognisable as a git source" do
let(:path) { "github.com/drewolson/testflight" }
it { is_expected.to eq("https://github.com/drewolson/testflight") }
end

context "with a golang.org path" do
let(:path) { "golang.org/x/tools" }
it { is_expected.to eq("https://github.com/golang/tools") }
end

context "with a path that ends in .git" do
let(:path) { "git.fd.io/govpp.git" }
it { is_expected.to eq("https://git.fd.io/govpp.git") }
end

context "with a vanity URL that needs to be fetched" do
let(:path) { "k8s.io/apimachinery" }
it { is_expected.to eq("https://github.com/kubernetes/apimachinery") }
end

xcontext "with a vanity URL that redirects" do
let(:path) { "code.cloudfoundry.org/bytefmt" }
it { is_expected.to eq("https://github.com/cloudfoundry/bytefmt") }
end

context "with a vanity URL that 404s, but is otherwise valid" do
let(:path) { "gonum.org/v1/gonum" }
it { is_expected.to eq("https://github.com/gonum/gonum") }
end

context "with a path that already includes a scheme" do
let(:path) { "https://github.com/drewolson/testflight" }
it { is_expected.to eq("https://github.com/drewolson/testflight") }
end
end

end
47 changes: 0 additions & 47 deletions go_modules/spec/dependabot/go_modules/path_converter_spec.rb

This file was deleted.

Loading

0 comments on commit 1fd501c

Please sign in to comment.