Add a clarifying error about dependency cycle found for internal tests (

bazelbuild#3422)

Co-authored-by: Yushan Lin <yushan@uber.com>

go/private/actions/archive.bzl

@@ -39,7 +39,8 @@ load(
     "emit_compilepkg",
 )
 
-def emit_archive(go, source = None, _recompile_suffix = "", extra_archive_datas = []):
+
+def emit_archive(go, source = None, _recompile_suffix = "", recompile_internal_deps = None, extra_archive_datas = []):
     """See go/toolchains.rst#archive for full documentation."""
 
     if source == None:
@@ -132,6 +133,7 @@ def emit_archive(go, source = None, _recompile_suffix = "", extra_archive_datas
             gc_goopts = source.gc_goopts,
             cgo = False,
             testfilter = testfilter,
+            recompile_internal_deps = recompile_internal_deps,
         )
 
     data = GoArchiveData(

go/private/actions/compilepkg.bzl

@@ -59,7 +59,8 @@ def emit_compilepkg(
         out_export = None,
         out_cgo_export_h = None,
         gc_goopts = [],
-        testfilter = None):  # TODO: remove when test action compiles packages
+        testfilter = None,  # TODO: remove when test action compiles packages
+        recompile_internal_deps = []):
     """Compiles a complete Go package."""
     if sources == None:
         fail("sources is a required parameter")
@@ -99,6 +100,8 @@ def emit_compilepkg(
         args.add("-cover_format", go.cover_format)
         args.add_all(cover, before_each = "-cover")
     args.add_all(archives, before_each = "-arc", map_each = _archive)
+    if recompile_internal_deps:
+        args.add_all(recompile_internal_deps, before_each = "-recompile_internal_deps")
     if importpath:
         args.add("-importpath", importpath)
     else:

go/private/rules/test.bzl

@@ -591,14 +591,26 @@ def _recompile_external_deps(go, external_source, internal_archive, library_labe
     # is shared between the internal and external archive. The internal archive
     # can't import anything that imports itself.
     internal_source = internal_archive.source
-    internal_deps = [dep for dep in internal_source.deps if not need_recompile[get_archive(dep).data.label]]
+
+    internal_deps = []
+
+    # Pass internal dependencies that need to be recompiled down to the builder to check if the internal archive
+    # tries to import any of the dependencies. If there is, that means that there is a dependency cycle.
+    need_recompile_deps = []
+    for dep in internal_source.deps:
+        dep_data = get_archive(dep).data
+        if not need_recompile[dep_data.label]:
+            internal_deps.append(dep)
+        else:
+            need_recompile_deps.append(dep_data.importpath)
+
     x_defs = dict(internal_source.x_defs)
     x_defs.update(internal_archive.x_defs)
     attrs = structs.to_dict(internal_source)
     attrs["deps"] = internal_deps
     attrs["x_defs"] = x_defs
     internal_source = GoSource(**attrs)
-    internal_archive = go.archive(go, internal_source, _recompile_suffix = ".recompileinternal")
+    internal_archive = go.archive(go, internal_source, _recompile_suffix = ".recompileinternal", recompile_internal_deps = need_recompile_deps)
 
     # Build a map from labels to possibly recompiled GoArchives.
     label_to_archive = {}

go/tools/builders/compilepkg.go

@@ -49,7 +49,7 @@ func compilePkg(args []string) error {
 
 	fs := flag.NewFlagSet("GoCompilePkg", flag.ExitOnError)
 	goenv := envFlags(fs)
-	var unfilteredSrcs, coverSrcs, embedSrcs, embedLookupDirs, embedRoots multiFlag
+	var unfilteredSrcs, coverSrcs, embedSrcs, embedLookupDirs, embedRoots, recompileInternalDeps multiFlag
 	var deps archiveMultiFlag
 	var importPath, packagePath, nogoPath, packageListPath, coverMode string
 	var outPath, outFactsPath, cgoExportHPath string
@@ -82,6 +82,7 @@ func compilePkg(args []string) error {
 	fs.StringVar(&cgoExportHPath, "cgoexport", "", "The _cgo_exports.h file to write")
 	fs.StringVar(&testFilter, "testfilter", "off", "Controls test package filtering")
 	fs.StringVar(&coverFormat, "cover_format", "", "Emit source file paths in coverage instrumentation suitable for the specified coverage format")
+	fs.Var(&recompileInternalDeps, "recompile_internal_deps", "The import path of the direct dependencies that needs to be recompiled.")
 	if err := fs.Parse(args); err != nil {
 		return err
 	}
@@ -162,7 +163,8 @@ func compilePkg(args []string) error {
 		outPath,
 		outFactsPath,
 		cgoExportHPath,
-		coverFormat)
+		coverFormat,
+		recompileInternalDeps)
 }
 
 func compileArchive(
@@ -192,6 +194,7 @@ func compileArchive(
 	outXPath string,
 	cgoExportHPath string,
 	coverFormat string,
+	recompileInternalDeps []string,
 ) error {
 	workDir, cleanup, err := goenv.workDir()
 	if err != nil {
@@ -363,7 +366,7 @@ func compileArchive(
 
 	// Check that the filtered sources don't import anything outside of
 	// the standard library and the direct dependencies.
-	imports, err := checkImports(srcs.goSrcs, deps, packageListPath)
+	imports, err := checkImports(srcs.goSrcs, deps, packageListPath, importPath, recompileInternalDeps)
 	if err != nil {
 		return err
 	}

go/tools/builders/importcfg.go

@@ -33,11 +33,11 @@ type archive struct {
 }
 
 // checkImports verifies that each import in files refers to a
-// direct dependendency in archives or to a standard library package
+// direct dependency in archives or to a standard library package
 // listed in the file at stdPackageListPath. checkImports returns
 // a map from source import paths to elements of archives or to nil
 // for standard library packages.
-func checkImports(files []fileInfo, archives []archive, stdPackageListPath string) (map[string]*archive, error) {
+func checkImports(files []fileInfo, archives []archive, stdPackageListPath string, importPath string, recompileInternalDeps []string) (map[string]*archive, error) {
 	// Read the standard package list.
 	packagesTxt, err := ioutil.ReadFile(stdPackageListPath)
 	if err != nil {
@@ -71,7 +71,11 @@ func checkImports(files []fileInfo, archives []archive, stdPackageListPath strin
 			importAliasToArchive[imp] = arc
 		}
 	}
-
+	// Construct recompileInternalDeps as a map to check if there are imports that are disallowed.
+	recompileInternalDepMap := make(map[string]struct{})
+	for _, dep := range recompileInternalDeps {
+		recompileInternalDepMap[dep] = struct{}{}
+	}
 	// Build the import map.
 	imports := make(map[string]*archive)
 	var derr depsError
@@ -83,6 +87,9 @@ func checkImports(files []fileInfo, archives []archive, stdPackageListPath strin
 				// errors for them here, but they will probably break something else.
 				continue
 			}
+			if _, ok := recompileInternalDepMap[path]; ok {
+				return nil, fmt.Errorf("dependency cycle detected between %q and %q in file %q", importPath, path, f.filename)
+			}
 			if stdPkgs[path] {
 				imports[path] = nil
 			} else if arc := importToArchive[path]; arc != nil {