[go] Internal/load: add more details for ` import cycle not allowed` error
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded the change for review![]( "Open in Gerrit")
Commit message
Internal/load: add more details for ` import cycle not allowed` error
The PR is to add more details for the error, so that it would be easier to trouble the cyclic imports error.
The change for the error looks like the following:
```
package cyclic-import-example
imports cyclic-import-example/packageA from /Users/personal/cyclic-import-example/main.go:4:5
imports cyclic-import-example/packageB from /Users/personal/cyclic-import-example/packageA/a.go:5:2
imports cyclic-import-example/packageA from /Users/personal/cyclic-import-example/packageB/bb.go:5:2: import cycle not allowed
```
Change diff
diff --git a/src/cmd/go/internal/load/pkg.go b/src/cmd/go/internal/load/pkg.go
index f241e93..c804766 100644
--- a/src/cmd/go/internal/load/pkg.go
+++ b/src/cmd/go/internal/load/pkg.go
@@ -326,7 +326,7 @@
// move the modload errors into this package to avoid a package import cycle,
// and from having to export an error type for the errors produced in build.
if !isMatchErr && (nogoErr != nil || isScanErr) {
- stk.Push(path)
+ stk.Push(&ImportInfo{Pkg: path, Pos: importPos})
defer stk.Pop()
}
@@ -337,7 +337,11 @@
}
p.Incomplete = true
- if path != stk.Top() {
+ top := ""
+ if stk.Top() != nil {
+ top = stk.Top().Pkg
+ }
+ if path != top {
p.Error.setPos(importPos)
}
}
@@ -558,12 +562,17 @@
return e.importPath
}
+type ImportInfo struct {
+ Pkg string
+ Pos []token.Position
+}
+
// An ImportStack is a stack of import paths, possibly with the suffix " (test)" appended.
// The import path of a test package is the import path of the corresponding
// non-test package with the suffix "_test" added.
-type ImportStack []string
+type ImportStack []*ImportInfo
-func (s *ImportStack) Push(p string) {
+func (s *ImportStack) Push(p *ImportInfo) {
*s = append(*s, p)
}
@@ -572,12 +581,37 @@
}
func (s *ImportStack) Copy() []string {
- return append([]string{}, *s...)
+ ss := make([]string, 0, len(*s))
+ for _, v := range *s {
+ ss = append(ss, v.Pkg)
+ }
+ return ss
}
-func (s *ImportStack) Top() string {
+func (s *ImportStack) CopyWithPos() []string {
+ ss := make([]string, 0, len(*s))
+ for _, v := range *s {
+ sPos := make([]string, 0, len(v.Pos))
+ for _, p := range v.Pos {
+ sPos = append(sPos, p.String())
+ }
+ lensPos := len(sPos)
+ if lensPos > 0 {
+ if lensPos > 10 {
+ sPos = append([]string{}, sPos[:10]...)
+ sPos = append(sPos, " and more")
+ }
+ ss = append(ss, v.Pkg+" from "+strings.Join(sPos, ","))
+ } else {
+ ss = append(ss, v.Pkg)
+ }
+ }
+ return ss
+}
+
+func (s *ImportStack) Top() *ImportInfo {
if len(*s) == 0 {
- return ""
+ return nil
}
return (*s)[len(*s)-1]
}
@@ -592,8 +626,12 @@
}
// If they are the same length, settle ties using string ordering.
for i := range s {
- if s[i] != t[i] {
- return s[i] < t[i]
+ siPkg := ""
+ if s[i] != nil {
+ siPkg = s[i].Pkg
+ }
+ if siPkg != t[i] {
+ return siPkg < t[i]
}
}
return false // they are equal
@@ -757,7 +795,7 @@
// sequence that empirically doesn't trigger for these errors, guarded by
// a somewhat complex condition. Figure out how to generalize that
// condition and eliminate the explicit calls here.
- stk.Push(path)
+ stk.Push(&ImportInfo{Pkg: path, Pos: importPos})
defer stk.Pop()
}
p.setLoadPackageDataError(err, path, stk, nil)
@@ -776,7 +814,7 @@
importPath := bp.ImportPath
p := packageCache[importPath]
if p != nil {
- stk.Push(path)
+ stk.Push(&ImportInfo{Pkg: path, Pos: importPos})
p = reusePackage(p, stk)
stk.Pop()
setCmdline(p)
@@ -1448,7 +1486,7 @@
if p.Internal.Imports == nil {
if p.Error == nil {
p.Error = &PackageError{
- ImportStack: stk.Copy(),
+ ImportStack: stk.CopyWithPos(),
Err: errors.New("import cycle not allowed"),
IsImportCycle: true,
}
@@ -1789,7 +1827,11 @@
// then the cause of the error is not within p itself: the error
// must be either in an explicit command-line argument,
// or on the importer side (indicated by a non-empty importPos).
- if path != stk.Top() && len(importPos) > 0 {
+ top := ""
+ if stk.Top() != nil {
+ top = stk.Top().Pkg
+ }
+ if path != top && len(importPos) > 0 {
p.Error.setPos(importPos)
}
}
@@ -1955,7 +1997,7 @@
// Errors after this point are caused by this package, not the importing
// package. Pushing the path here prevents us from reporting the error
// with the position of the import declaration.
- stk.Push(path)
+ stk.Push(&ImportInfo{Pkg: path, Pos: importPos})
defer stk.Pop()
pkgPath := p.ImportPath
diff --git a/src/cmd/go/internal/load/test.go b/src/cmd/go/internal/load/test.go
index 3135805..c207327 100644
--- a/src/cmd/go/internal/load/test.go
+++ b/src/cmd/go/internal/load/test.go
@@ -113,7 +113,7 @@
var stk ImportStack
var testEmbed, xtestEmbed map[string][]string
var incomplete bool
- stk.Push(p.ImportPath + " (test)")
+ stk.Push(&ImportInfo{Pkg: p.ImportPath + " (test)"})
rawTestImports := str.StringList(p.TestImports)
for i, path := range p.TestImports {
p1, err := loadImport(ctx, opts, pre, path, p.Dir, p, &stk, p.Internal.Build.TestImportPos[path], ResolveImport)
@@ -140,7 +140,7 @@
}
stk.Pop()
- stk.Push(p.ImportPath + "_test")
+ stk.Push(&ImportInfo{Pkg: p.ImportPath + "_test"})
pxtestNeedsPtest := false
var pxtestIncomplete bool
rawXTestImports := str.StringList(p.XTestImports)
@@ -296,7 +296,7 @@
// The generated main also imports testing, regexp, and os.
// Also the linker introduces implicit dependencies reported by LinkerDeps.
- stk.Push("testmain")
+ stk.Push(&ImportInfo{Pkg: "testmain"})
deps := TestMainDeps // cap==len, so safe for append
ldDeps, err := LinkerDeps(p)
if err != nil && pmain.Error == nil {
diff --git a/src/cmd/go/internal/work/action.go b/src/cmd/go/internal/work/action.go
index 5e83f1e..cb68f02 100644
--- a/src/cmd/go/internal/work/action.go
+++ b/src/cmd/go/internal/work/action.go
@@ -631,7 +631,7 @@
// vet expects to be able to import "fmt".
var stk load.ImportStack
- stk.Push("vet")
+ stk.Push(&load.ImportInfo{Pkg: "vet"})
p1, err := load.LoadImportWithFlags("fmt", p.Dir, p, &stk, nil, 0)
if err != nil {
base.Fatalf("unexpected error loading fmt package from package %s: %v", p.ImportPath, err)
Change information
- M src/cmd/go/internal/load/pkg.go
- M src/cmd/go/internal/load/test.go
- M src/cmd/go/internal/work/action.go
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gopher Robot (Gerrit)
Gopher Robot added 1 comment![]( "Open in Gerrit")
I spotted some possible problems.
These findings are based on simple heuristics. If a finding appears wrong, briefly reply here saying so. Otherwise, please address any problems and update the GitHub PR. When complete, mark this comment as 'Done' and click the [blue 'Reply' button](https://go.dev/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it) above.
Possible problems detected:
1. The commit title should start with the primary affected package name followed by a colon, like "net/http: improve [...]".
2. Lines in the commit message should be wrapped at ~76 characters unless needed for things like URLs or tables. You have a 108 character line.
3. Are you using markdown? Markdown should not be used to augment text in the commit message.
4. You usually need to reference a bug number for all but trivial or cosmetic fixes. For this repo, the format is usually 'Fixes #12345' or 'Updates #12345' at the end of the commit message. Should you have a bug reference?
The commit title and commit message body come from the GitHub PR title and description, and must be edited in the GitHub web interface (not via git). For instructions, see [here](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message). For guidelines on commit messages for the Go project, see [here](https://go.dev/doc/contribute#commit_messages).
(In general for Gerrit code reviews, the change author is expected to [log in to Gerrit](https://go-review.googlesource.com/login/) with a Gmail or other Google account and then close out each piece of feedback by marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 2 comments![]( "Open in Gerrit")
I spotted some possible problems.
These findings are based on simple heuristics. If a finding appears wrong, briefly reply here saying so. Otherwise, please address any problems and update the GitHub PR. When complete, mark this comment as 'Done' and click the [blue 'Reply' button](https://go.dev/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it) above.
Possible problems detected:
1. The commit title should start with the primary affected package name followed by a colon, like "net/http: improve [...]".
2. Lines in the commit message should be wrapped at ~76 characters unless needed for things like URLs or tables. You have a 108 character line.
3. Are you using markdown? Markdown should not be used to augment text in the commit message.
4. You usually need to reference a bug number for all but trivial or cosmetic fixes. For this repo, the format is usually 'Fixes #12345' or 'Updates #12345' at the end of the commit message. Should you have a bug reference?The commit title and commit message body come from the GitHub PR title and description, and must be edited in the GitHub web interface (not via git). For instructions, see [here](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message). For guidelines on commit messages for the Go project, see [here](https://go.dev/doc/contribute#commit_messages).
(In general for Gerrit code reviews, the change author is expected to [log in to Gerrit](https://go-review.googlesource.com/login/) with a Gmail or other Google account and then close out each piece of feedback by marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)
Acknowledged
Remove the markdown in github PR, but did not have a permission to edit in gerrit.
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
I will keep in mind to not use markdown in comments.
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
qiu laidongfeng2 (Gerrit)
qiu laidongfeng2 voted and added 1 comment![]( "Open in Gerrit")
Votes added by qiu laidongfeng2
| Commit-Queue | +1 |
Remove the markdown in github PR, but did not have a permission to edit in gerrit.
CL commit description comes from the first content of github PR, if want to change the commit description, please change there.
Related details
- Michael Matloob
- Sam Thanawalla
- Xin Zhang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
qiu laidongfeng2 (Gerrit)
qiu laidongfeng2 added 1 comment![]( "Open in Gerrit")
Please rebase to master to resolve the failure caused by an old branch.Thanks.
Related details
- Michael Matloob
- Sam Thanawalla
- Xin Zhang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Please rebase to master to resolve the failure caused by an old branch.Thanks.
Hi Laidongfeng,
Thanks for pointing it out!
Rebased the to master branch.
Thanks,
Xin
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
qiu laidongfeng2Remove the markdown in github PR, but did not have a permission to edit in gerrit.
CL commit description comes from the first content of github PR, if want to change the commit description, please change there.
Thanks for the info! The only thing I was trying to change is to remove the markdown syntax.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Another question : do I need to create an issue on github for the code change? Sorry for my basic question.
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Matloob (Gerrit)
Michael Matloob added 1 comment![]( "Open in Gerrit")
Another question : do I need to create an issue on github for the code change? Sorry for my basic question.
Yes, for a change like this there should be an issue that we can get agreement on before we do the code review.
Related details
- Sam Thanawalla
- Xin Zhang
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Michael MatloobAnother question : do I need to create an issue on github for the code change? Sorry for my basic question.
Yes, for a change like this there should be an issue that we can get agreement on before we do the code review.
Hi Michael, got it. Will create an issue and link here, Thanks!
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Hi All, created an issue, and link is : https://github.com/golang/go/issues/68407
Happy to hear any comments!
Thanks!
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Hi All, my issue was duplicated, the first issue link: https://github.com/golang/go/issues/66078
Thanks!
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Matloob (Gerrit)
Michael Matloob added 1 comment![]( "Open in Gerrit")
Hi All, my issue was duplicated, the first issue link: https://github.com/golang/go/issues/66078
Thanks!
Thanks! let's continue the discussion on that issue. I agree with the issue author that we should just print the basename of the file instead of the full path. I think adding line numbers to the file name sounds reasonable too.
Related details
- Sam Thanawalla
- Xin Zhang
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Michael MatloobHi All, my issue was duplicated, the first issue link: https://github.com/golang/go/issues/66078
Thanks!
Thanks! let's continue the discussion on that issue. I agree with the issue author that we should just print the basename of the file instead of the full path. I think adding line numbers to the file name sounds reasonable too.
Cool!Thank you for checking the issue! Let's continue the discussion on the issue. Meanwhile, I will address the comment to use basename instead of fullpath.
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Matloob (Gerrit)
Michael Matloob added 1 comment![]( "Open in Gerrit")
Michael MatloobHi All, my issue was duplicated, the first issue link: https://github.com/golang/go/issues/66078
Thanks!
Xin ZhangThanks! let's continue the discussion on that issue. I agree with the issue author that we should just print the basename of the file instead of the full path. I think adding line numbers to the file name sounds reasonable too.
Cool!Thank you for checking the issue! Let's continue the discussion on the issue. Meanwhile, I will address the comment to use basename instead of fullpath.
- Sam Thanawalla
- Xin Zhang
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Michael Matloob
- Sam Thanawalla
- Xin Zhang
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Michael MatloobHi All, my issue was duplicated, the first issue link: https://github.com/golang/go/issues/66078
Thanks!
Xin ZhangThanks! let's continue the discussion on that issue. I agree with the issue author that we should just print the basename of the file instead of the full path. I think adding line numbers to the file name sounds reasonable too.
Michael MatloobCool!Thank you for checking the issue! Let's continue the discussion on the issue. Meanwhile, I will address the comment to use basename instead of fullpath.
Great! Thanks!
No problem! Addressed the comment.
The new message is
package cyclic-import-example
imports cyclic-import-example/packageA from main.go:4:5
imports cyclic-import-example/packageB from a.go:5:2
imports cyclic-import-example/packageA from bb.go:5:2: import cycle not allowed
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Hi All, we have some discussion on the issue: https://github.com/golang/go/issues/66078
Please feel free to add comments.
Thank you!
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Hi Michael, Laidongfeng, by chance you have time, could you help me to trigger a TryBots-Pass build? Thank you!!
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Matloob (Gerrit)
Michael Matloob voted Commit-Queue+1![]( "Open in Gerrit")
- Sam Thanawalla
- qiu laidongfeng2
Michael Matloob (Gerrit)
Michael Matloob voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michael Matloob
| Commit-Queue | +1 |
1 comment
Some tests are failing due to the expected error message not matching what we now produce.
There also seems to be a test failing in x/tools. It looks like it's parsing the error message. We'll have to update go/packages to expect the new format.
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Thanks Michael for triggering the tests! I found that there are tests in which we check if the output of error message of import cycle matches the expected error messages, the tests are from some places inside golang repo and golang/x/tool repo. I will make changes in the tests and maybe another PR for golang/x/tool repo.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Hi Michael, I added changes to fix the tests in golang repo. Also I created another PR for a x/tools' test. https://github.com/golang/tools/pull/507, and I do not have a permission to add you as reviewer. By chance you have time, could you take a look?
Also, It seems we may need to merge the PR of x/tools first to be able to fix the test for the golang CL. Sorry for the inconvenience.
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Update:
The x/tools' PR was imported to Gerrit: https://go-review.googlesource.com/c/tools/+/601575
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Hi Michael, sorry for my late, addressed your comment and submitted the changes. Right now, we keep ImportStack as it was without position info, and add ImportStackWithPos which has the position info. So that, ImportStack is backward compatible.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Hi Michael adn Laidengfeng,
By chance you have time, could you help me to trigger TryBots-Pass build? Thank you!!
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 2 comments![]( "Open in Gerrit")
Hi Michael adn Laidengfeng,
By chance you have time, could you help me to trigger TryBots-Pass build? Thank you!!
Hi Michael and Laidongfeng,
By chance you have time, could you help me to trigger TryBots-Pass build? Thank you!!
Michael Matloob (Gerrit)
Michael Matloob voted Commit-Queue+1![]( "Open in Gerrit")
- Sam Thanawalla
- qiu laidongfeng2
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Hi Michael, I submitted code for fixing tests. When you have time, please help me to trigger a TryBots-Pass. Thanks for your comment on the other CL, very helpful info for me to find the way to fix.
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Matloob (Gerrit)
Michael Matloob voted Run-TryBot+1![]( "Open in Gerrit")
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
Legacy-TryBots-Pass
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Gopher RobotTryBots beginning. Status page: https://farmer.golang.org/try?commit=089f437c
Xin Zhang1 of 1 TryBots failed.
Failed on freebsd-amd64-12_3: https://storage.googleapis.com/go-build-log/089f437c/freebsd-amd64-12_3_a7f5e38e.logConsult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test *exactly* your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.
Hi Michael, thanks for triggering the tests! I will look into the failing test (freebsd-amd64-12_3)
Related details
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
Legacy-TryBots-Pass
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Matloob (Gerrit)
Michael Matloob added 1 comment![]( "Open in Gerrit")
Gopher RobotTryBots beginning. Status page: https://farmer.golang.org/try?commit=089f437c
Xin Zhang1 of 1 TryBots failed.
Failed on freebsd-amd64-12_3: https://storage.googleapis.com/go-build-log/089f437c/freebsd-amd64-12_3_a7f5e38e.logConsult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test *exactly* your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.
Hi Michael, thanks for triggering the tests! I will look into the failing test (freebsd-amd64-12_3)
- Sam Thanawalla
- Xin Zhang
- qiu laidongfeng2
Code-Review
Legacy-TryBots-Pass
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Legacy-TryBots-Pass: TryBot-Result-1 by Gopher Robot, Run-TryBot+1 by Michael Matloob
Related details
- Michael Matloob
- Sam Thanawalla
- Xin Zhang
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Gopher RobotTryBots beginning. Status page: https://farmer.golang.org/try?commit=089f437c
Xin Zhang1 of 1 TryBots failed.
Failed on freebsd-amd64-12_3: https://storage.googleapis.com/go-build-log/089f437c/freebsd-amd64-12_3_a7f5e38e.logConsult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test *exactly* your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.
Michael MatloobHi Michael, thanks for triggering the tests! I will look into the failing test (freebsd-amd64-12_3)
Thanks!
Hi Michael! Good morning! When you have time, could you please help me to trigger the TryBots? Thank you!
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Matloob (Gerrit)
Michael Matloob voted Run-TryBot+1![]( "Open in Gerrit")
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
Legacy-TryBots-Pass
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Gopher RobotTryBots beginning. Status page: https://farmer.golang.org/try?commit=c20c0c55
1 of 1 TryBots failed.
Failed on freebsd-amd64-12_3: https://storage.googleapis.com/go-build-log/c20c0c55/freebsd-amd64-12_3_eaf55317.log
Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test *exactly* your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.
Hi Michael, seems the error is a connection error in tests.
--- FAIL: TestCrossVersionResume (0.02s)
--- FAIL: TestCrossVersionResume/TLSv12 (0.01s)
handshake_test.go:495: failed to call cli.Close: close tcp 127.0.0.1:59412->127.0.0.1:13448: connection reset by peer
--- FAIL: TestHandshakeKyber (0.00s)
--- FAIL: TestHandshakeKyber/ServerCurvePreferencesHRR (0.00s)
handshake_test.go:495: failed to call cli.Close: close tcp 127.0.0.1:59666->127.0.0.1:13448: connection reset by peer
Seems unrelated to the CL change. When you have time, could you try to re-run it?
Michael Matloob (Gerrit)
Michael Matloob voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Hi Michael, seeing errors in the tests, will fix them. Thanks!
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Legacy-TryBots-Pass: TryBot-Result-1 by Gopher Robot, Run-TryBot+1 by Michael Matloob
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Hi Michael, I fixed the npe and some other error. And I have questions about the tests under /TestScript, somehow it does not use the golang change in this CL instead it uses the golang version without the CL change.
Wondering how to specify to use go version in the CL instead of local go version? Thanks!
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Hi Michael, I finally figured out the list_import_cycle_deps_errors test (it was because I need to add ImportStackWithPos in anonymous struct https://github.com/golang/go/pull/68337/files#diff-abdadaf0d85a2e6c8e45da716909b2697d830b0c75149b9e35accda9c38622bdR507),
the CL is ready for another trybot-pass, when you have time, please help me to trigger it, thank you so much!
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Matloob (Gerrit)
Michael Matloob voted Commit-Queue+1![]( "Open in Gerrit")
- Sam Thanawalla
- qiu laidongfeng2
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Hi Michael, thank you so much for triggering the tests! I will look into the errors.
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Hi Michael, I fixed all errors. For the error of list_test_cycle, I reverted some code change for this test because in the ImportStack implementation of load/test.go, The reason is because it does not have token.position for ImportStack, so that we could not find a proper golang file name for each import.
Please let me know if you think it makes sense or not.
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Matloob (Gerrit)
Michael Matloob voted Commit-Queue+1![]( "Open in Gerrit")
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Matloob (Gerrit)
Michael Matloob added 1 comment![]( "Open in Gerrit")
Hi Michael, I fixed all errors. For the error of list_test_cycle, I reverted some code change for this test because in the ImportStack implementation of load/test.go, The reason is because it does not have token.position for ImportStack, so that we could not find a proper golang file name for each import.
Please let me know if you think it makes sense or not.
Hm, can we get the import position info from p.Internal.Build.ImportPos?
Related details
- Sam Thanawalla
- Xin Zhang
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Michael MatloobHi Michael, I fixed all errors. For the error of list_test_cycle, I reverted some code change for this test because in the ImportStack implementation of load/test.go, The reason is because it does not have token.position for ImportStack, so that we could not find a proper golang file name for each import.
Please let me know if you think it makes sense or not.
Hm, can we get the import position info from p.Internal.Build.ImportPos?
Oh, I see, good to know! Yes, I will use that field, thanks for the info!
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 2 comments![]( "Open in Gerrit")
Michael MatloobHi Michael, I fixed all errors. For the error of list_test_cycle, I reverted some code change for this test because in the ImportStack implementation of load/test.go, The reason is because it does not have token.position for ImportStack, so that we could not find a proper golang file name for each import.
Please let me know if you think it makes sense or not.
Xin ZhangHm, can we get the import position info from p.Internal.Build.ImportPos?
Oh, I see, good to know! Yes, I will use that field, thanks for the info!
Hi Michael, good afternoon! When you have time, could you help me to trigger a trybot-
Hi Michael, good afternoon! I addressed your comment about using internal.Build.ImportPos. When you have time, could you help me to trigger a tryBots-pass.
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Matloob (Gerrit)
Michael Matloob voted Commit-Queue+1![]( "Open in Gerrit")
- Sam Thanawalla
- qiu laidongfeng2
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Hi Michael, when you have time, could you help to review the code change in the CL?
Thank you!
Related details
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Matloob (Gerrit)
Michael Matloob added 12 comments![]( "Open in Gerrit")
Internal/load: add more details for import cycle not allowed error"cmd/go/internal/load" or "cmd/go"
You may have to shorten the rest of the message to get everything to fit.
```Please remove the backticks here and below.
ImportStackWithPos []string // shortest path from package named on command line to this one with positionI think we should remove this too. See the comment on MarshalJSON.
IsImportCycle bool // the error is an import cycle
Hard bool // whether the error is soft or hard; soft errors are ignored in some places
alwaysPrintStack bool // whether to always print the ImportStack
}Please remove these from the documentation since we don't marshal them with MarshalJSON.
ImportStack []string // shortest path from package named on command line to this one
ImportStackWithPos []string // shortest path from package named on command line to this one with position
Pos string // position of errorI think we should have a single field of type ImportStack ([]*ImportInfo). Then we can move all the formatting to be done by PackageError.Error (or a function that it calls)
MarshalJSON would have to call Copy itself. (though I think we should change the name. see below)
ImportStackWithPos []stringI don't think we should include ImportStackWithPos in the JSON output unless we're sure that we have a usage. (I also think that if we want to marshal the positions of the import stack it should be in a more structured format)
type ImportInfo struct {I think we should probably unexport this. It's mostly an internal implementation detail. I think the reason it currently needs to be exported is so we can pass a path and pos in to Push in internal/work/action.go. But I think we can just have push take (string, []token.Position) as arguments and construct the import info itself.
Pos []token.PositionCould we use a token.Position rather than a slice? If the input []token.Position has at least one element we can use pos[0], and otherwise we can just use an empty token.Position{}.
Then instead of checking the length of this field to determine whether there is a position to print, we can just call IsValid() on the position. (It will return false for empty positions)
func (s *ImportStack) Copy() []string {I think we should keep the previous version of Copy, but also add a new function with a different name that does what this function does so we can extract the set of packages for marshaling the JSON.
ss = append(ss, v.Pkg+" from "+trimLineNumber(convertToBasename(v.Pos[0].String())))I think it would be better for us to construct the string from v.Pos rather than calling String() and then doing string manipulation on it.
"ImportStackWithPos": [See comment on PackageError.MarshalJSON. I don't think we should marshal the ImportStackWithPos field.
Related details
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Hi Michael, thank you so much for the comments! Will address them.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 10 comments![]( "Open in Gerrit")
Internal/load: add more details for import cycle not allowed error"cmd/go/internal/load" or "cmd/go"
You may have to shorten the rest of the message to get everything to fit.
Thanks for pointing it out!
The new title and summary are :
cmd/go: add file names for cyclic import error
For some reason, I could not update the commit info.
When click save:
An error occurred
Error 403: modifying commit message not permitted
Endpoint: /changes/~/message
Internal/load: add more details for import cycle not allowed error"cmd/go/internal/load" or "cmd/go"
You may have to shorten the rest of the message to get everything to fit.
Thanks for pointing it out!
The new title and summary are :
cmd/go: add file names for cyclic import error
For some reason, I could not update the commit info.
When click save:
An error occurred
Error 403: modifying commit message not permitted
Endpoint: /changes/*~*/message
Please remove the backticks here and below.
Thanks for highlighting that!
I tried to remove it, but when clicking the save button, got 403 error.
Please remove the backticks here and below.
Yes, but got the same 403 error when clicking save.
Xin Zhangadd "Fixes #66078"
Yes, but got the same 403 error when clicking save.
Xin Zhangadd "Fixes #66078"
Yes, but got the same 403 error when clicking save.
ImportStackWithPos []string // shortest path from package named on command line to this one with positionI think we should remove this too. See the comment on MarshalJSON.
Done
IsImportCycle bool // the error is an import cycle
Hard bool // whether the error is soft or hard; soft errors are ignored in some places
alwaysPrintStack bool // whether to always print the ImportStack
}Please remove these from the documentation since we don't marshal them with MarshalJSON.
Done
ss = append(ss, v.Pkg+" from "+trimLineNumber(convertToBasename(v.Pos[0].String())))I think it would be better for us to construct the string from v.Pos rather than calling String() and then doing string manipulation on it.
Done, yes, I find that there is Filename var from Pos, so we could directly use it to simplify the code and without extracting basename and trimming number.
See comment on PackageError.MarshalJSON. I don't think we should marshal the ImportStackWithPos field.
Done
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Hi Michael, I finished addressing some of comments and still addressing other comments, will let you know when the CL is ready for another round of review. Thanks!
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Matloob (Gerrit)
Michael Matloob added 1 comment![]( "Open in Gerrit")
Internal/load: add more details for import cycle not allowed errorXin Zhang"cmd/go/internal/load" or "cmd/go"
You may have to shorten the rest of the message to get everything to fit.
Thanks for pointing it out!
The new title and summary are :
cmd/go: add file names for cyclic import errorFor some reason, I could not update the commit info.
When click save:An error occurred
Error 403: modifying commit message not permitted
Michael MatloobEndpoint: /changes/*~*/message
Hi, I am not too familiar with the Github workflow but I believe the first line is taken from the title of the github pull request and the rest are taken from the first comment. Could you try editing those?
Related details
- Sam Thanawalla
- Xin Zhang
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Michael Matloob
- Sam Thanawalla
- Xin Zhang
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Internal/load: add more details for import cycle not allowed errorXin Zhang"cmd/go/internal/load" or "cmd/go"
You may have to shorten the rest of the message to get everything to fit.
Michael MatloobThanks for pointing it out!
The new title and summary are :
cmd/go: add file names for cyclic import errorFor some reason, I could not update the commit info.
When click save:An error occurred
Error 403: modifying commit message not permittedEndpoint: /changes/*~*/message
Hi, I am not too familiar with the Github workflow but I believe the first line is taken from the title of the github pull request and the rest are taken from the first comment. Could you try editing those?
Hi Michael, thanks for the suggestion! I edited on github, hope CL will sync up with the github change.
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Internal/load: add more details for import cycle not allowed errorXin Zhang"cmd/go/internal/load" or "cmd/go"
You may have to shorten the rest of the message to get everything to fit.
Michael MatloobThanks for pointing it out!
The new title and summary are :
cmd/go: add file names for cyclic import errorFor some reason, I could not update the commit info.
When click save:An error occurred
Error 403: modifying commit message not permittedEndpoint: /changes/*~*/message
Xin ZhangHi, I am not too familiar with the Github workflow but I believe the first line is taken from the title of the github pull request and the rest are taken from the first comment. Could you try editing those?
Hi Michael, thanks for the suggestion! I edited on github, hope CL will sync up with the github change.
Cool! It did update!
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 6 comments![]( "Open in Gerrit")
Hi Michael, addressed all the comments, when you have time, the CL is ready for a second look.
ImportStack []string // shortest path from package named on command line to this one
ImportStackWithPos []string // shortest path from package named on command line to this one with position
Pos string // position of errorI think we should have a single field of type ImportStack ([]*ImportInfo). Then we can move all the formatting to be done by PackageError.Error (or a function that it calls)
MarshalJSON would have to call Copy itself. (though I think we should change the name. see below)
Done
ImportStackWithPos []stringXin ZhangI don't think we should include ImportStackWithPos in the JSON output unless we're sure that we have a usage. (I also think that if we want to marshal the positions of the import stack it should be in a more structured format)
Done, yes, introduced position inside importInfo struct.
type ImportInfo struct {Xin ZhangI think we should probably unexport this. It's mostly an internal implementation detail. I think the reason it currently needs to be exported is so we can pass a path and pos in to Push in internal/work/action.go. But I think we can just have push take (string, []token.Position) as arguments and construct the import info itself.
Got it. I changed to push *importInfo, which has path and *token.Position, since we only need one position if it has. Please correct me if I am wrong.
Could we use a token.Position rather than a slice? If the input []token.Position has at least one element we can use pos[0], and otherwise we can just use an empty token.Position{}.
Then instead of checking the length of this field to determine whether there is a position to print, we can just call IsValid() on the position. (It will return false for empty positions)
Done, yes, agreed, we only need one. Changed to use *token.Position (use pointer for checking if it is nil) instead of []token.Position
func (s *ImportStack) Copy() []string {Xin ZhangI think we should keep the previous version of Copy, but also add a new function with a different name that does what this function does so we can extract the set of packages for marshaling the JSON.
Done, yes, added, so now there are three methods:
Copy(), CopyPkgs(), CopyPkgsWithPos()
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
qiu laidongfeng2 (Gerrit)
qiu laidongfeng2 voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Hi Qiu Laidongfeng, thanks for triggering the trybot-pass! I will look into the errors.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Hi Michael and Qiu Laidongfeng, I fixed the error from tests. When you have time, could you help to trigger the TryBots-Pass? Thanks!
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gopher Robot (Gerrit)
Message from Gopher Robot![]( "Open in Gerrit")
Congratulations on opening your first change. Thank you for your contribution!
Next steps:
A maintainer will review your change and provide feedback. See
https://go.dev/doc/contribute#review for more info and tips to get your
patch through code review.
Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.
During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11 or adds a tag like "wait-release", it means that this CL will be
reviewed as part of the next development cycle. See https://go.dev/s/release
for more details.
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
qiu laidongfeng2 (Gerrit)
qiu laidongfeng2 voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Michael Matloob
- Sam Thanawalla
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Hi Michael and Qiu Laidongfeng, I fixed some errors from tests. When you have time, could you help to trigger the TryBots-Pass? Thanks!
Related details
- Michael Matloob
- Sam Thanawalla
- qiu laidongfeng2
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
qiu laidongfeng2 (Gerrit)
qiu laidongfeng2 voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Michael Matloob
- Sam Thanawalla
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Thanks Qiu Laidongfeng for triggering the TryBot-Pass!
Related details
- Michael Matloob
- Sam Thanawalla
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Hi Michael, the tests passed, when you have time, could you take a second look?
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Hi Michael, when you have time, could you please review the PR, thank you!
Michael Matloob (Gerrit)
Michael Matloob added 3 comments![]( "Open in Gerrit")
Fixes [#66078](https://go.dev/issue/66078)`Fixes #66078`
move this after line 18/before line 20
func (s *ImportStack) Copy() []string {Xin ZhangI think we should keep the previous version of Copy, but also add a new function with a different name that does what this function does so we can extract the set of packages for marshaling the JSON.
Done, yes, added, so now there are three methods:
Copy(), CopyPkgs(), CopyPkgsWithPos()
I think maybe we should just call these `Pkgs` and `PkgsWithPos`
ppos = &token.Position{Why are we making copies of the `token.Position`s? Do we modify them later?
(Same question for the other places we make copies of them)
Related details
- Sam Thanawalla
- Xin Zhang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xin Zhang (Gerrit)
Xin Zhang added 1 comment![]( "Open in Gerrit")
Hi Michael, thanks for the comments! Will address them.
Related details
- Sam Thanawalla
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |