From feced2cf9ec138eae5678dda576eabcb879ca1c9 Mon Sep 17 00:00:00 2001 From: Jeff Widman Date: Mon, 10 Apr 2023 12:37:28 -0700 Subject: [PATCH] Replace custom `go` helper with `go list` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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](https://github.com/dependabot/dependabot-core/issues/4448#issuecomment-1247317691): > 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](http://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 https://github.com/dependabot/dependabot-core/issues/6938 that there _is both_ an upstream copy (https://github.com/golang/go/issues/18387#issuecomment-277001616, which may potentially get deprecated https://github.com/golang/go/issues/57051) and an actual `go list` command that provides enough of what we need: * https://github.com/golang/go/issues/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: * https://github.com/golang/go/issues/18387 For our purposes it provides enough of what we need since we already have the module path extracted from `go.mod` (https://github.com/golang/go/issues/18387#issuecomment-1492664354). 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 back 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. --- go_modules/Dockerfile | 5 -- go_modules/README.md | 10 +-- go_modules/dependabot-go_modules.gemspec | 2 +- go_modules/helpers/Makefile | 9 --- go_modules/helpers/build | 28 -------- go_modules/helpers/go.mod | 5 -- go_modules/helpers/go.sum | 2 - go_modules/helpers/importresolver/main.go | 34 ---------- go_modules/helpers/main.go | 67 ------------------- .../lib/dependabot/go_modules/file_parser.rb | 1 - .../go_modules/file_updater/go_mod_updater.rb | 1 - .../dependabot/go_modules/metadata_finder.rb | 29 +++++++- .../dependabot/go_modules/native_helpers.rb | 20 ------ .../dependabot/go_modules/path_converter.rb | 20 ------ .../dependabot/go_modules/update_checker.rb | 1 - .../go_modules/metadata_finder_spec.rb | 42 ++++++++++++ .../go_modules/path_converter_spec.rb | 47 ------------- .../latest_version_finder_spec.rb | 1 - 18 files changed, 72 insertions(+), 252 deletions(-) delete mode 100644 go_modules/helpers/Makefile delete mode 100755 go_modules/helpers/build delete mode 100644 go_modules/helpers/go.mod delete mode 100644 go_modules/helpers/go.sum delete mode 100644 go_modules/helpers/importresolver/main.go delete mode 100644 go_modules/helpers/main.go delete mode 100644 go_modules/lib/dependabot/go_modules/native_helpers.rb delete mode 100644 go_modules/lib/dependabot/go_modules/path_converter.rb delete mode 100644 go_modules/spec/dependabot/go_modules/path_converter_spec.rb diff --git a/go_modules/Dockerfile b/go_modules/Dockerfile index b2fe5de260f..7e6cb1d45fd 100644 --- a/go_modules/Dockerfile +++ b/go_modules/Dockerfile @@ -17,10 +17,5 @@ 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 COPY --chown=dependabot:dependabot go_modules /home/dependabot/go_modules diff --git a/go_modules/README.md b/go_modules/README.md index 178dfe922bc..f0bcb9a1a61 100644 --- a/go_modules/README.md +++ b/go_modules/README.md @@ -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 ``` diff --git a/go_modules/dependabot-go_modules.gemspec b/go_modules/dependabot-go_modules.gemspec index 6021a2a3155..73aa0f6e221 100644 --- a/go_modules/dependabot-go_modules.gemspec +++ b/go_modules/dependabot-go_modules.gemspec @@ -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 diff --git a/go_modules/helpers/Makefile b/go_modules/helpers/Makefile deleted file mode 100644 index 1d5994f7005..00000000000 --- a/go_modules/helpers/Makefile +++ /dev/null @@ -1,9 +0,0 @@ -.PHONY = all - -all: darwin linux - -darwin: - GOOS=darwin GOARCH=amd64 go build -o go-helpers.darwin64 . - -linux: - GOOS=linux GOARCH=amd64 go build -o go-helpers.linux64 . diff --git a/go_modules/helpers/build b/go_modules/helpers/build deleted file mode 100755 index 0833055ed06..00000000000 --- a/go_modules/helpers/build +++ /dev/null @@ -1,28 +0,0 @@ -#!/bin/bash - -set -e - -if [ -z "$DEPENDABOT_NATIVE_HELPERS_PATH" ]; then - echo "Unable to build, DEPENDABOT_NATIVE_HELPERS_PATH is not set" - exit 1 -fi - -install_dir="$DEPENDABOT_NATIVE_HELPERS_PATH/go_modules" - -if ! [[ "$install_dir" =~ ^/ ]]; then - echo "$install_dir must be an absolute path" - exit 1 -fi - -if [ ! -d "$install_dir/bin" ]; then - mkdir -p "$install_dir/bin" -fi - -helpers_dir="$(dirname "${BASH_SOURCE[0]}")" -cd "$helpers_dir" - -os="$(uname -s | tr '[:upper:]' '[:lower:]')" -echo "building $install_dir/bin/helper" - -GOOS="$os" GOARCH=amd64 go build -o "$install_dir/bin/helper" . -go clean -cache -modcache diff --git a/go_modules/helpers/go.mod b/go_modules/helpers/go.mod deleted file mode 100644 index 56db04ad0be..00000000000 --- a/go_modules/helpers/go.mod +++ /dev/null @@ -1,5 +0,0 @@ -module github.com/dependabot/dependabot-core/go_modules/helpers - -go 1.20 - -require github.com/Masterminds/vcs v1.13.3 diff --git a/go_modules/helpers/go.sum b/go_modules/helpers/go.sum deleted file mode 100644 index 89365af4e39..00000000000 --- a/go_modules/helpers/go.sum +++ /dev/null @@ -1,2 +0,0 @@ -github.com/Masterminds/vcs v1.13.3 h1:IIA2aBdXvfbIM+yl/eTnL4hb1XwdpvuQLglAix1gweE= -github.com/Masterminds/vcs v1.13.3/go.mod h1:TiE7xuEjl1N4j016moRd6vezp6e6Lz23gypeXfzXeW8= diff --git a/go_modules/helpers/importresolver/main.go b/go_modules/helpers/importresolver/main.go deleted file mode 100644 index 938980edbef..00000000000 --- a/go_modules/helpers/importresolver/main.go +++ /dev/null @@ -1,34 +0,0 @@ -package importresolver - -import ( - "io/ioutil" - "strings" - - "github.com/Masterminds/vcs" -) - -type Args struct { - Import string -} - -func VCSRemoteForImport(args *Args) (interface{}, error) { - remote := args.Import - scheme := strings.Split(remote, ":")[0] - switch scheme { - case "http", "https": - default: - remote = "https://" + remote - } - - local, err := ioutil.TempDir("", "unused-vcs-local-dir") - if err != nil { - return nil, err - } - - repo, err := vcs.NewRepo(remote, local) - if err != nil { - return nil, err - } - - return repo.Remote(), nil -} diff --git a/go_modules/helpers/main.go b/go_modules/helpers/main.go deleted file mode 100644 index 4f8d6e8747d..00000000000 --- a/go_modules/helpers/main.go +++ /dev/null @@ -1,67 +0,0 @@ -package main - -import ( - "encoding/json" - "fmt" - "log" - "os" - - "github.com/dependabot/dependabot-core/go_modules/helpers/importresolver" -) - -type HelperParams struct { - Function string `json:"function"` - Args json.RawMessage `json:"args"` -} - -type Output struct { - Error string `json:"error,omitempty"` - Result interface{} `json:"result,omitempty"` -} - -func main() { - d := json.NewDecoder(os.Stdin) - helperParams := &HelperParams{} - if err := d.Decode(helperParams); err != nil { - abort(err) - } - - var ( - funcOut interface{} - funcErr error - ) - switch helperParams.Function { - case "getVcsRemoteForImport": - var args importresolver.Args - parseArgs(helperParams.Args, &args) - funcOut, funcErr = importresolver.VCSRemoteForImport(&args) - default: - abort(fmt.Errorf("unrecognised function '%s'", helperParams.Function)) - } - - if funcErr != nil { - abort(funcErr) - } - - output(&Output{Result: funcOut}) -} - -func parseArgs(data []byte, args interface{}) { - if err := json.Unmarshal(data, args); err != nil { - abort(err) - } -} - -func output(o *Output) { - bytes, jsonErr := json.Marshal(o) - if jsonErr != nil { - log.Fatal(jsonErr) - } - - os.Stdout.Write(bytes) -} - -func abort(err error) { - output(&Output{Error: err.Error()}) - os.Exit(1) -} diff --git a/go_modules/lib/dependabot/go_modules/file_parser.rb b/go_modules/lib/dependabot/go_modules/file_parser.rb index 023010f8c5c..6f7a1813baa 100644 --- a/go_modules/lib/dependabot/go_modules/file_parser.rb +++ b/go_modules/lib/dependabot/go_modules/file_parser.rb @@ -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" diff --git a/go_modules/lib/dependabot/go_modules/file_updater/go_mod_updater.rb b/go_modules/lib/dependabot/go_modules/file_updater/go_mod_updater.rb index 6ffabe89186..8ce5e2472c0 100644 --- a/go_modules/lib/dependabot/go_modules/file_updater/go_mod_updater.rb +++ b/go_modules/lib/dependabot/go_modules/file_updater/go_mod_updater.rb @@ -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" diff --git a/go_modules/lib/dependabot/go_modules/metadata_finder.rb b/go_modules/lib/dependabot/go_modules/metadata_finder.rb index 037d85431d1..c0fa27cc14a 100644 --- a/go_modules/lib/dependabot/go_modules/metadata_finder.rb +++ b/go_modules/lib/dependabot/go_modules/metadata_finder.rb @@ -2,7 +2,6 @@ require "dependabot/metadata_finders" require "dependabot/metadata_finders/base" -require "dependabot/go_modules/path_converter" module Dependabot module GoModules @@ -10,9 +9,35 @@ 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 + end + + def look_up_source_using_go_list + # Turn off the module proxy for private dependencies + environment = { "GOPRIVATE" => @goprivate } + + command = + "go list -m -json #{dependency.name}@latest" + # TODO: Should this be SharedHelpers.run_shell_command() or SharedHelpers.escape_command()? + # See both usages here back to back: https://github.com/dependabot/dependabot-core/blob/2932de643fc6fb0b3eeac91acc55364bc3c090e0/go_modules/lib/dependabot/go_modules/update_checker/latest_version_finder.rb#L89-L97 + command = SharedHelpers.escape_command(command) + + stdout, stderr, status = Open3.capture3(environment, command) + handle_subprocess_error(stderr) unless status.success? + + module_metadata_json = stdout + # Other useful metadata is available, see output from this example: $ go list -m -json golang.org/x/tools@latest + url = JSON.parse(module_metadata_json["Origin"]["URL"]) 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 diff --git a/go_modules/lib/dependabot/go_modules/native_helpers.rb b/go_modules/lib/dependabot/go_modules/native_helpers.rb deleted file mode 100644 index 676a783c00d..00000000000 --- a/go_modules/lib/dependabot/go_modules/native_helpers.rb +++ /dev/null @@ -1,20 +0,0 @@ -# frozen_string_literal: true - -module Dependabot - module GoModules - module NativeHelpers - def self.helper_path - clean_path(File.join(native_helpers_root, "go_modules/bin/helper")) - end - - def self.native_helpers_root - default_path = File.join(__dir__, "../../../helpers/install-dir") - ENV.fetch("DEPENDABOT_NATIVE_HELPERS_PATH", default_path) - end - - def self.clean_path(path) - Pathname.new(path).cleanpath.to_path - end - end - end -end diff --git a/go_modules/lib/dependabot/go_modules/path_converter.rb b/go_modules/lib/dependabot/go_modules/path_converter.rb deleted file mode 100644 index dc073992a6c..00000000000 --- a/go_modules/lib/dependabot/go_modules/path_converter.rb +++ /dev/null @@ -1,20 +0,0 @@ -# frozen_string_literal: true - -require "dependabot/go_modules/native_helpers" - -module Dependabot - module GoModules - module PathConverter - def self.git_url_for_path(path) - # Save a query by manually converting golang.org/x names - import_path = path.gsub(%r{^golang\.org/x}, "github.com/golang") - - SharedHelpers.run_helper_subprocess( - command: NativeHelpers.helper_path, - function: "getVcsRemoteForImport", - args: { import: import_path } - ) - end - end - end -end diff --git a/go_modules/lib/dependabot/go_modules/update_checker.rb b/go_modules/lib/dependabot/go_modules/update_checker.rb index 3f6ac0dbb11..2329a8a493e 100644 --- a/go_modules/lib/dependabot/go_modules/update_checker.rb +++ b/go_modules/lib/dependabot/go_modules/update_checker.rb @@ -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 diff --git a/go_modules/spec/dependabot/go_modules/metadata_finder_spec.rb b/go_modules/spec/dependabot/go_modules/metadata_finder_spec.rb index 7a8b1e28d91..d662b7840b3 100644 --- a/go_modules/spec/dependabot/go_modules/metadata_finder_spec.rb +++ b/go_modules/spec/dependabot/go_modules/metadata_finder_spec.rb @@ -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 diff --git a/go_modules/spec/dependabot/go_modules/path_converter_spec.rb b/go_modules/spec/dependabot/go_modules/path_converter_spec.rb deleted file mode 100644 index 53599d9c371..00000000000 --- a/go_modules/spec/dependabot/go_modules/path_converter_spec.rb +++ /dev/null @@ -1,47 +0,0 @@ -# frozen_string_literal: true - -require "spec_helper" -require "dependabot/go_modules/path_converter" - -RSpec.describe Dependabot::GoModules::PathConverter do - describe ".git_url_for_path" do - subject { described_class.git_url_for_path(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 diff --git a/go_modules/spec/dependabot/go_modules/update_checker/latest_version_finder_spec.rb b/go_modules/spec/dependabot/go_modules/update_checker/latest_version_finder_spec.rb index b13e01fd435..d7d46981431 100644 --- a/go_modules/spec/dependabot/go_modules/update_checker/latest_version_finder_spec.rb +++ b/go_modules/spec/dependabot/go_modules/update_checker/latest_version_finder_spec.rb @@ -3,7 +3,6 @@ require "spec_helper" require "dependabot/dependency" require "dependabot/dependency_file" -require "dependabot/go_modules/native_helpers" require "dependabot/go_modules/update_checker/latest_version_finder" RSpec.describe Dependabot::GoModules::UpdateChecker::LatestVersionFinder do