From ee4fbbc6211cd978f199dd26ab73ff72cc8d95fd Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Fri, 9 Jun 2017 15:46:47 -0400 Subject: [PATCH] cmd/go: stop creating nested temp directory trees Now that we have -importcfg, there's no need for the temporary directory trees that mirror the import path structure, and we can drop a bunch of complex code that was building and maintaining that structure. This should fix "file name too long" errors on systems with low limits. (For example #10651 and #17070, although we fixed those by adding code to deal with very long file names on Windows instead.) Change-Id: I11e221c6c1edeb81c3b2f1d89988f5235aa2bbb9 Reviewed-on: https://go-review.googlesource.com/56280 Reviewed-by: Ian Lance Taylor --- src/cmd/go/internal/test/test.go | 100 ++++++++++----------------- src/cmd/go/internal/work/build.go | 109 ++++++++++-------------------- 2 files changed, 72 insertions(+), 137 deletions(-) diff --git a/src/cmd/go/internal/test/test.go b/src/cmd/go/internal/test/test.go index bcb659b131..6fed6ac837 100644 --- a/src/cmd/go/internal/test/test.go +++ b/src/cmd/go/internal/test/test.go @@ -773,29 +773,6 @@ func builderTest(b *work.Builder, p *load.Package) (buildAction, runAction, prin } testBinary := elem + ".test" - // The ptest package needs to be importable under the - // same import path that p has, but we cannot put it in - // the usual place in the temporary tree, because then - // other tests will see it as the real package. - // Instead we make a _test directory under the import path - // and then repeat the import path there. We tell the - // compiler and linker to look in that _test directory first. - // - // That is, if the package under test is unicode/utf8, - // then the normal place to write the package archive is - // $WORK/unicode/utf8.a, but we write the test package archive to - // $WORK/unicode/utf8/_test/unicode/utf8.a. - // We write the external test package archive to - // $WORK/unicode/utf8/_test/unicode/utf8_test.a. - testDir := filepath.Join(b.WorkDir, filepath.FromSlash(p.ImportPath+"/_test")) - ptestObj := work.BuildToolchain.Pkgpath(testDir, p) - - // Create the directory for the .a files. - ptestDir, _ := filepath.Split(ptestObj) - if err := b.Mkdir(ptestDir); err != nil { - return nil, nil, nil, err - } - // Should we apply coverage analysis locally, // only for this package and only for this test? // Yes, if -cover is on but -coverpkg has not specified @@ -812,7 +789,6 @@ func builderTest(b *work.Builder, p *load.Package) (buildAction, runAction, prin ptest.Internal.Target = "" ptest.Imports = str.StringList(p.Imports, p.TestImports) ptest.Internal.Imports = append(append([]*load.Package{}, p.Internal.Imports...), imports...) - ptest.Internal.Pkgdir = testDir ptest.Internal.Fake = true ptest.Internal.ForceLibrary = true ptest.Stale = true @@ -857,7 +833,6 @@ func builderTest(b *work.Builder, p *load.Package) (buildAction, runAction, prin ImportPos: p.Internal.Build.XTestImportPos, }, Imports: ximports, - Pkgdir: testDir, Fake: true, External: true, }, @@ -867,19 +842,23 @@ func builderTest(b *work.Builder, p *load.Package) (buildAction, runAction, prin } } + testDir := b.NewObjdir() + if err := b.Mkdir(testDir); err != nil { + return nil, nil, nil, err + } + // Action for building pkg.test. pmain = &load.Package{ PackagePublic: load.PackagePublic{ Name: "main", Dir: testDir, GoFiles: []string{"_testmain.go"}, - ImportPath: "testmain", + ImportPath: p.ImportPath + " (testmain)", Root: p.Root, Stale: true, }, Internal: load.PackageInternal{ Build: &build.Package{Name: "main"}, - Pkgdir: testDir, Fake: true, OmitDebug: !testC && !testNeedBinary, }, @@ -942,18 +921,21 @@ func builderTest(b *work.Builder, p *load.Package) (buildAction, runAction, prin if ptest != p && localCover { // We have made modifications to the package p being tested - // and are rebuilding p (as ptest), writing it to the testDir tree. - // Arrange to rebuild, writing to that same tree, all packages q - // such that the test depends on q, and q depends on p. + // and are rebuilding p (as ptest). + // Arrange to rebuild all packages q such that + // the test depends on q and q depends on p. // This makes sure that q sees the modifications to p. // Strictly speaking, the rebuild is only necessary if the // modifications to p change its export metadata, but // determining that is a bit tricky, so we rebuild always. + // TODO(rsc): Once we get export metadata changes + // handled properly, look into the expense of dropping + // "&& localCover" above. // // This will cause extra compilation, so for now we only do it // when testCover is set. The conditions are more general, though, // and we may find that we need to do it always in the future. - recompileForTest(pmain, p, ptest, testDir) + recompileForTest(pmain, p, ptest) } t.NeedCgo = forceCgo @@ -965,9 +947,9 @@ func builderTest(b *work.Builder, p *load.Package) (buildAction, runAction, prin } if !cfg.BuildN { - // writeTestmain writes _testmain.go. This must happen after recompileForTest, - // because recompileForTest modifies XXX. - if err := writeTestmain(filepath.Join(testDir, "_testmain.go"), t); err != nil { + // writeTestmain writes _testmain.go, + // using the test description gathered in t. + if err := writeTestmain(testDir+"_testmain.go", t); err != nil { return nil, nil, nil, err } } @@ -976,23 +958,11 @@ func builderTest(b *work.Builder, p *load.Package) (buildAction, runAction, prin if ptest != p { a := b.Action(work.ModeBuild, work.ModeBuild, ptest) - a.Objdir = testDir + string(filepath.Separator) + "_obj_test" + string(filepath.Separator) - a.Objpkg = ptestObj - a.Target = ptestObj a.Link = false } - if pxtest != nil { - a := b.Action(work.ModeBuild, work.ModeBuild, pxtest) - a.Objdir = testDir + string(filepath.Separator) + "_obj_xtest" + string(filepath.Separator) - a.Objpkg = work.BuildToolchain.Pkgpath(testDir, pxtest) - a.Target = a.Objpkg - } - a := b.Action(work.ModeBuild, work.ModeBuild, pmain) - a.Objdir = testDir + string(filepath.Separator) - a.Objpkg = filepath.Join(testDir, "main.a") - a.Target = filepath.Join(testDir, testBinary) + cfg.ExeSuffix + a.Target = testDir + testBinary + cfg.ExeSuffix if cfg.Goos == "windows" { // There are many reserved words on Windows that, // if used in the name of an executable, cause Windows @@ -1018,7 +988,7 @@ func builderTest(b *work.Builder, p *load.Package) (buildAction, runAction, prin // we could just do this always on Windows. for _, bad := range windowsBadWords { if strings.Contains(testBinary, bad) { - a.Target = filepath.Join(testDir, "test.test") + cfg.ExeSuffix + a.Target = testDir + "test.test" + cfg.ExeSuffix break } } @@ -1056,6 +1026,7 @@ func builderTest(b *work.Builder, p *load.Package) (buildAction, runAction, prin Func: builderCleanTest, Deps: []*work.Action{runAction}, Package: p, + Objdir: testDir, } printAction = &work.Action{ Func: builderPrintTest, @@ -1085,7 +1056,7 @@ Search: return stk } -func recompileForTest(pmain, preal, ptest *load.Package, testDir string) { +func recompileForTest(pmain, preal, ptest *load.Package) { // The "test copy" of preal is ptest. // For each package that depends on preal, make a "test copy" // that depends on ptest. And so on, up the dependency tree. @@ -1098,19 +1069,19 @@ func recompileForTest(pmain, preal, ptest *load.Package, testDir string) { return } didSplit = true - if p.Internal.Pkgdir != testDir { - p1 := new(load.Package) - testCopy[p] = p1 - *p1 = *p - p1.Internal.Imports = make([]*load.Package, len(p.Internal.Imports)) - copy(p1.Internal.Imports, p.Internal.Imports) - p = p1 - p.Internal.Pkgdir = testDir - p.Internal.Target = "" - p.Internal.Fake = true - p.Stale = true - p.StaleReason = "depends on package being tested" + if testCopy[p] != nil { + panic("recompileForTest loop") } + p1 := new(load.Package) + testCopy[p] = p1 + *p1 = *p + p1.Internal.Imports = make([]*load.Package, len(p.Internal.Imports)) + copy(p1.Internal.Imports, p.Internal.Imports) + p = p1 + p.Internal.Target = "" + p.Internal.Fake = true + p.Stale = true + p.StaleReason = "depends on package being tested" } // Update p.Internal.Imports to use test copies. @@ -1288,9 +1259,10 @@ func builderCleanTest(b *work.Builder, a *work.Action) error { if cfg.BuildWork { return nil } - run := a.Deps[0] - testDir := filepath.Join(b.WorkDir, filepath.FromSlash(run.Package.ImportPath+"/_test")) - os.RemoveAll(testDir) + if cfg.BuildX { + b.Showcmd("", "rm -r %s", a.Objdir) + } + os.RemoveAll(a.Objdir) return nil } diff --git a/src/cmd/go/internal/work/build.go b/src/cmd/go/internal/work/build.go index 0300831ebf..932626d060 100644 --- a/src/cmd/go/internal/work/build.go +++ b/src/cmd/go/internal/work/build.go @@ -658,6 +658,9 @@ type Builder struct { flagCache map[[2]string]bool // a cache of supported compiler flags Print func(args ...interface{}) (int, error) + objdirSeq int // counter for NewObjdir + pkgSeq int + output sync.Mutex scriptDir string // current directory in printed script @@ -683,7 +686,6 @@ type Action struct { // Generated files, directories. Link bool // target is executable, not just package - Pkgdir string // the -I or -L argument to use when importing this package Objdir string // directory for intermediate objects Objpkg string // the intermediate package .a file created during the action Target string // goal of the action: the created package or executable @@ -746,6 +748,19 @@ func (b *Builder) Init() { } } +// NewObjdir returns the name of a fresh object directory under b.WorkDir. +// It is up to the caller to call b.Mkdir on the result at an appropriate time. +// The result ends in a slash, so that file names in that directory +// can be constructed with direct string addition. +// +// NewObjdir must be called only from a single goroutine at a time, +// so it is safe to call during action graph construction, but it must not +// be called during action graph execution. +func (b *Builder) NewObjdir() string { + b.objdirSeq++ + return filepath.Join(b.WorkDir, fmt.Sprintf("b%03d", b.objdirSeq)) + string(filepath.Separator) +} + // readpkglist returns the list of packages that were built into the shared library // at shlibpath. For the native toolchain this list is stored, newline separated, in // an ELF note with name "Go\x00\x00" and type 1. For GCCGO it is extracted from the @@ -816,10 +831,7 @@ func (b *Builder) action1(mode BuildMode, depMode BuildMode, p *load.Package, lo return a } - a = &Action{Package: p, Pkgdir: p.Internal.Build.PkgRoot} - if p.Internal.Pkgdir != "" { // overrides p.t - a.Pkgdir = p.Internal.Pkgdir - } + a = &Action{Package: p} b.actionCache[key] = a for _, p1 := range p.Internal.Imports { @@ -885,13 +897,9 @@ func (b *Builder) action1(mode BuildMode, depMode BuildMode, p *load.Package, lo // Imported via local path. No permanent target. mode = ModeBuild } - work := p.Internal.Pkgdir - if work == "" { - work = b.WorkDir - } - a.Objdir = filepath.Join(work, a.Package.ImportPath, "_obj") + string(filepath.Separator) - a.Objpkg = BuildToolchain.Pkgpath(work, a.Package) - a.Link = p.Name == "main" + a.Objdir = b.NewObjdir() + a.Objpkg = a.Objdir + "_pkg_.a" + a.Link = p.Name == "main" && !p.Internal.ForceLibrary switch mode { case ModeInstall: @@ -915,8 +923,7 @@ func (b *Builder) action1(mode BuildMode, depMode BuildMode, p *load.Package, lo Package: a.Package, Deps: []*Action{a.Deps[0]}, Func: (*Builder).installHeader, - Pkgdir: a.Pkgdir, - Objdir: a.Objdir, + Objdir: a.Deps[0].Objdir, Target: hdrTarget, } a.Deps = append(a.Deps, ah) @@ -1654,70 +1661,23 @@ func BuildInstallFunc(b *Builder, a *Action) (err error) { // with aggressive buffering, cleaning incrementally like // this keeps the intermediate objects from hitting the disk. if !cfg.BuildWork { - defer os.RemoveAll(a1.Objdir) - defer os.Remove(a1.Target) + defer func() { + if cfg.BuildX { + b.Showcmd("", "rm -r %s", a1.Objdir) + } + os.RemoveAll(a1.Objdir) + if _, err := os.Stat(a1.Target); err == nil { + if cfg.BuildX { + b.Showcmd("", "rm %s", a1.Target) + } + os.Remove(a1.Target) + } + }() } return b.moveOrCopyFile(a, a.Target, a1.Target, perm, false) } -// includeArgs returns the -I or -L directory list for access -// to the results of the list of actions. -func (b *Builder) includeArgs(flag string, all []*Action) []string { - inc := []string{} - incMap := map[string]bool{ - b.WorkDir: true, // handled later - cfg.GOROOTpkg: true, - "": true, // ignore empty strings - } - - // Look in the temporary space for results of test-specific actions. - // This is the $WORK/my/package/_test directory for the - // package being built, so there are few of these. - for _, a1 := range all { - if a1.Package == nil { - continue - } - if dir := a1.Pkgdir; dir != a1.Package.Internal.Build.PkgRoot && !incMap[dir] { - incMap[dir] = true - inc = append(inc, flag, dir) - } - } - - // Also look in $WORK for any non-test packages that have - // been built but not installed. - inc = append(inc, flag, b.WorkDir) - - // Finally, look in the installed package directories for each action. - // First add the package dirs corresponding to GOPATH entries - // in the original GOPATH order. - need := map[string]*build.Package{} - for _, a1 := range all { - if a1.Package != nil && a1.Pkgdir == a1.Package.Internal.Build.PkgRoot { - need[a1.Package.Internal.Build.Root] = a1.Package.Internal.Build - } - } - for _, root := range cfg.Gopath { - if p := need[root]; p != nil && !incMap[p.PkgRoot] { - incMap[p.PkgRoot] = true - inc = append(inc, flag, p.PkgTargetRoot) - } - } - - // Then add anything that's left. - for _, a1 := range all { - if a1.Package == nil { - continue - } - if dir := a1.Pkgdir; dir == a1.Package.Internal.Build.PkgRoot && !incMap[dir] { - incMap[dir] = true - inc = append(inc, flag, a1.Package.Internal.Build.PkgTargetRoot) - } - } - - return inc -} - // moveOrCopyFile is like 'mv src dst' or 'cp src dst'. func (b *Builder) moveOrCopyFile(a *Action, dst, src string, perm os.FileMode, force bool) error { if cfg.BuildN { @@ -1842,6 +1802,9 @@ func (b *Builder) installHeader(a *Action) error { if _, err := os.Stat(src); os.IsNotExist(err) { // If the file does not exist, there are no exported // functions, and we do not install anything. + if cfg.BuildX { + b.Showcmd("", "# %s not created", src) + } return nil } -- 2.48.1