From 8b55dd4bd292f6b7666d9b84f0e470632e3da2a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Mart=C3=AD?= Date: Sat, 12 Mar 2022 16:23:19 +0000 Subject: [PATCH] work around a build cache regression in the previous commit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The added comment in main.go explains the situation in detail. The added test is a minimization of the scenario, which failed: > cd mod1 > garble -seed=${SEED1} build -v gopkg.in/garbletest.v2 > cd ../mod2 > garble -seed=${SEED1} build -v [stderr] test/main/mod2 # test/main/mod2 cannot load garble export file for gopkg.in/garbletest.v2: open […]/go-build/ed/[…]-garble-ZV[…]-d: no such file or directory To work around the problem, we'll always add each package's GarbleActionID to its build artifact, even when not using -seed. This will get us the previous behavior with regards to the build cache, meaning that we fix the recent regression. The added variable doesn't make it to the final binary either. While here, improve the cached file loading error's context, and add an extra sanity check for duplicates on ListedPackages. --- main.go | 22 +++++- shared.go | 5 +- .../mod/gopkg.in_garbletest.v2_v2.999.0.txt | 14 +++- testdata/mod/rsc.io_sampler_v1.3.0.txt | 4 + testdata/mod/rsc.io_sampler_v1.99.99.txt | 4 + testdata/scripts/imports.txt | 6 +- testdata/scripts/seed-cache.txt | 74 +++++++++++++++++++ testdata/scripts/seed.txt | 8 +- 8 files changed, 129 insertions(+), 8 deletions(-) create mode 100644 testdata/scripts/seed-cache.txt diff --git a/main.go b/main.go index c39b189..197ef45 100644 --- a/main.go +++ b/main.go @@ -735,6 +735,26 @@ func transformCompile(args []string) ([]string, error) { if err != nil { return nil, err } + // It is possible to end up in an edge case where two instances of the + // same package have different Action IDs, but their obfuscation and + // builds produce exactly the same results. + // In such an edge case, Go's build cache is smart enough for the second + // instance to reuse the first's build artifact. + // However, garble's caching via garbleExportFile is not as smart, + // as we base the location of these files purely based on Action IDs. + // Thus, the incremental build can fail to find garble's cached file. + // To sidestep this bug entirely, ensure that different action IDs never + // produce the same cached output when building with garble. + // Note that this edge case tends to happen when a -seed is provided, + // as then a package's Action ID is not used as an obfuscation seed. + // TODO(mvdan): replace this workaround with an actual fix if we can. + // This workaround is presumably worse on the build cache, + // as we end up with extra near-duplicate cached artifacts. + if i == 0 { + src = append(src, fmt.Sprintf( + "\nvar garbleActionID = %q\n", hashToString(curPkg.GarbleActionID), + )...) + } // Uncomment for some quick debugging. Do not delete. // if curPkg.ToObfuscate { @@ -1027,7 +1047,7 @@ func loadCachedOutputs() error { return nil }() if err != nil { - return err + return fmt.Errorf("cannot load garble export file for %s: %w", path, err) } loaded++ } diff --git a/shared.go b/shared.go index d95ac65..c770f27 100644 --- a/shared.go +++ b/shared.go @@ -209,7 +209,10 @@ func appendListedPackages(packages []string, withDeps bool) error { if err := dec.Decode(&pkg); err != nil { return err } - if pkg.Export != "" { + if cache.ListedPackages[pkg.ImportPath] != nil { + return fmt.Errorf("duplicate package: %q", pkg.ImportPath) + } + if pkg.BuildID != "" { actionID := decodeHash(splitActionID(pkg.BuildID)) pkg.GarbleActionID = addGarbleToHash(actionID) } diff --git a/testdata/mod/gopkg.in_garbletest.v2_v2.999.0.txt b/testdata/mod/gopkg.in_garbletest.v2_v2.999.0.txt index 308e50a..41f4a1e 100644 --- a/testdata/mod/gopkg.in_garbletest.v2_v2.999.0.txt +++ b/testdata/mod/gopkg.in_garbletest.v2_v2.999.0.txt @@ -3,10 +3,22 @@ module gopkg.in/garbletest.v2@v2.999.0 -- .mod -- module gopkg.in/garbletest.v2 -go 1.15 +go 1.17 + +require "rsc.io/sampler" v1.3.0 -- .info -- {"Version":"v2.999.0","Time":"2020-11-17T15:46:20Z"} +-- go.mod -- +module gopkg.in/garbletest.v2 + +go 1.17 + +require "rsc.io/sampler" v1.3.0 -- apic.go -- package garbletest +import "rsc.io/sampler/unchanged" + func Test() { println("this is the dummy garbletest.v2 package") } + +var _ = unchanged.Foo diff --git a/testdata/mod/rsc.io_sampler_v1.3.0.txt b/testdata/mod/rsc.io_sampler_v1.3.0.txt index febe51f..5a316bb 100644 --- a/testdata/mod/rsc.io_sampler_v1.3.0.txt +++ b/testdata/mod/rsc.io_sampler_v1.3.0.txt @@ -200,3 +200,7 @@ func (t *text) find(prefs []language.Tag) string { } return s } +-- unchanged/unchanged.go -- +package unchanged + +const Foo = 4 diff --git a/testdata/mod/rsc.io_sampler_v1.99.99.txt b/testdata/mod/rsc.io_sampler_v1.99.99.txt index e89cacc..e4dbba0 100644 --- a/testdata/mod/rsc.io_sampler_v1.99.99.txt +++ b/testdata/mod/rsc.io_sampler_v1.99.99.txt @@ -138,3 +138,7 @@ func (t *text) find(prefs []language.Tag) string { } return s } +-- unchanged/unchanged.go -- +package unchanged + +const Foo = 4 diff --git a/testdata/scripts/imports.txt b/testdata/scripts/imports.txt index 4d83b75..246ef84 100644 --- a/testdata/scripts/imports.txt +++ b/testdata/scripts/imports.txt @@ -60,11 +60,11 @@ require ( -- go.sum -- golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c h1:pvCbr/wm8HzDD3fVywevekufpn6tCGPY3spdHeZJEsw= golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= -gopkg.in/garbletest.v2 v2.999.0 h1:wiZfOKGiXX7DoYVgbNvnTaCjqElrpZQSvKg0HYouw/o= -gopkg.in/garbletest.v2 v2.999.0/go.mod h1:MF1BPTBjmDdc9x86+9UMLL9pAH2eMFPHvltohOvlGEw= +gopkg.in/garbletest.v2 v2.999.0 h1:XHlBQi3MAcJL2fjNiEPAPAilkzc7hAv4vyyjY5w+IUY= +gopkg.in/garbletest.v2 v2.999.0/go.mod h1:MI9QqKJD8i8oL8mW/bR0qq19/VuezEdJbVvl2B8Pa40= rsc.io/quote v1.5.2 h1:3fEykkD9k7lYzXqCYrwGAf7iNhbk4yCjHmKBN9td4L0= rsc.io/quote v1.5.2/go.mod h1:LzX7hefJvL54yjefDEDHNONDjII0t9xZLPXsUe+TKr0= -rsc.io/sampler v1.3.0 h1:HLGR/BgEtI3r0uymSP/nl2uPLsUnNJX8toRyhfpBTII= +rsc.io/sampler v1.3.0 h1:+lXbM7nYGGOYhnMEiMtjCwcUfjn4sajeMm15HMT6SnU= rsc.io/sampler v1.3.0/go.mod h1:T1hPZKmBbMNahiBKFy5HrXp6adAjACjK9JXDnKaTXpA= rsc.io/testonly v1.0.0 h1:K/VWHdO+Jv7woUXG0GzVNx1czBXUt3Ib1deaMn+xk64= rsc.io/testonly v1.0.0/go.mod h1:OqmGbIFOcF+XrFReLOGZ6BhMM7uMBiQwZsyNmh74SzY= diff --git a/testdata/scripts/seed-cache.txt b/testdata/scripts/seed-cache.txt new file mode 100644 index 0000000..0cb945a --- /dev/null +++ b/testdata/scripts/seed-cache.txt @@ -0,0 +1,74 @@ +# Regression test for a build cache edge case. +# Both mod1 and mod2 build the same version of gopkg.in/garbletest.v2, +# but with different versions of one of its deps, rsc.io/quote. +# However, the change in quote's version does not actually change the build. +# This results in mod2's build never recompiling garbletest.v2, +# even though its cached build from mod1 has a different Action ID hash. +# In the past, this threw off garble's extra cached gob files. + +env SEED1=OQg9kACEECQ +env GOGARBLE=* + +# First, ensure that mod1's garbletest.v2 is in the cache. +cd mod1 +garble -seed=${SEED1} build gopkg.in/garbletest.v2 + +# We collect the Action IDs to ensure they're different. +# This is one of the factors that confused garble. +go list -trimpath -export -f '{{.BuildID}}' gopkg.in/garbletest.v2 +cp stdout ../buildid-mod1 + +# Then, do the mod2 build, using the different-but-equal garbletest.v2. +# Ensure that our workaround's inserted garbleActionID does not end up in the binary. +cd ../mod2 +garble -seed=${SEED1} build +! binsubstr mod2$exe 'garbleActionID' + +go list -trimpath -export -f '{{.BuildID}}' gopkg.in/garbletest.v2 +cp stdout ../buildid-mod2 + +cd .. + +! bincmp buildid-mod1 buildid-mod2 + +-- mod1/go.mod -- +module test/main/mod1 + +go 1.17 + +require gopkg.in/garbletest.v2 v2.999.0 + +require rsc.io/sampler v1.3.0 // indirect +-- mod1/go.sum -- +golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= +gopkg.in/garbletest.v2 v2.999.0 h1:XHlBQi3MAcJL2fjNiEPAPAilkzc7hAv4vyyjY5w+IUY= +gopkg.in/garbletest.v2 v2.999.0/go.mod h1:MI9QqKJD8i8oL8mW/bR0qq19/VuezEdJbVvl2B8Pa40= +rsc.io/sampler v1.3.0 h1:+lXbM7nYGGOYhnMEiMtjCwcUfjn4sajeMm15HMT6SnU= +rsc.io/sampler v1.3.0/go.mod h1:T1hPZKmBbMNahiBKFy5HrXp6adAjACjK9JXDnKaTXpA= +-- mod1/pkg.go -- +package main + +import garbletest "gopkg.in/garbletest.v2" + +func main() { garbletest.Test() } +-- mod2/go.mod -- +module test/main/mod2 + +go 1.17 + +require gopkg.in/garbletest.v2 v2.999.0 + +require rsc.io/sampler v1.99.99 // indirect +-- mod2/go.sum -- +golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= +gopkg.in/garbletest.v2 v2.999.0 h1:XHlBQi3MAcJL2fjNiEPAPAilkzc7hAv4vyyjY5w+IUY= +gopkg.in/garbletest.v2 v2.999.0/go.mod h1:MI9QqKJD8i8oL8mW/bR0qq19/VuezEdJbVvl2B8Pa40= +rsc.io/sampler v1.3.0/go.mod h1:T1hPZKmBbMNahiBKFy5HrXp6adAjACjK9JXDnKaTXpA= +rsc.io/sampler v1.99.99 h1:fz0uBgsEGkv94x3b3GDw3Tvhj6yint6lYdsQOnFXNuw= +rsc.io/sampler v1.99.99/go.mod h1:T1hPZKmBbMNahiBKFy5HrXp6adAjACjK9JXDnKaTXpA= +-- mod2/pkg.go -- +package main + +import garbletest "gopkg.in/garbletest.v2" + +func main() { garbletest.Test() } diff --git a/testdata/scripts/seed.txt b/testdata/scripts/seed.txt index 21b1480..dd53e3e 100644 --- a/testdata/scripts/seed.txt +++ b/testdata/scripts/seed.txt @@ -2,7 +2,11 @@ env GOGARBLE=test/main # Note that in this test we use "! bincmp" on plaintext output files, # as a workaround for "cmp" not supporting "! cmp". +# TODO: now that obfuscation with -seed is deterministic, +# can we just rely on the regular "cmp" with fixed output files? +# TODO: consider setting these seeds globally, +# so we can reuse them across tests and make better use of the shared build cache. env SEED1=OQg9kACEECQ env SEED2=NruiDmVz6/s @@ -11,7 +15,7 @@ garble -seed=${SEED1} build exec ./main$exe cmp stderr main.stderr binsubstr main$exe 'teststring' 'imported var value' -! binsubstr main$exe 'ImportedVar' +! binsubstr main$exe 'ImportedVar' ${SEED1} [short] stop # the extra checks are relatively expensive @@ -23,7 +27,7 @@ cp stderr importedpkg-seed-static-1 cp main$exe main_seed1$exe rm main$exe garble -seed=${SEED1}= build -v -#! stderr . +! stderr . bincmp main$exe main_seed1$exe exec ./main$exe test/main/imported