From 7a96da11c053a19e7b507b22951f184cd6d8d94f Mon Sep 17 00:00:00 2001 From: Paul Jolly Date: Mon, 1 Apr 2019 16:38:05 +0100 Subject: [PATCH] main: fix definition and implementation of relative GOHACK (#49) rogpeppe has a far sharper brain than me and pointed out a major issue with the previous implementation: that we hadn't actually implemented a relative value of GOHACK relative to the main module. --- cmdget.go | 33 +++++++++-------- cmdstatus.go | 8 +---- cmdundo.go | 12 +++---- main.go | 44 +++++++++++++++++------ mod.go | 53 +++++++++++++++++++-------- os_1.11.go | 37 +++++++++++++++++++ os_1.12.go | 12 +++++++ testdata/get-relative-parent.txt | 14 ++++++-- testdata/get-relative.txt | 11 +++++- testdata/get-vcs-relative-parent.txt | 54 ++++++++++++++++++++++++++++ testdata/get-vcs-relative.txt | 54 ++++++++++++++++++++++++++++ testdata/get-vcs.txt | 5 ++- 12 files changed, 275 insertions(+), 62 deletions(-) create mode 100644 os_1.11.go create mode 100644 os_1.12.go create mode 100644 testdata/get-vcs-relative-parent.txt create mode 100644 testdata/get-vcs-relative.txt diff --git a/cmdget.go b/cmdget.go index 17f95d4..161bc98 100644 --- a/cmdget.go +++ b/cmdget.go @@ -61,10 +61,6 @@ func runGet1(args []string) error { // Perhaps we should be more resilient in that case? return errors.Notef(err, nil, "cannot get module info") } - modf, err := goModInfo() - if err != nil { - return errors.Notef(err, nil, "cannot get local module info") - } for _, mpath := range args { m := mods[mpath] if m == nil { @@ -74,7 +70,7 @@ func runGet1(args []string) error { // Early check that we can replace the module, so we don't // do all the work to check it out only to find we can't // add the replace directive. - if err := checkCanReplace(modf, mpath); err != nil { + if err := checkCanReplace(mainModFile, mpath); err != nil { errorf("%v", err) continue } @@ -99,9 +95,8 @@ func runGet1(args []string) error { } repl = repl1 } - // Automatically generate a go.mod file if one doesn't - // already exist, because otherwise the directory cannot be - // used as a module. + // Automatically generate a go.mod file if one doesn't already exist, + // because otherwise the directory cannot be used as a module. if err := ensureGoModFile(repl.modulePath, repl.dir); err != nil { errorf("%v", err) } @@ -110,14 +105,14 @@ func runGet1(args []string) error { if len(repls) == 0 { return errors.New("all modules failed; not replacing anything") } - if err := replace(modf, repls); err != nil { + if err := replace(mainModFile, repls); err != nil { return errors.Notef(err, nil, "cannot replace") } - if err := writeModFile(modf); err != nil { + if err := writeModFile(mainModFile); err != nil { return errors.Wrap(err) } for _, info := range repls { - fmt.Printf("%s => %s\n", info.modulePath, info.dir) + fmt.Printf("%s => %s\n", info.modulePath, info.replDir) } return nil } @@ -130,7 +125,10 @@ func updateFromLocalDir(m *listModule) (*modReplace, error) { if err != nil { return nil, errors.Notef(err, nil, "cannot hash %q", m.Dir) } - destDir := moduleDir(m.Path) + destDir, replDir, err := moduleDir(m.Path) + if err != nil { + return nil, errors.Notef(err, nil, "failed to determine target directory for %v", m.Path) + } _, err = os.Stat(destDir) if err != nil && !os.IsNotExist(err) { return nil, errors.Wrap(err) @@ -138,6 +136,7 @@ func updateFromLocalDir(m *listModule) (*modReplace, error) { repl := &modReplace{ modulePath: m.Path, dir: destDir, + replDir: replDir, } if err != nil { // Destination doesn't exist. Copy the entire directory. @@ -234,18 +233,22 @@ func updateVCSDir(m *listModule) (*modReplace, error) { return &modReplace{ modulePath: m.Path, dir: info.dir, + replDir: info.replDir, }, nil } type modReplace struct { + // modulePath is the module path modulePath string - dir string + // dir holds the absolute path to the replacement directory. + dir string + // replDir holds the path to use for the module in the go.mod replace directive. + replDir string } func replace(f *modfile.File, repls []*modReplace) error { for _, repl := range repls { - // TODO should we use relative path here? - if err := replaceModule(f, repl.modulePath, repl.dir); err != nil { + if err := replaceModule(f, repl.modulePath, repl.replDir); err != nil { return errors.Wrap(err) } } diff --git a/cmdstatus.go b/cmdstatus.go index 672fa6a..9710132 100644 --- a/cmdstatus.go +++ b/cmdstatus.go @@ -2,8 +2,6 @@ package main import ( "fmt" - - "gopkg.in/errgo.v2/fmt/errors" ) var statusCommand = &Command{ @@ -30,11 +28,7 @@ func cmdStatus(_ *Command, args []string) int { } func printReplacementInfo() error { - m, err := goModInfo() - if err != nil { - return errors.Wrap(err) - } - for _, r := range m.Replace { + for _, r := range mainModFile.Replace { if r.Old.Version == "" && r.New.Version == "" { fmt.Printf("%s => %s\n", r.Old.Path, r.New.Path) } diff --git a/cmdundo.go b/cmdundo.go index 9dfdb9f..bee4de1 100644 --- a/cmdundo.go +++ b/cmdundo.go @@ -39,10 +39,6 @@ func cmdUndo(_ *Command, args []string) int { } func cmdUndo1(modules []string) error { - modFile, err := goModInfo() - if err != nil { - return errors.Wrap(err) - } modMap := make(map[string]bool) if len(modules) > 0 { for _, m := range modules { @@ -51,7 +47,7 @@ func cmdUndo1(modules []string) error { } else { // With no modules specified, we un-gohack all modules // we can find with local directory info in the go.mod file. - for _, r := range modFile.Replace { + for _, r := range mainModFile.Replace { if r.Old.Version == "" && r.New.Version == "" { modMap[r.Old.Path] = true modules = append(modules, r.Old.Path) @@ -59,7 +55,7 @@ func cmdUndo1(modules []string) error { } } drop := make(map[string]bool) - for _, r := range modFile.Replace { + for _, r := range mainModFile.Replace { if !modMap[r.Old.Path] || r.Old.Version != "" || r.New.Version != "" { continue } @@ -93,7 +89,7 @@ func cmdUndo1(modules []string) error { delete(modMap, r.Old.Path) } for m := range drop { - if err := modFile.DropReplace(m, ""); err != nil { + if err := mainModFile.DropReplace(m, ""); err != nil { return errors.Notef(err, nil, "cannot drop replacement for %v", m) } } @@ -105,7 +101,7 @@ func cmdUndo1(modules []string) error { for _, m := range failed { errorf("%s not currently replaced; cannot drop", m) } - if err := writeModFile(modFile); err != nil { + if err := writeModFile(mainModFile); err != nil { return errors.Wrap(err) } for _, m := range modules { diff --git a/main.go b/main.go index 59ed761..44dc20d 100644 --- a/main.go +++ b/main.go @@ -12,6 +12,7 @@ import ( "fmt" "os" + "github.com/rogpeppe/go-internal/modfile" "gopkg.in/errgo.v2/fmt/errors" ) @@ -52,6 +53,8 @@ var ( var ( exitCode = 0 cwd = "." + + mainModFile *modfile.File ) var commands = []*Command{ @@ -68,7 +71,7 @@ func main1() int { if dir, err := os.Getwd(); err == nil { cwd = dir } else { - errorf("cannot get current working directory: %v", err) + return errorf("cannot get current working directory: %v", err) } flag.Usage = func() { mainUsage(os.Stderr) @@ -83,22 +86,40 @@ func main1() int { if cmdName == "help" { return runHelp(args) } - for _, cmd := range commands { - if cmd.Name() != cmdName { - continue + var cmd *Command + for _, c := range commands { + if c.Name() == cmdName { + cmd = c + break + } + } + if cmd == nil { + errorf("gohack %s: unknown command\nRun 'gohack help' for usage\n", cmdName) + return 2 + } + + cmd.Flag.Usage = func() { cmd.Usage() } + + if err := cmd.Flag.Parse(args); err != nil { + if err != flag.ErrHelp { + errorf(err.Error()) } - cmd.Flag.Usage = func() { cmd.Usage() } - cmd.Flag.Parse(args) - rcode := cmd.Run(cmd, cmd.Flag.Args()) - return max(exitCode, rcode) + return 2 + } + + if mf, err := goModInfo(); err == nil { + mainModFile = mf + } else { + return errorf("cannot determine main module: %v", err) } - errorf("gohack %s: unknown command\nRun 'gohack help' for usage\n", cmdName) - return 2 + + rcode := cmd.Run(cmd, cmd.Flag.Args()) + return max(exitCode, rcode) } const debug = false -func errorf(f string, a ...interface{}) { +func errorf(f string, a ...interface{}) int { fmt.Fprintln(os.Stderr, fmt.Sprintf(f, a...)) if debug { for _, arg := range a { @@ -108,6 +129,7 @@ func errorf(f string, a ...interface{}) { } } exitCode = 1 + return exitCode } func max(a, b int) int { diff --git a/mod.go b/mod.go index 324ca1e..6d7cb47 100644 --- a/mod.go +++ b/mod.go @@ -4,6 +4,7 @@ import ( "io" "os" "path/filepath" + "strings" "github.com/rogpeppe/go-internal/dirhash" "golang.org/x/tools/go/vcs" @@ -45,8 +46,10 @@ type moduleVCSInfo struct { module *listModule // alreadyExists holds whether the replacement directory already exists. alreadyExists bool - // dir holds the path to the replacement directory. + // dir holds the absolute path to the replacement directory. dir string + // replDir holds the path to use for the module in the go.mod replace directive. + replDir string // root holds information on the VCS root of the module. root *vcs.RepoRoot // vcs holds the implementation of the VCS used by the module. @@ -71,7 +74,10 @@ func getVCSInfoForModule(m *listModule) (*moduleVCSInfo, error) { if !ok { return nil, errors.Newf("unknown VCS kind %q", root.VCS.Cmd) } - dir := moduleDir(m.Path) + dir, replDir, err := moduleDir(m.Path) + if err != nil { + return nil, errors.Notef(err, nil, "failed to determine target directory for %v", m.Path) + } dirInfo, err := os.Stat(dir) if err != nil && !os.IsNotExist(err) { return nil, errors.Wrap(err) @@ -84,6 +90,7 @@ func getVCSInfoForModule(m *listModule) (*moduleVCSInfo, error) { root: root, alreadyExists: err == nil, dir: dir, + replDir: replDir, vcs: v, } if !info.alreadyExists { @@ -108,22 +115,38 @@ func getVCSInfoForModule(m *listModule) (*moduleVCSInfo, error) { return info, nil } -// moduleDir returns the path to the directory to be -// used for storing the module with the given path. -func moduleDir(module string) string { - // TODO decide what color this bikeshed should be. - d := filepath.FromSlash(os.Getenv("GOHACK")) +// moduleDir returns the path to the directory to be used for storing the +// module with the given path, as well as the filepath to be used in a replace +// directive. If $GOHACK is set then it will be used. A relative $GOHACK will +// be interpreted relative to main module directory. +func moduleDir(module string) (path string, replPath string, err error) { + modfp := filepath.FromSlash(module) + d := os.Getenv("GOHACK") if d == "" { - d = filepath.Join(os.Getenv("HOME"), "gohack") + uhd, err := UserHomeDir() + if err != nil { + return "", "", errors.Notef(err, nil, "failed to determine user home dir") + } + path = filepath.Join(uhd, "gohack", modfp) + return path, path, nil } - return join(d, filepath.FromSlash(module)) -} + if filepath.IsAbs(d) { + path = filepath.Join(d, modfp) + return path, path, nil + } -func join(ps ...string) string { - res := filepath.Join(ps...) - if !filepath.IsAbs(res) && res[0] != '.' { - res = "." + string(os.PathSeparator) + res + replPath = filepath.Join(d, modfp) + if !strings.HasPrefix(replPath, ".."+string(os.PathSeparator)) { + // We know replPath is relative, but filepath.Join strips any leading + // "./" prefix, and we need that in the replace directive because + // otherwise the path will be treated as a module path rather than a + // relative file path, so add it back. + replPath = "." + string(os.PathSeparator) + replPath } - return res + + mainModDir := filepath.Dir(mainModFile.Syntax.Name) + path = filepath.Join(mainModDir, replPath) + + return path, replPath, err } diff --git a/os_1.11.go b/os_1.11.go new file mode 100644 index 0000000..86c8497 --- /dev/null +++ b/os_1.11.go @@ -0,0 +1,37 @@ +// +build !go1.12 + +package main + +import ( + "errors" + "os" + "runtime" +) + +// UserHomeDir was introduced in Go 1.12. When we drop support for Go 1.11, we can +// lose this file. + +// UserHomeDir returns the current user's home directory. +// +// On Unix, including macOS, it returns the $HOME environment variable. +// On Windows, it returns %USERPROFILE%. +// On Plan 9, it returns the $home environment variable. +func UserHomeDir() (string, error) { + env, enverr := "HOME", "$HOME" + switch runtime.GOOS { + case "windows": + env, enverr = "USERPROFILE", "%userprofile%" + case "plan9": + env, enverr = "home", "$home" + case "nacl", "android": + return "/", nil + case "darwin": + if runtime.GOARCH == "arm" || runtime.GOARCH == "arm64" { + return "/", nil + } + } + if v := os.Getenv(env); v != "" { + return v, nil + } + return "", errors.New(enverr + " is not defined") +} diff --git a/os_1.12.go b/os_1.12.go new file mode 100644 index 0000000..feb0de2 --- /dev/null +++ b/os_1.12.go @@ -0,0 +1,12 @@ +// +build go1.12 + +package main + +import "os" + +// UserHomeDir was introduced in Go 1.12. When we drop support for Go 1.11, we can +// lose this file. + +func UserHomeDir() (string, error) { + return os.UserHomeDir() +} diff --git a/testdata/get-relative-parent.txt b/testdata/get-relative-parent.txt index 8448d7c..b62fc97 100644 --- a/testdata/get-relative-parent.txt +++ b/testdata/get-relative-parent.txt @@ -1,10 +1,13 @@ -cd repo +cd repo/dummy go get rsc.io/quote@v1.5.2 -env GOHACK=../gohack/ +env GOHACK=../gohack gohack get rsc.io/quote stdout '^rsc.io/quote => \.\./gohack/rsc.io/quote$' ! stderr .+ +# move back to the module root +cd .. + # Check that the replace statement is there. grep -count=1 '^replace rsc\.io/quote => \.\./gohack/rsc.io/quote$' go.mod @@ -25,5 +28,12 @@ func main() { fmt.Println(quote.Glass()) } +-- repo/dummy/file -- + +// This dummy file exists in order to create the dummy +// directory so that the gohack command can be run from +// within the dummy directory + + -- repo/go.mod -- module example.com/repo diff --git a/testdata/get-relative.txt b/testdata/get-relative.txt index a4750b1..0c0c8df 100644 --- a/testdata/get-relative.txt +++ b/testdata/get-relative.txt @@ -1,10 +1,13 @@ -cd repo +cd repo/dummy go get rsc.io/quote@v1.5.2 env GOHACK=. gohack get rsc.io/quote stdout '^rsc.io/quote => \./rsc.io/quote$' ! stderr .+ +# move back to the module root +cd .. + # Check that the replace statement is there. grep -count=1 '^replace rsc\.io/quote => \./rsc.io/quote$' go.mod @@ -28,5 +31,11 @@ func main() { fmt.Println(quote.Glass()) } +-- repo/dummy/file -- + +// This dummy file exists in order to create the dummy +// directory so that the gohack command can be run from +// within the dummy directory + -- repo/go.mod -- module example.com/repo diff --git a/testdata/get-vcs-relative-parent.txt b/testdata/get-vcs-relative-parent.txt new file mode 100644 index 0000000..624bf2f --- /dev/null +++ b/testdata/get-vcs-relative-parent.txt @@ -0,0 +1,54 @@ +# This is same as get.txt except that we use the -vcs flag on gohack get with a +# parent-relative GOHACK set. Until we can figure out better, this requires +# access to the network. + +[!net] skip +[!exec:git] skip +env GOPROXY= + +cd repo/dummy +go get rsc.io/quote@v1.5.2 +env GOHACK=../gohack +gohack get -vcs rsc.io/quote +stdout '^rsc.io/quote => \.\./gohack/rsc.io/quote$' +! stderr .+ + +# move back to the module root +cd .. + +# Check that the replace statement is there. +grep -count=1 '^replace rsc\.io/quote => \.\./gohack/rsc.io/quote$' go.mod + +# Check that it's a git repository +exists ../gohack/rsc.io/quote/.git + +# Check that the source files have been copied. +exists ../gohack/rsc.io/quote/quote.go + +# Check that we can compile the command OK. +go install example.com/repo + +-- repo/main.go -- + +package main +import ( + "fmt" + "rsc.io/quote" +) + +func main() { + fmt.Println(quote.Glass()) +} + +-- repo/dummy/file -- + +// This dummy file exists in order to create the dummy +// directory so that the gohack command can be run from +// within the dummy directory + +-- repo/go.mod -- +module example.com/repo + +-- bogus.go -- + +package wrong diff --git a/testdata/get-vcs-relative.txt b/testdata/get-vcs-relative.txt new file mode 100644 index 0000000..09b9d13 --- /dev/null +++ b/testdata/get-vcs-relative.txt @@ -0,0 +1,54 @@ +# This is same as get.txt except that we use the -vcs flag on gohack get with a +# relative GOHACK set. Until we can figure out better, this requires access to +# the network. + +[!net] skip +[!exec:git] skip +env GOPROXY= + +cd repo/dummy +go get rsc.io/quote@v1.5.2 +env GOHACK=. +gohack get -vcs rsc.io/quote +stdout '^rsc.io/quote => \./rsc.io/quote$' +! stderr .+ + +# move back to the module root +cd .. + +# Check that the replace statement is there. +grep -count=1 '^replace rsc\.io/quote => \./rsc.io/quote$' go.mod + +# Check that it's a git repository +exists ./rsc.io/quote/.git + +# Check that the source files have been copied. +exists ./rsc.io/quote/quote.go + +# Check that we can compile the command OK. +go install example.com/repo + +-- repo/main.go -- + +package main +import ( + "fmt" + "rsc.io/quote" +) + +func main() { + fmt.Println(quote.Glass()) +} + +-- repo/dummy/file -- + +// This dummy file exists in order to create the dummy +// directory so that the gohack command can be run from +// within the dummy directory + +-- repo/go.mod -- +module example.com/repo + +-- bogus.go -- + +package wrong diff --git a/testdata/get-vcs.txt b/testdata/get-vcs.txt index f7149f0..92e7510 100644 --- a/testdata/get-vcs.txt +++ b/testdata/get-vcs.txt @@ -1,6 +1,5 @@ -# This is thame as get.txt except that we use the -vcs flag -# on gohack get. Until we can figure out better, this requires -# access to the network. +# This is same as get.txt except that we use the -vcs flag on gohack get. Until +# we can figure out better, this requires access to the network. [!net] skip [!exec:git] skip