Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit dce1a93

Browse files
committedMar 19, 2025
mirror: fix non-determinism in case two packages have the same path
This code previously assumed that `go mod download -json` would not produce two different versions of the same dependency with the same path. This is typically a sensible assumption but no longer holds in some niche scenarios. We use [replace](https://github.com/cockroachdb/cockroach/blob/65b2ed4fbdf5502f3fbe0af4ddbd30a7ac7eabb4/go.mod#L500) in `go.mod` to effectively import two different versions of the same dependency with the same path. This results in non-determinism in the mirroring code with respect to which version of the dependency we select. We now disambiguate with a path/version pair, which will be unique. We also add some additional validation to check assumptions so if these assumptions are ever broken in the future, the tool will fail loudly instead of proceeding silently and performing a potentially harmful operation. Fixes cockroachdb#143168 Epic: CRDB-17171 Release note: None
1 parent 65b2ed4 commit dce1a93

File tree

1 file changed

+41
-6
lines changed

1 file changed

+41
-6
lines changed
 

‎pkg/cmd/mirror/go/mirror.go

+41-6
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,11 @@ import (
3030

3131
const gcpBucket = "cockroach-godeps"
3232

33+
type versionedDependency struct {
34+
Path string
35+
Version string
36+
}
37+
3338
// downloadedModule captures `go mod download -json` output.
3439
type downloadedModule struct {
3540
Path string `json:"Path"`
@@ -134,7 +139,7 @@ func createTmpDir() (tmpdir string, err error) {
134139

135140
func downloadZips(
136141
tmpdir string, listed map[string]listedModule,
137-
) (map[string]downloadedModule, error) {
142+
) (map[versionedDependency]downloadedModule, error) {
138143
gobin, err := bazel.Runfile("bin/go")
139144
if err != nil {
140145
return nil, err
@@ -160,15 +165,22 @@ func downloadZips(
160165
downloadArgs, string(jsonBytes), string(stderr), err)
161166
}
162167
var jsonBuilder strings.Builder
163-
ret := make(map[string]downloadedModule)
168+
ret := make(map[versionedDependency]downloadedModule)
164169
for _, line := range strings.Split(string(jsonBytes), "\n") {
165170
jsonBuilder.WriteString(line)
166171
if strings.HasPrefix(line, "}") {
167172
var mod downloadedModule
168173
if err := json.Unmarshal([]byte(jsonBuilder.String()), &mod); err != nil {
169174
return nil, err
170175
}
171-
ret[mod.Path] = mod
176+
key := versionedDependency{
177+
Path: mod.Path,
178+
Version: mod.Version,
179+
}
180+
if _, ok := ret[key]; ok {
181+
panic(fmt.Sprintf("found entry in `go mod download -json` with duplicate key %+v", key))
182+
}
183+
ret[key] = mod
172184
jsonBuilder.Reset()
173185
}
174186
}
@@ -206,6 +218,19 @@ func listAllModules(tmpdir string) (map[string]listedModule, error) {
206218
if mod.Path == "github.com/cockroachdb/cockroach" {
207219
continue
208220
}
221+
// Sanity check: we expect the paths for all modules in
222+
// this set to be unique. This is true of the output of
223+
// `go list` but notably NOT the output of `go mod download`!!!
224+
// `go list` lists modules by their imported names,
225+
// and `go mod download` lists modules by their "real"
226+
// names. For example, if you do `replace A => B`, the
227+
// imported name is A but the "real" name is B. The
228+
// imported name (A) is unique, but the "real" name is
229+
// not. We can import B many times under different
230+
// imported names.
231+
if _, ok := ret[mod.Path]; ok {
232+
panic(fmt.Sprintf("found duplicate imported path: %s. This is a bug. Go tell dev-inf about it.", mod.Path))
233+
}
209234
ret[mod.Path] = mod
210235
}
211236
}
@@ -307,7 +332,7 @@ func dumpBuildNamingConventionArgsForRepo(repoName string) {
307332

308333
func dumpNewDepsBzl(
309334
listed map[string]listedModule,
310-
downloaded map[string]downloadedModule,
335+
downloaded map[versionedDependency]downloadedModule,
311336
existingMirrors map[string]starlarkutil.DownloadableArtifact,
312337
) error {
313338
var sorted []string
@@ -363,6 +388,10 @@ def go_deps():
363388
replaced := &mod
364389
if mod.Replace != nil {
365390
replaced = mod.Replace
391+
// Sanity check: there should not be multiple levels of "replace".
392+
if replaced.Replace != nil {
393+
panic(fmt.Sprintf("replaced module %s is replaced itself?? This is a bug. Go talk to dev-inf.", replaced.Replace))
394+
}
366395
}
367396
fmt.Printf(` go_repository(
368397
name = "%s",
@@ -387,7 +416,10 @@ def go_deps():
387416
`, oldMirror.Sha256, replaced.Path, replaced.Version, oldMirror.URL)
388417
} else if canMirror() {
389418
// We'll have to mirror our copy of the zip ourselves.
390-
d := downloaded[replaced.Path]
419+
d := downloaded[versionedDependency{
420+
Path: replaced.Path,
421+
Version: replaced.Version,
422+
}]
391423
sha, err := getSha256OfFile(d.Zip)
392424
if err != nil {
393425
return fmt.Errorf("could not get zip for %v: %w", *replaced, err)
@@ -404,7 +436,10 @@ def go_deps():
404436
} else {
405437
// We don't have a mirror and can't upload one, so just
406438
// have Gazelle pull the repo for us.
407-
d := downloaded[replaced.Path]
439+
d := downloaded[versionedDependency{
440+
Path: replaced.Path,
441+
Version: replaced.Version,
442+
}]
408443
if mod.Replace != nil {
409444
fmt.Printf(" replace = \"%s\",\n", replaced.Path)
410445
}

0 commit comments

Comments
 (0)
Please sign in to comment.