go_repository: add importpath and vcs flags (#313) (#319)

* go_repository: add importpath and vcs flags (#313)

This change allows fetch_repo to fetch repositories without attempting
to determine the repo root if the user supplies the information
themselves. This allows users to be able to fetch repositories that the
vcs package would otherwise not be able to recognize.

This change also changes the behavior of the --remote flag to match the
git_repository rule.

* Respond to jayconrod@ comments in #319.

* Add BUILD rule for fetch_repo_test.go
* Stub out vcs.RepoRootForImportPath
* Make remote and vcs flags not being set in fetch_repo a hard error.
* Verify VCS is not nil before returning it in a RepoRoot.

* Fix go_repository rule for #319.

Since the vcs field in go_repository was always set to "git", fetch_repo was
always failing if the user passed in only the importpath.

README.md

@@ -204,13 +204,16 @@ placed in the WORKSPACE.
 ### `go_repository`
 
 ```bzl
-go_repository(name, importpath, remote, commit, tag)
+go_repository(name, importpath, remote, vcs, commit, tag)
 ```
 
 Fetches a remote repository of a Go project, expecting it contains `BUILD`
 files. It is an analogy to `git_repository` but it recognizes importpath
 redirection of Go.
 
+Either `importpath` or `remote` may be specified. To bypass importpath
+redirection, specify both `remote` and `vcs`.
+
 <table class="table table-condensed table-bordered table-params">
   <colgroup>
     <col class="col-param" />
@@ -232,7 +235,7 @@ redirection of Go.
     <tr>
       <td><code>importpath</code></td>
       <td>
-        <code>String, required</code>
+        <code>String, optional</code>
         <p>An import path in Go, which also provides a default value for the
         root of the target remote repository</p>
       </td>
@@ -241,8 +244,16 @@ redirection of Go.
       <td><code>remote</code></td>
       <td>
         <code>String, optional</code>
-        <p>The root of the target remote repository, if this differs from the
-        value of <code>importpath</code></p>
+        <p>The URI of the target remote repository, if this cannot be determined
+        from the value of <code>importpath</code>.</p>
+      </td>
+    </tr>
+    <tr>
+      <td><code>vcs</code></td>
+      <td>
+        <code>String, optional</code>
+        <p>The version control system to use for fetching the repository. Useful
+        for disabling importpath redirection if necessary.</p>
       </td>
     </tr>
     <tr>
@@ -267,7 +278,7 @@ redirection of Go.
 ### `new_go_repository`
 
 ```bzl
-new_go_repository(name, importpath, remote, commit, tag)
+new_go_repository(name, importpath, remote, vcs, commit, tag)
 ```
 
 Fetches a remote repository of a Go project and automatically generates
@@ -295,7 +306,7 @@ importpath redirection of Go.
     <tr>
       <td><code>importpath</code></td>
       <td>
-        <code>String, required</code>
+        <code>String, optional</code>
         <p>An import path in Go, which also provides a default value for the
         root of the target remote repository</p>
       </td>
@@ -304,10 +315,17 @@ importpath redirection of Go.
       <td><code>remote</code></td>
       <td>
         <code>String, optional</code>
-        <p>The root of the target remote repository, if this differs from the
+        <p>The URI of the target remote repository, if this differs from the
         value of <code>importpath</code></p>
       </td>
     </tr>
+    <tr>
+      <td><code>vcs</code></td>
+      <td>
+        <code>String, optional</code>
+        <p>The version control system to use for fetching the repository.</p>
+      </td>
+    </tr>
     <tr>
       <td><code>commit</code></td>
       <td>

go/private/go_repository.bzl

@@ -27,14 +27,18 @@ def _go_repository_impl(ctx):
 
   # TODO(yugui): support submodule?
   # c.f. https://www.bazel.io/versions/master/docs/be/workspace.html#git_repository.init_submodules
-  remote = ctx.attr.remote if ctx.attr.remote else ctx.attr.importpath
+  remote = ctx.attr.remote
+  vcs = ctx.attr.vcs
+  importpath = ctx.attr.importpath
   result = ctx.execute([
       fetch_repo,
       '--dest', ctx.path(''),
       '--remote', remote,
-      '--rev', rev])
+      '--rev', rev,
+      '--vcs', vcs,
+      '--importpath', importpath])
   if result.return_code:
-    fail("failed to fetch %s: %s" % (remote, result.stderr))
+    fail("failed to fetch %s: %s" % (ctx.name, result.stderr))
 
 
 def _new_go_repository_impl(ctx):
@@ -57,8 +61,9 @@ def _new_go_repository_impl(ctx):
 
 _go_repository_attrs = {
     "build_file_name": attr.string(),
-    "importpath": attr.string(mandatory = True),
+    "importpath": attr.string(),
     "remote": attr.string(),
+    "vcs": attr.string(default="", values=["", "git", "hg", "svn", "bzr"]),
     "commit": attr.string(),
     "tag": attr.string(),
     "_fetch_repo": attr.label(

go/tools/fetch_repo/BUILD

@@ -1,7 +1,20 @@
-load("//go:def.bzl", "go_binary")
+load("//go:def.bzl", "go_binary", "go_library", "go_test")
 
 go_binary(
     name = "fetch_repo",
+    library = ":go_default_library",
+)
+
+go_library(
+    name = "go_default_library",
     srcs = ["main.go"],
+    visibility = ["//visibility:private"],
+    deps = ["@org_golang_x_tools//go/vcs:go_default_library"],
+)
+
+go_test(
+    name = "go_default_test",
+    srcs = ["fetch_repo_test.go"],
+    library = ":go_default_library",
     deps = ["@org_golang_x_tools//go/vcs:go_default_library"],
 )

go/tools/fetch_repo/fetch_repo_test.go

@@ -0,0 +1,96 @@
+package main
+
+import (
+	"os"
+	"reflect"
+	"testing"
+
+	"golang.org/x/tools/go/vcs"
+)
+
+var (
+	root = &vcs.RepoRoot{
+		VCS:  vcs.ByCmd("git"),
+		Repo: "https://github.com/bazeltest/rules_go",
+		Root: "github.com/bazeltest/rules_go",
+	}
+)
+
+func TestMain(m *testing.M) {
+	// Replace vcs.RepoRootForImportPath to disable any network calls.
+	repoRootForImportPath = func(_ string, _ bool) (*vcs.RepoRoot, error) {
+		return root, nil
+	}
+	os.Exit(m.Run())
+}
+
+func TestGetRepoRoot(t *testing.T) {
+	for _, tc := range []struct {
+		label      string
+		remote     string
+		cmd        string
+		importpath string
+		r          *vcs.RepoRoot
+	}{
+		{
+			label:      "all",
+			remote:     "https://github.com/bazeltest/rules_go",
+			cmd:        "git",
+			importpath: "github.com/bazeltest/rules_go",
+			r:          root,
+		},
+		{
+			label:      "different remote",
+			remote:     "https://example.com/rules_go",
+			cmd:        "git",
+			importpath: "github.com/bazeltest/rules_go",
+			r: &vcs.RepoRoot{
+				VCS:  vcs.ByCmd("git"),
+				Repo: "https://example.com/rules_go",
+				Root: "github.com/bazeltest/rules_go",
+			},
+		},
+		{
+			label:      "only importpath",
+			importpath: "github.com/bazeltest/rules_go",
+			r:          root,
+		},
+	} {
+		r, err := getRepoRoot(tc.remote, tc.cmd, tc.importpath)
+		if err != nil {
+			t.Errorf("[%s] %v", tc.label, err)
+		}
+		if !reflect.DeepEqual(r, tc.r) {
+			t.Errorf("[%s] Expected %+v, got %+v", tc.label, tc.r, r)
+		}
+	}
+}
+
+func TestGetRepoRoot_error(t *testing.T) {
+	for _, tc := range []struct {
+		label      string
+		remote     string
+		cmd        string
+		importpath string
+	}{
+		{
+			label:  "importpath as remote",
+			remote: "github.com/bazeltest/rules_go",
+		},
+		{
+			label:      "missing vcs",
+			remote:     "https://github.com/bazeltest/rules_go",
+			importpath: "github.com/bazeltest/rules_go",
+		},
+		{
+			label:      "missing remote",
+			cmd:        "git",
+			importpath: "github.com/bazeltest/rules_go",
+		},
+	} {
+		r, err := getRepoRoot(tc.remote, tc.cmd, tc.importpath)
+		if err == nil {
+			t.Errorf("[%s] expected error. Got %+v", tc.label, r)
+		}
+	}
+}

go/tools/fetch_repo/main.go

@@ -19,19 +19,50 @@ import (
 )
 
 var (
-	remote = flag.String("remote", "", "Go importpath to the repository fetch")
-	rev    = flag.String("rev", "", "target revision")
-	dest   = flag.String("dest", "", "destination directory")
+	remote     = flag.String("remote", "", "The URI of the remote repository. Must be used with the --vcs flag.")
+	cmd        = flag.String("vcs", "", "Version control system to use to fetch the repository. Should be one of: git,hg,svn,bzr. Must be used with the --remote flag.")
+	rev        = flag.String("rev", "", "target revision")
+	dest       = flag.String("dest", "", "destination directory")
+	importpath = flag.String("importpath", "", "Go importpath to the repository fetch")
+
+	// Used for overriding in tests to disable network calls.
+	repoRootForImportPath = vcs.RepoRootForImportPath
 )
 
+func getRepoRoot(remote, cmd, importpath string) (*vcs.RepoRoot, error) {
+	if (cmd == "") != (remote == "") {
+		return nil, fmt.Errorf("--remote should be used with the --vcs flag. If this is an import path, use --importpath instead.")
+	}
+
+	if cmd != "" && remote != "" {
+		v := vcs.ByCmd(cmd)
+		if v == nil {
+			return nil, fmt.Errorf("invalid VCS type: %s", cmd)
+		}
+		return &vcs.RepoRoot{
+			VCS:  v,
+			Repo: remote,
+			Root: importpath,
+		}, nil
+	}
+
+	// User did not give us complete information for VCS / Remote.
+	// Try to figure out the information from the import path.
+	r, err := repoRootForImportPath(importpath, true)
+	if err != nil {
+		return nil, err
+	}
+	if importpath != r.Root {
+		return nil, fmt.Errorf("not a root of a repository: %s", importpath)
+	}
+	return r, nil
+}
+
 func run() error {
-	r, err := vcs.RepoRootForImportPath(*remote, true)
+	r, err := getRepoRoot(*remote, *cmd, *importpath)
 	if err != nil {
 		return err
 	}
-	if *remote != r.Root {
-		return fmt.Errorf("not a root of a repository: %s", *remote)
-	}
 	return r.VCS.CreateAtRev(*dest, r.Repo, *rev)
 }