internal/fetch: support multiple build contexts
Extend the goPackage type to handle multiple documentation build
contexts (GOOS/GOARCH combinations).
When goPackages are coverted to Units, ignore all but the first.
For golang/go#37232
Change-Id: I706e6e407c2b3c31963032f1db5aa9a297e7ad8a
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/279458
Trust: Jonathan Amsterdam <jba@google.com>
Trust: Julie Qiu <julie@golang.org>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Julie Qiu <julie@golang.org>
diff --git a/internal/fetch/fetch_test.go b/internal/fetch/fetch_test.go
index b7fd746..897efb1 100644
--- a/internal/fetch/fetch_test.go
+++ b/internal/fetch/fetch_test.go
@@ -88,13 +88,13 @@
}
t.Run(fmt.Sprintf("%s:%s", fetcher.name, test.name), func(t *testing.T) {
ctx := context.Background()
- ctx, cancel := context.WithTimeout(ctx, 60*time.Second)
+ ctx, cancel := context.WithTimeout(ctx, 150*time.Second)
defer cancel()
got, d := fetcher.fetch(t, true, ctx, test.mod, test.fetchVersion)
defer got.Defer()
if got.Error != nil {
- t.Fatal("fetching failed: %w", got.Error)
+ t.Fatalf("fetching failed: %v", got.Error)
}
if !test.cleaned {
test.mod.fr = cleanFetchResult(t, test.mod.fr, d)
diff --git a/internal/fetch/fetchlocal.go b/internal/fetch/fetchlocal.go
index 6325f60..8f171b3 100644
--- a/internal/fetch/fetchlocal.go
+++ b/internal/fetch/fetchlocal.go
@@ -19,7 +19,6 @@
"golang.org/x/mod/modfile"
"golang.org/x/pkgsite/internal/derrors"
- "golang.org/x/pkgsite/internal/log"
"golang.org/x/pkgsite/internal/source"
)
@@ -55,8 +54,6 @@
if fi != nil {
finishFetchInfo(fi, fr.Status, fr.Error)
}
- log.Debugf(ctx, "memory after fetch of %s: %dM", fr.ModulePath, allocMeg())
-
}()
info, err := os.Stat(localPath)
diff --git a/internal/fetch/load.go b/internal/fetch/load.go
index 1e62873..be5b7ad 100644
--- a/internal/fetch/load.go
+++ b/internal/fetch/load.go
@@ -54,16 +54,18 @@
}
// loadPackage loads a Go package by calling loadPackageWithBuildContext, trying
-// several build contexts in turn. The first build context in the list to produce
-// a non-empty package is used. If none of them result in a package, then
-// loadPackage returns nil, nil.
+// several build contexts in turn. It returns a goPackage with documentation
+// information for each build context that results in a valid package, in the
+// same order that the build contexts are listed. If none of them result in a
+// package, then loadPackage returns nil, nil.
//
-// If the package is fine except that its documentation is too large, loadPackage
-// returns a package whose err field is a non-nil error with godoc.ErrTooLarge in its chain.
+// If a package is fine except that its documentation is too large, loadPackage
+// returns a goPackage whose err field is a non-nil error with godoc.ErrTooLarge in its chain.
func loadPackage(ctx context.Context, zipGoFiles []*zip.File, innerPath string, sourceInfo *source.Info, modInfo *godoc.ModuleInfo) (_ *goPackage, err error) {
defer derrors.Wrap(&err, "loadPackage(ctx, zipGoFiles, %q, sourceInfo, modInfo)", innerPath)
ctx, span := trace.StartSpan(ctx, "fetch.loadPackage")
defer span.End()
+ var pkgs []*goPackage
for _, env := range goEnvs {
pkg, err := loadPackageWithBuildContext(ctx, env.GOOS, env.GOARCH, zipGoFiles, innerPath, sourceInfo, modInfo)
if err != nil && !errors.Is(err, godoc.ErrTooLarge) && !errors.Is(err, derrors.NotFound) {
@@ -71,10 +73,39 @@
}
if pkg != nil {
pkg.err = err
+ pkgs = append(pkgs, pkg)
+ }
+ }
+ return mergePackages(pkgs)
+}
+
+// mergePackages combines multiple packages from different build contexts into
+// one.
+func mergePackages(pkgs []*goPackage) (*goPackage, error) {
+ if len(pkgs) == 0 {
+ return nil, nil
+ }
+ // If any build context has an error, return that one.
+ for _, pkg := range pkgs {
+ if pkg.err != nil {
return pkg, nil
}
}
- return nil, nil
+
+ // Use everything from the first package, just merge the docs.
+ result := pkgs[0]
+ for _, pkg := range pkgs[1:] {
+ if pkg.name != result.name {
+ // All the build environments should use the same package name. Although
+ // it's technically legal for different build tags to result in different
+ // package names, it's not something we support.
+ return nil, &BadPackageError{
+ Err: fmt.Errorf("more than one package name (%q and %q)", result.name, pkg.name),
+ }
+ }
+ result.docs = append(result.docs, pkg.docs[0])
+ }
+ return result, nil
}
// httpPost allows package fetch tests to stub out playground URL fetches.
@@ -132,14 +163,16 @@
}
v1path := internal.V1Path(importPath, modulePath)
return &goPackage{
- path: importPath,
- name: packageName,
- synopsis: synopsis,
- v1path: v1path,
- imports: imports,
- goos: goos,
- goarch: goarch,
- source: src,
+ path: importPath,
+ name: packageName,
+ v1path: v1path,
+ imports: imports,
+ docs: []*internal.Documentation{{
+ GOOS: goos,
+ GOARCH: goarch,
+ Synopsis: synopsis,
+ Source: src,
+ }},
}, err
}
diff --git a/internal/fetch/load_test.go b/internal/fetch/load_test.go
index 94f90d2..166a369 100644
--- a/internal/fetch/load_test.go
+++ b/internal/fetch/load_test.go
@@ -10,6 +10,9 @@
"testing"
"github.com/google/go-cmp/cmp"
+ "github.com/google/go-cmp/cmp/cmpopts"
+ "golang.org/x/pkgsite/internal"
+ "golang.org/x/pkgsite/internal/godoc"
"golang.org/x/pkgsite/internal/testing/testhelper"
)
@@ -95,3 +98,85 @@
})
}
}
+
+func TestMergePackages(t *testing.T) {
+ doc1 := &internal.Documentation{
+ GOOS: "linux",
+ GOARCH: "amd64",
+ Synopsis: "s1",
+ }
+ doc2 := &internal.Documentation{
+ GOOS: "js",
+ GOARCH: "wasm",
+ Synopsis: "s2",
+ }
+
+ pkg := func(name, imp string, doc *internal.Documentation, err error) *goPackage {
+ return &goPackage{
+ name: name,
+ imports: []string{imp},
+ docs: []*internal.Documentation{doc},
+ err: err,
+ }
+ }
+
+ for _, test := range []struct {
+ name string
+ pkgs []*goPackage
+ want *goPackage
+ wantErr bool
+ }{
+ {
+ name: "no packages",
+ pkgs: nil,
+ want: nil,
+ wantErr: false,
+ },
+ {
+ name: "one package",
+ pkgs: []*goPackage{pkg("name1", "imp1", doc1, nil)},
+ want: pkg("name1", "imp1", doc1, nil),
+ },
+ {
+ name: "two packages",
+ pkgs: []*goPackage{
+ pkg("name1", "imp1", doc1, nil),
+ pkg("name1", "imp2", doc2, nil),
+ },
+ want: &goPackage{
+ name: "name1",
+ imports: []string{"imp1"}, // keep the first one
+ docs: []*internal.Documentation{doc1, doc2}, // keep both, in order
+ },
+ },
+ {
+ name: "one package has err",
+ pkgs: []*goPackage{
+ pkg("name1", "imp1", doc1, nil),
+ pkg("name1", "imp2", doc2, godoc.ErrTooLarge),
+ },
+ want: pkg("name1", "imp2", doc2, godoc.ErrTooLarge), // return one with err
+ },
+ {
+ name: "different names",
+ pkgs: []*goPackage{
+ pkg("name1", "imp1", doc1, nil),
+ pkg("name2", "imp2", doc2, nil),
+ },
+ wantErr: true,
+ },
+ } {
+ t.Run(test.name, func(t *testing.T) {
+ got, err := mergePackages(test.pkgs)
+ if err != nil {
+ if !test.wantErr {
+ t.Fatalf("got %v, want no error", err)
+ }
+ return
+ }
+ if diff := cmp.Diff(test.want, got, cmp.AllowUnexported(goPackage{}), cmpopts.EquateErrors()); diff != "" {
+ t.Errorf("mismatch (-want, +got):\n%s", diff)
+ }
+ })
+ }
+}
diff --git a/internal/fetch/package.go b/internal/fetch/package.go
index b4bed4c..8acee3b 100644
--- a/internal/fetch/package.go
+++ b/internal/fetch/package.go
@@ -30,20 +30,14 @@
type goPackage struct {
path string
name string
- synopsis string
imports []string
isRedistributable bool
licenseMeta []*licenses.Metadata // metadata of applicable licenses
- // goos and goarch are environment variables used to parse the
- // package.
- goos string
- goarch string
-
// v1path is the package path of a package with major version 1 in a given
// series.
v1path string
- source []byte // the source files of the package, for generating doc at serving time
- err error // non-fatal error when loading the package (e.g. documentation is too large)
+ docs []*internal.Documentation // doc for different build contexts
+ err error // non-fatal error when loading the package (e.g. documentation is too large)
}
// extractPackagesFromZip returns a slice of packages from the module zip r.
diff --git a/internal/fetch/unit.go b/internal/fetch/unit.go
index a33f7f1..7bce5bb 100644
--- a/internal/fetch/unit.go
+++ b/internal/fetch/unit.go
@@ -61,12 +61,8 @@
if pkg, ok := pkgLookup[dirPath]; ok {
dir.Name = pkg.name
dir.Imports = pkg.imports
- dir.Documentation = &internal.Documentation{
- GOOS: pkg.goos,
- GOARCH: pkg.goarch,
- Synopsis: pkg.synopsis,
- Source: pkg.source,
- }
+ // TODO(golang/go#37232): keep all docs
+ dir.Documentation = pkg.docs[0]
}
units = append(units, dir)
}
diff --git a/internal/fetch/unit_test.go b/internal/fetch/unit_test.go
index b14f862..1bd8440 100644
--- a/internal/fetch/unit_test.go
+++ b/internal/fetch/unit_test.go
@@ -85,12 +85,14 @@
name: path.Base(p),
path: p,
v1path: internal.V1Path(p, modulePath),
- synopsis: sample.Synopsis,
isRedistributable: true,
licenseMeta: sample.LicenseMetadata,
imports: sample.Imports,
- goos: sample.GOOS,
- goarch: sample.GOARCH,
+ docs: []*internal.Documentation{{
+ GOOS: sample.GOOS,
+ GOARCH: sample.GOARCH,
+ Synopsis: sample.Synopsis,
+ }},
}
}
