From 1b882214df749ed0583385a7e02f058790fb9e56 Mon Sep 17 00:00:00 2001 From: Cosmos Nicolaou Date: Tue, 20 Oct 2020 16:04:25 -0700 Subject: [PATCH 1/3] . --- v23/verror/pkgpath.go | 79 +++++++++++++++++++++++++++---------------- v23/verror/verror.go | 8 ++--- 2 files changed, 53 insertions(+), 34 deletions(-) diff --git a/v23/verror/pkgpath.go b/v23/verror/pkgpath.go index 59e6c6968..329142aae 100644 --- a/v23/verror/pkgpath.go +++ b/v23/verror/pkgpath.go @@ -6,15 +6,13 @@ package verror import ( "fmt" - "io/ioutil" "os" "path" "path/filepath" + "reflect" "runtime" "strings" "sync" - - "golang.org/x/mod/modfile" ) type pathCache struct { @@ -53,40 +51,63 @@ func (pc *pathCache) set(dir, pkg string) { pc.paths[dir] = pkg } -func (pc *pathCache) pkgPath(file string) (string, error) { - dir := filepath.Clean(filepath.Dir(file)) - if p, ok := pc.has(dir); ok { - return p, nil - } - root, err := enclosingGoMod(dir) - if err != nil { - return "", err - } - gomodfile := filepath.Join(root, "go.mod") - gomod, err := ioutil.ReadFile(gomodfile) - if err != nil { - return "", err - } - module := modfile.ModulePath(gomod) - if len(module) == 0 { - return "", fmt.Errorf("failed to read module path from %v", gomodfile) +// IDPath returns a string of the form . +// where is derived from the type of the supplied +// value. Typical usage would be except that dummy can be replaced +// by an existing type defined in the package. +// +// type dummy int +// verror.ID(verror.IDPath(dummy(0), "MyError")) +// +func IDPath(val interface{}, id string) ID { + return ID(reflect.TypeOf(val).PkgPath() + "." + id) +} + +func longestCommonSuffix(pkgPath, filename string) string { + longest := "" + for { + fl := filepath.Base(filename) + pl := path.Base(pkgPath) + if fl == pl { + longest = path.Join(fl, longest) + filename = filepath.Dir(filename) + pkgPath = path.Dir(pkgPath) + continue + } + break } + return longest +} - pkgPath := strings.TrimPrefix(dir, root) - if !strings.HasPrefix(pkgPath, module) { - pkgPath = path.Join(module, pkgPath) +var thisPkg string +var thisPkgOnce sync.Once + +func initThisPkg() { + type dummy int + thisPkg = reflect.TypeOf(dummy(0)).PkgPath() +} + +func (pc *pathCache) pkgPath(file string) string { + thisPkgOnce.Do(initThisPkg) + pkgPath := longestCommonSuffix(thisPkg, filepath.Dir(file)) + if len(pkgPath) == 0 { + return "" } - pc.set(dir, pkgPath) - return pkgPath, nil + pkgPath = path.Join(strings.TrimSuffix(thisPkg, pkgPath), pkgPath) + pc.set(filepath.Dir(file), pkgPath) + return pkgPath } func ensurePackagePath(id ID) ID { + sid := string(id) + if strings.Contains(sid, ".") && sid[0] != '.' { + return id + } _, file, _, _ := runtime.Caller(2) - pkg, err := pkgPathCache.pkgPath(file) - if err != nil { - panic(fmt.Sprintf("failed to determine package name for %v: %v", file, err)) + pkg := pkgPathCache.pkgPath(file) + if len(pkg) == 0 { + return id } - sid := string(id) if strings.HasPrefix(sid, pkg) { return id } diff --git a/v23/verror/verror.go b/v23/verror/verror.go index b70a43aea..0c4e16d76 100644 --- a/v23/verror/verror.go +++ b/v23/verror/verror.go @@ -178,9 +178,8 @@ type IDAction struct { // Register returns a IDAction with the given ID and Action fields, and // inserts a message into the default i18n Catalogue in US English. -// Other languages can be added by adding to the Catalogue. Register -// will internally prepend the caller's package path to id if it's not -// already present. +// Other languages can be added by adding to the Catalogue. +// IDPath can be used to generate an appropriate ID. func Register(id ID, action ActionCode, englishText string) IDAction { id = ensurePackagePath(id) i18n.Cat().SetWithBase(defaultLangID(i18n.NoLangID), i18n.MsgID(id), englishText) @@ -189,8 +188,7 @@ func Register(id ID, action ActionCode, englishText string) IDAction { // NewIDAction creates a new instance of IDAction with the given ID and Action // field. It should be used when localization support is not required instead -// of Register. NewIDAction will internally prepend the caller's package path -// to id if it's not already present. +// of Register. IDPath can be used to func NewIDAction(id ID, action ActionCode) IDAction { return IDAction{ensurePackagePath(id), action} } From 95d4df97801144e3eb295c3fae7f18cd365ddcc2 Mon Sep 17 00:00:00 2001 From: Cosmos Nicolaou Date: Tue, 20 Oct 2020 16:20:13 -0700 Subject: [PATCH 2/3] . --- v23/verror/pkgpath.go | 18 ++++++++++++++++++ x/ref/runtime/internal/flow/util.go | 2 ++ x/ref/runtime/internal/flow/util_test.go | 6 +++--- 3 files changed, 23 insertions(+), 3 deletions(-) diff --git a/v23/verror/pkgpath.go b/v23/verror/pkgpath.go index 329142aae..ac2674cd7 100644 --- a/v23/verror/pkgpath.go +++ b/v23/verror/pkgpath.go @@ -63,6 +63,7 @@ func IDPath(val interface{}, id string) ID { return ID(reflect.TypeOf(val).PkgPath() + "." + id) } +/* func longestCommonSuffix(pkgPath, filename string) string { longest := "" for { @@ -86,6 +87,14 @@ func initThisPkg() { type dummy int thisPkg = reflect.TypeOf(dummy(0)).PkgPath() } +*/ + +var thisDir string + +func init() { + _, file, _, _ := runtime.Caller(0) + thisDir := filepath.Dir(file) +} func (pc *pathCache) pkgPath(file string) string { thisPkgOnce.Do(initThisPkg) @@ -99,20 +108,29 @@ func (pc *pathCache) pkgPath(file string) string { } func ensurePackagePath(id ID) ID { + fmt.Printf("EP: %v\n", id) sid := string(id) if strings.Contains(sid, ".") && sid[0] != '.' { + fmt.Printf("EP: 1 %v\n", id) + return id } _, file, _, _ := runtime.Caller(2) pkg := pkgPathCache.pkgPath(file) if len(pkg) == 0 { + fmt.Printf("EP: 2 %v ... %v\n", file, id) return id } if strings.HasPrefix(sid, pkg) { + fmt.Printf("EP: 3 %v\n", id) + return id } if strings.HasPrefix(sid, ".") { + fmt.Printf("EP: 4 %v\n", id) + return ID(pkg + sid) } + fmt.Printf("SEP: %v -> %v\n", id, ID(pkg+"."+sid)) return ID(pkg + "." + sid) } diff --git a/x/ref/runtime/internal/flow/util.go b/x/ref/runtime/internal/flow/util.go index a65c37121..c428f49ac 100644 --- a/x/ref/runtime/internal/flow/util.go +++ b/x/ref/runtime/internal/flow/util.go @@ -5,6 +5,7 @@ package flow import ( + "fmt" "strings" "v.io/v23/context" @@ -35,6 +36,7 @@ func shouldWrap(err error) bool { } id := verror.ErrorID(err) for _, pkg := range noWrapPackages { + fmt.Printf("COMPARE: %v %v %v\n", err, id, pkg) if strings.HasPrefix(string(id), pkg) { return false } diff --git a/x/ref/runtime/internal/flow/util_test.go b/x/ref/runtime/internal/flow/util_test.go index 10b1217ed..75244d92f 100644 --- a/x/ref/runtime/internal/flow/util_test.go +++ b/x/ref/runtime/internal/flow/util_test.go @@ -24,7 +24,7 @@ func TestMaybeWrapError(t *testing.T) { {verror.ErrUnknown.Errorf(ctx, "some random thing"), false}, {flow.ErrAuth.Errorf(ctx, ""), false}, } - for _, test := range tests { + for i, test := range tests { werr := MaybeWrapError(flow.ErrAuth, ctx, test.err) // If the returned error is not equal to the original error it was wrapped. msg := "" @@ -33,9 +33,9 @@ func TestMaybeWrapError(t *testing.T) { } if wasWrapped := werr.Error() != msg; wasWrapped != test.wrap { if test.wrap { - t.Errorf("wanted %v to be wrapped", test.err) + t.Errorf("%v: wanted %v to be wrapped", i, test.err) } else { - t.Errorf("did not want %v to be wrapped", test.err) + t.Errorf("%v: did not want %v to be wrapped", i, test.err) } } } From 3905919075f0b7090f810ccddf842d4bfd042eb5 Mon Sep 17 00:00:00 2001 From: Cosmos Nicolaou Date: Tue, 20 Oct 2020 19:26:00 -0700 Subject: [PATCH 3/3] . --- v23/verror/pkgpath.go | 85 ++++++++++++++--------------- v23/verror/pkgpath_test.go | 51 +++++++++++++++++ v23/verror/verror_id_test.go | 24 ++++++++ x/ref/runtime/internal/flow/util.go | 2 - 4 files changed, 116 insertions(+), 46 deletions(-) create mode 100644 v23/verror/pkgpath_test.go create mode 100644 v23/verror/verror_id_test.go diff --git a/v23/verror/pkgpath.go b/v23/verror/pkgpath.go index ac2674cd7..f67b1031c 100644 --- a/v23/verror/pkgpath.go +++ b/v23/verror/pkgpath.go @@ -5,8 +5,6 @@ package verror import ( - "fmt" - "os" "path" "path/filepath" "reflect" @@ -20,20 +18,6 @@ type pathCache struct { paths map[string]string } -func enclosingGoMod(dir string) (string, error) { - for { - gomodfile := filepath.Join(dir, "go.mod") - if fi, err := os.Stat(gomodfile); err == nil && !fi.IsDir() { - return dir, nil - } - d := filepath.Dir(dir) - if d == dir { - return "", fmt.Errorf("failed to find enclosing go.mod for dir %v", dir) - } - dir = d - } -} - var pkgPathCache = pathCache{ paths: make(map[string]string), } @@ -63,74 +47,87 @@ func IDPath(val interface{}, id string) ID { return ID(reflect.TypeOf(val).PkgPath() + "." + id) } -/* -func longestCommonSuffix(pkgPath, filename string) string { - longest := "" +// longestCommonSuffix for a package path and filename. +func longestCommonSuffix(pkgPath, filename string) (string, string) { + longestPkg, longestFilePath := "", "" for { fl := filepath.Base(filename) pl := path.Base(pkgPath) if fl == pl { - longest = path.Join(fl, longest) + longestPkg = path.Join(fl, longestPkg) + longestFilePath = filepath.Join(fl, longestFilePath) filename = filepath.Dir(filename) pkgPath = path.Dir(pkgPath) + if fl == "/" { + break + } continue } break } - return longest + return longestPkg, longestFilePath } -var thisPkg string -var thisPkgOnce sync.Once +type pathState struct { + pkg string // pkg path for the value passed to init + dir string // the directory component for the file passed to init + // The portion of the local file path that is outside of the go module, + // e.g. for /a/b/c/core/v23/verror it would be /a/b/c/core. + filePrefix string + // the portion of the package path that does not appear in the file name, + // e.g. for /a/b/c/core/v23/verror and v.io/v23/verror it would be v.io. + pkgPrefix string +} -func initThisPkg() { - type dummy int - thisPkg = reflect.TypeOf(dummy(0)).PkgPath() +func (ps *pathState) init(pkgPath string, file string) { + ps.pkg = pkgPath + ps.dir = filepath.Dir(file) + pkgLCS, fileLCS := longestCommonSuffix(ps.pkg, ps.dir) + ps.filePrefix = filepath.Clean(strings.TrimSuffix(ps.dir, fileLCS)) + ps.pkgPrefix = path.Clean(strings.TrimSuffix(ps.pkg, pkgLCS)) } -*/ -var thisDir string +var ( + ps = &pathState{} + initOnce sync.Once +) -func init() { - _, file, _, _ := runtime.Caller(0) - thisDir := filepath.Dir(file) +func convertFileToPkgName(filename string) string { + return path.Clean(strings.ReplaceAll(filename, string(filepath.Separator), "/")) } func (pc *pathCache) pkgPath(file string) string { - thisPkgOnce.Do(initThisPkg) - pkgPath := longestCommonSuffix(thisPkg, filepath.Dir(file)) - if len(pkgPath) == 0 { + initOnce.Do(func() { + type dummy int + _, file, _, _ := runtime.Caller(0) + ps.init(reflect.TypeOf(dummy(0)).PkgPath(), file) + }) + pdir := filepath.Dir(file) + rel := strings.TrimPrefix(pdir, ps.filePrefix) + if rel == pdir { return "" } - pkgPath = path.Join(strings.TrimSuffix(thisPkg, pkgPath), pkgPath) + relPkg := convertFileToPkgName(rel) + pkgPath := path.Join(ps.pkgPrefix, relPkg) pc.set(filepath.Dir(file), pkgPath) return pkgPath } func ensurePackagePath(id ID) ID { - fmt.Printf("EP: %v\n", id) sid := string(id) if strings.Contains(sid, ".") && sid[0] != '.' { - fmt.Printf("EP: 1 %v\n", id) - return id } _, file, _, _ := runtime.Caller(2) pkg := pkgPathCache.pkgPath(file) if len(pkg) == 0 { - fmt.Printf("EP: 2 %v ... %v\n", file, id) return id } if strings.HasPrefix(sid, pkg) { - fmt.Printf("EP: 3 %v\n", id) - return id } if strings.HasPrefix(sid, ".") { - fmt.Printf("EP: 4 %v\n", id) - return ID(pkg + sid) } - fmt.Printf("SEP: %v -> %v\n", id, ID(pkg+"."+sid)) return ID(pkg + "." + sid) } diff --git a/v23/verror/pkgpath_test.go b/v23/verror/pkgpath_test.go new file mode 100644 index 000000000..3c571685a --- /dev/null +++ b/v23/verror/pkgpath_test.go @@ -0,0 +1,51 @@ +package verror + +import ( + "path/filepath" + "reflect" + "runtime" + "strings" + "testing" +) + +func TestLCS(t *testing.T) { + for i, tc := range []struct { + pkg, path string + lcsPkg, lcsPath string + }{ + {"a", "b", "", ""}, + {"/a/b/c", "/a/b/c", "/a/b/c", "/a/b/c"}, + {"/a/b/c/d", "/x/y/c/d", "c/d", "c/d"}, + } { + lcsPkg, lcsPath := longestCommonSuffix(tc.pkg, tc.path) + if got, want := lcsPkg, tc.lcsPkg; got != want { + t.Errorf("%v: got %v, want %v", i, got, want) + } + if got, want := lcsPath, tc.lcsPath; got != want { + t.Errorf("%v: got %v, want %v", i, got, want) + } + } +} + +func TestPkgPath(t *testing.T) { + type dummy int + ps := &pathState{} + _, file, _, _ := runtime.Caller(0) + ps.init(reflect.TypeOf(dummy(0)).PkgPath(), file) + if got, want := ps.pkg, "v.io/v23/verror"; got != want { + t.Errorf("got %v, want %v", got, want) + } + if got, want := ps.dir, filepath.Dir(file); got != want { + t.Errorf("got %v, want %v", got, want) + } + if got, want := strings.HasPrefix(file, ps.filePrefix), true; got != want { + t.Errorf("got %v, want %v", got, want) + } + if got, want := ps.pkgPrefix, "v.io"; got != want { + t.Errorf("got %v, want %v", got, want) + } + ps.init("github.com/grailbio/base", "/a/b/src/base/file") + if got, want := ps.pkgPrefix, "github.com/grailbio"; got != want { + t.Errorf("got %v, want %v", got, want) + } +} diff --git a/v23/verror/verror_id_test.go b/v23/verror/verror_id_test.go new file mode 100644 index 000000000..3e0f94f31 --- /dev/null +++ b/v23/verror/verror_id_test.go @@ -0,0 +1,24 @@ +package verror_test + +import ( + "testing" + + "v.io/v23/flow" + "v.io/v23/verror" + "v.io/x/ref/lib/security" +) + +func TestPackageErrorIDs(t *testing.T) { + for i, tc := range []struct { + err error + id string + }{ + {verror.ErrInternal.Errorf(nil, "x"), "v.io/v23/verror.Internal"}, + {flow.ErrAuth.Errorf(nil, "x"), "v.io/v23/flow.Auth"}, + {security.ErrBadPassphrase.Errorf(nil, "x"), "v.io/x/ref/lib/security.errBadPassphrase"}, + } { + if got, want := verror.ErrorID(tc.err), verror.ID(tc.id); got != want { + t.Errorf("%v: got %v, want %v", i, got, want) + } + } +} diff --git a/x/ref/runtime/internal/flow/util.go b/x/ref/runtime/internal/flow/util.go index c428f49ac..a65c37121 100644 --- a/x/ref/runtime/internal/flow/util.go +++ b/x/ref/runtime/internal/flow/util.go @@ -5,7 +5,6 @@ package flow import ( - "fmt" "strings" "v.io/v23/context" @@ -36,7 +35,6 @@ func shouldWrap(err error) bool { } id := verror.ErrorID(err) for _, pkg := range noWrapPackages { - fmt.Printf("COMPARE: %v %v %v\n", err, id, pkg) if strings.HasPrefix(string(id), pkg) { return false }