Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cmd/go: comment indicating module is an indirect dependency is not removed when the comment has no whitespace #45932

Closed
komuw opened this issue May 3, 2021 · 5 comments
Closed

cmd/go: comment indicating module is an indirect dependency is not removed when the comment has no whitespace #45932

komuw opened this issue May 3, 2021 · 5 comments
Labels
FrozenDueToAge GoCommand cmd/go modules NeedsFix The path to resolution is known, but the work has not been done. okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1
Milestone
Go1.17

Comments

@komuw
Copy link
Contributor

komuw commented May 3, 2021 •
edited

What version of Go are you using (go version)?

go version go1.16 darwin/amd64

Does this issue reproduce with the latest release?

yes

gotip version
go version devel go1.17-9f347035 Mon May 3 19:14:16 2021 +0000 darwin/amd64

What operating system and processor architecture are you using (go env)?

go env Output
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/me/Library/Caches/go-build"
GOENV="/Users/me/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/me/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/me/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.16"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/me/mystuff/ote/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/gc/sfs6hvtd1r392kn91n3jp17m0000gn/T/go-build210366927=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

rm -rf /tmp/example; mkdir -p /tmp/example; cd /tmp/example
tee -a go.mod <<EOF
module example
go 1.16
require github.com/pkg/errors v0.9.1 //indirect
EOF

tee -a main.go <<EOF
package main
import "github.com/pkg/errors"
func main(){errors.New("hi")}
EOF
go mod tidy

What did you expect to see?

  • The //indirect comment should be removed from the require stanza

What did you see instead?

  • The //indirect comment is replaced with a //ct comment
@gopherbot gopherbot added this to the Unreleased milestone May 3, 2021
@komuw
Copy link
Contributor Author

komuw commented May 3, 2021

My guess is that this is caused by the call to len() here; https://github.com/golang/mod/blob/858fdbee9c245c8109c359106e89c6b8d321f19c/modfile/rule.go#L509-L514 which expects the //indirect comment to always have a space between // and indirect

@komuw komuw mentioned this issue May 3, 2021
Closed
@seankhliao seankhliao added GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done. labels May 3, 2021
@bcmills bcmills added the modules label May 3, 2021
@bcmills bcmills modified the milestones: Unreleased, Go1.17 May 3, 2021
@bcmills bcmills added the okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 label May 3, 2021
@bcmills bcmills self-assigned this May 3, 2021
@komuw komuw mentioned this issue May 3, 2021
Closed
@gopherbot
Copy link

Change https://golang.org/cl/316569 mentions this issue: modfile: take into account that // indirect comments may not be well formatted

@bcmills
Copy link
Contributor

bcmills commented May 4, 2021

CL 316569 fixes this in the x/mod repo (thanks, @komuw!). I'll vendor it into the cmd module to fix the bug in go mod tidy and add a regression test at that point.

gopherbot pushed a commit to golang/mod that referenced this issue May 4, 2021
@komuw
modfile: take into account that // indirect comments may not be well …
67f1c1e 
…formatted

When there is an // indirect comment next to a dependency that is not actually indirect;
go mod tidy should remove it.
This was not the case when the //indirect comment was badly formatted.

We now check whether such a comment exists irrespective of the formatting.

Updates golang/go#45932

Change-Id: I6a7dca23059a0aca6f8f940da975a0d79f701571
GitHub-Last-Rev: b884ee1
GitHub-Pull-Request: #3
Reviewed-on: https://go-review.googlesource.com/c/mod/+/316569
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Trust: Bryan C. Mills <bcmills@google.com>
Trust: Jay Conrod <jayconrod@google.com>
@gopherbot
Copy link

Change https://golang.org/cl/316751 mentions this issue: cmd/go: update x/mod to fix "//indirect" comment editing

@bcmills bcmills changed the title x/mod: comment indicating module is an indirect dependency is not removed when the comment has no whitespace cmd/go: comment indicating module is an indirect dependency is not removed when the comment has no whitespace May 4, 2021
@gopherbot
Copy link

Change https://golang.org/cl/317129 mentions this issue: cmd/go.sum: remove untidy checksums

gopherbot pushed a commit that referenced this issue May 5, 2021
cmd/go.sum: remove untidy checksums
bb5e452 
I missed the 'go mod tidy' step in CL 316751 because I forgot to run
the cmd/internal/moddeps test in long mode. 😞

Updates #45932

Change-Id: Ic3f9b303ad5798ecd8cb044d4b8c766aa820bf69
Reviewed-on: https://go-review.googlesource.com/c/go/+/317129
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: David Chase <drchase@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators May 5, 2022
@rsc rsc unassigned komuw and bcmills Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge GoCommand cmd/go modules NeedsFix The path to resolution is known, but the work has not been done. okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1
Projects
None yet
Milestone
Go1.17
Development

No branches or pull requests
4 participants
@komuw @bcmills @gopherbot @seankhliao