From 35df1d2cd0dfadefb1b91abb111ba1ea79964e27 Mon Sep 17 00:00:00 2001 From: Regen Van Walbeek Date: Tue, 22 Mar 2022 08:16:17 -0500 Subject: [PATCH] Fix: Don't bust cache so aggressively when compiling test sources --- build/build.go | 17 ++++++------ build/cache/cache.go | 24 ++++++++--------- build/cache/cache_test.go | 56 ++++++++++++++++++++++++++++++++++----- 3 files changed, 69 insertions(+), 28 deletions(-) diff --git a/build/build.go b/build/build.go index f343d1b69..ac4acee7c 100644 --- a/build/build.go +++ b/build/build.go @@ -458,13 +458,12 @@ func NewSession(options *Options) (*Session, error) { } s.xctx = NewBuildContext(s.InstallSuffix(), s.options.BuildTags) s.buildCache = cache.BuildCache{ - GOOS: s.xctx.GOOS(), - GOARCH: "js", - GOROOT: options.GOROOT, - GOPATH: options.GOPATH, - BuildTags: options.BuildTags, - Minify: options.Minify, - TestedPackage: options.TestedPackage, + GOOS: s.xctx.GOOS(), + GOARCH: "js", + GOROOT: options.GOROOT, + GOPATH: options.GOPATH, + BuildTags: options.BuildTags, + Minify: options.Minify, } s.Types = make(map[string]*types.Package) if options.Watch { @@ -608,7 +607,7 @@ func (s *Session) BuildPackage(pkg *PackageData) (*compiler.Archive, error) { } if !s.options.NoCache { - archive := s.buildCache.LoadArchive(pkg.ImportPath) + archive := s.buildCache.LoadArchive(pkg.ImportPath, s.options.TestedPackage) if archive != nil && !pkg.SrcModTime.After(archive.BuildTime) { if err := archive.RegisterTypes(s.Types); err != nil { panic(fmt.Errorf("Failed to load type information from %v: %w", archive, err)) @@ -649,7 +648,7 @@ func (s *Session) BuildPackage(pkg *PackageData) (*compiler.Archive, error) { fmt.Println(pkg.ImportPath) } - s.buildCache.StoreArchive(archive) + s.buildCache.StoreArchive(archive, s.options.TestedPackage) s.UpToDateArchives[pkg.ImportPath] = archive return archive, nil diff --git a/build/cache/cache.go b/build/cache/cache.go index 79e0471cf..c72db2c32 100644 --- a/build/cache/cache.go +++ b/build/cache/cache.go @@ -58,7 +58,7 @@ func Clear() error { // the cache. For example, any artifacts that were cached for a minified build // must not be reused for a non-minified build. GopherJS version change also // invalidates the cache. It is callers responsibility to ensure that artifacts -// passed the the StoreArchive function were generated with the same build +// passed to the StoreArchive function were generated with the same build // parameters as the cache is configured. // // There is no upper limit for the total cache size. It can be cleared @@ -77,11 +77,6 @@ type BuildCache struct { GOPATH string BuildTags []string Minify bool - // When building for tests, import path of the package being tested. The - // package under test is built with *_test.go sources included, and since it - // may be imported by other packages in the binary we can't reuse the "normal" - // cache. - TestedPackage string } func (bc BuildCache) String() string { @@ -90,11 +85,11 @@ func (bc BuildCache) String() string { // StoreArchive compiled archive in the cache. Any error inside this method // will cause the cache not to be persisted. -func (bc *BuildCache) StoreArchive(a *compiler.Archive) { +func (bc *BuildCache) StoreArchive(a *compiler.Archive, testedPackage string) { if bc == nil { return // Caching is disabled. } - path := cachedPath(bc.archiveKey(a.ImportPath)) + path := cachedPath(bc.archiveKey(a.ImportPath, testedPackage)) if err := os.MkdirAll(filepath.Dir(path), 0750); err != nil { log.Warningf("Failed to create build cache directory: %v", err) return @@ -125,11 +120,11 @@ func (bc *BuildCache) StoreArchive(a *compiler.Archive) { // // The returned archive would have been built with the same configuration as // the build cache was. -func (bc *BuildCache) LoadArchive(importPath string) *compiler.Archive { +func (bc *BuildCache) LoadArchive(importPath string, testedPackage string) *compiler.Archive { if bc == nil { return nil // Caching is disabled. } - path := cachedPath(bc.archiveKey(importPath)) + path := cachedPath(bc.archiveKey(importPath, testedPackage)) f, err := os.Open(path) if err != nil { if os.IsNotExist(err) { @@ -156,6 +151,11 @@ func (bc *BuildCache) commonKey() string { } // archiveKey returns a full cache key for a package's compiled archive. -func (bc *BuildCache) archiveKey(importPath string) string { - return path.Join("archive", bc.commonKey(), importPath) +func (bc *BuildCache) archiveKey(importPath string, testedPackage string) string { + // When building for tests, testedPackage is import path of the package being tested. + // testedPackage is built with *_test.go sources included, and since it + // may be imported by other packages in the binary we can't reuse the "normal" + // cache. + compiledWithTests := testedPackage == importPath + return path.Join("archive", bc.commonKey(), importPath, fmt.Sprintf("compiledWithTests: %v", compiledWithTests)) } diff --git a/build/cache/cache_test.go b/build/cache/cache_test.go index fd89ec187..b7dc4e009 100644 --- a/build/cache/cache_test.go +++ b/build/cache/cache_test.go @@ -16,20 +16,20 @@ func TestStore(t *testing.T) { } bc := BuildCache{} - if got := bc.LoadArchive(want.ImportPath); got != nil { + if got := bc.LoadArchive(want.ImportPath, ""); got != nil { t.Errorf("Got: %s was found in the cache. Want: empty cache.", got.ImportPath) } - bc.StoreArchive(want) - got := bc.LoadArchive(want.ImportPath) + bc.StoreArchive(want, "") + got := bc.LoadArchive(want.ImportPath, "") if got == nil { - t.Errorf("Got: %s wan not found in the cache. Want: archive is can be loaded after store.", want.ImportPath) + t.Errorf("Got: %s was not found in the cache. Want: archive can be loaded after store.", want.ImportPath) } if diff := cmp.Diff(want, got); diff != "" { t.Errorf("Loaded archive is different from stored (-want,+got):\n%s", diff) } // Make sure the package names are a part of the cache key. - if got := bc.LoadArchive("fake/other"); got != nil { + if got := bc.LoadArchive("fake/other", ""); got != nil { t.Errorf("Got: fake/other was found in cache: %#v. Want: nil for packages that weren't cached.", got) } } @@ -61,15 +61,57 @@ func TestInvalidation(t *testing.T) { for _, test := range tests { a := &compiler.Archive{ImportPath: "package/fake"} - test.cache1.StoreArchive(a) + test.cache1.StoreArchive(a, "") - if got := test.cache2.LoadArchive(a.ImportPath); got != nil { + if got := test.cache2.LoadArchive(a.ImportPath, ""); got != nil { t.Logf("-cache1,+cache2:\n%s", cmp.Diff(test.cache1, test.cache2)) t.Errorf("Got: %v loaded from cache. Want: build parameter change invalidates cache.", got) } } } +func TestDoNotReuseCacheWhenLoadingPackageForTest(t *testing.T) { + cacheForTest(t) + + sharedPkgArchive := &compiler.Archive{ + ImportPath: "fake/sharedTestPkg", + } + + bc := BuildCache{} + if got := bc.LoadArchive(sharedPkgArchive.ImportPath, "fake/pkg1"); got != nil { + t.Errorf("Got: %s was found in the cache. Want: empty cache.", got.ImportPath) + } + bc.StoreArchive(sharedPkgArchive, "fake/package_1") + + // sharedPkgArchive has not been stored with *_test.go sources, so it cannot be reused + if got := bc.LoadArchive(sharedPkgArchive.ImportPath, sharedPkgArchive.ImportPath); got != nil { + t.Errorf("Got: %s was found in the cache. Want: empty cache.", got.ImportPath) + } +} + +func TestDifferentSourcesCanShareSameArchive(t *testing.T) { + cacheForTest(t) + + sharedPkgArchive := &compiler.Archive{ + ImportPath: "fake/sharedTestPkg", + } + + bc := BuildCache{} + if got := bc.LoadArchive(sharedPkgArchive.ImportPath, "fake/pkg1"); got != nil { + t.Errorf("Got: %s was found in the cache. Want: empty cache.", got.ImportPath) + } + bc.StoreArchive(sharedPkgArchive, "fake/package_1") + + // sharedPkgArchive has been stored without *_test.go sources, so it can be reused + if got := bc.LoadArchive(sharedPkgArchive.ImportPath, "fake/pkg2"); got == nil { + t.Errorf("Got: %s was not found in the cache. Want: archive can be loaded after store.", sharedPkgArchive.ImportPath) + } + + if got := bc.LoadArchive(sharedPkgArchive.ImportPath, ""); got == nil { + t.Errorf("Got: %s was not found in the cache. Want: archive can be loaded after store.", sharedPkgArchive.ImportPath) + } +} + func cacheForTest(t *testing.T) { t.Helper() originalRoot := cacheRoot