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.

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/dist: accept imports with leading space #47544

Closed
XiaodongLoong opened this issue Aug 5, 2021 · 10 comments
Closed

cmd/dist: accept imports with leading space #47544

XiaodongLoong opened this issue Aug 5, 2021 · 10 comments
Labels
FrozenDueToAge

Comments

@XiaodongLoong
Copy link
Contributor

XiaodongLoong commented Aug 5, 2021 •
edited 

$ ./make.bash 
Building Go cmd/dist using /usr/lib/go-1.11. (go1.11.6 linux/amd64)
Building Go toolchain1 using /usr/lib/go-1.11.
../../../../src/cmd/compile/internal/mips64/ggen.go:9: cannot find package "cmd/compile/internal/objw" in any of:
	/usr/lib/go-1.11/src/cmd/compile/internal/objw (from $GOROOT)
	/home/liuxiaodong/go-orig/pkg/bootstrap/src/cmd/compile/internal/objw (from $GOPATH)
go tool dist: FAILED: /usr/lib/go-1.11/bin/go install -gcflags=-l -tags=math_big_pure_go compiler_bootstrap bootstrap/cmd/...: exit status 1

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

$ go version
go version go1.11.6 linux/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/liuxiaodong/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/liuxiaodong/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/lib/go-1.11"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go-1.11/pkg/tool/linux_amd64"
GCCGO="/usr/bin/gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/liuxiaodong/go-orig/src/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 -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build598535792=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I import packages and indent with spaces, just like this:

diff --git a/src/cmd/compile/internal/mips64/ggen.go b/src/cmd/compile/internal/mips64/ggen.go
index 37bb871958..881fe8776d 100644
--- a/src/cmd/compile/internal/mips64/ggen.go
+++ b/src/cmd/compile/internal/mips64/ggen.go
@@ -6,7 +6,7 @@ package mips64
 
 import (
        "cmd/compile/internal/ir"
-       "cmd/compile/internal/objw"    // indent with tab
+        "cmd/compile/internal/objw"   // indent with spaces
        "cmd/compile/internal/types"
        "cmd/internal/obj"
        "cmd/internal/obj/mips"

What did you expect to see?

When I import packages and indent with spaces, I can execute make.bash successfully.
Just keep consistent with the behavior of the go compiler.
Thanks!

What did you see instead?
@gopherbot
Copy link

Change https://golang.org/cl/340030 mentions this issue: cmd/dist: make.bash failed when import package and indent with spaces

@seankhliao
Copy link
Member

Go 1.11 is not a supported version, does this reproduce on the latest release?

@XiaodongLoong
Copy link
Contributor Author

XiaodongLoong commented Aug 5, 2021 •
edited

Yes, the code that have bug is following:

go/src/cmd/dist/buildtool.go

Line 291 in 6e73886

inBlock && (strings.HasPrefix(line, "\t\"") || strings.HasPrefix(line, "\t. \"") || strings.HasPrefix(line, "\texec \"")) {

@seankhliao seankhliao changed the title make.bash failed: src/cmd/compile/internal/mips64/ggen.go:9: cannot find package "cmd/compile/internal/objw" in any of cmd/dist: accept imports with leading space Aug 5, 2021
@seankhliao
Copy link
Member

The go code should be formatted with gofmt which will rewrite it to use tabs though

@ianlancetaylor
Copy link
Contributor

cmd/dist is only used to build the Go standard library and tools. It's OK for cmd/dist to assume that all the input files have gone through gofmt. In fact, it's a good thing if cmd/dist fails if the files have not gone through gofmt. So I don't think we should change this.

@XiaodongLoong
Copy link
Contributor Author

IMO, code style and correct code might be different things. I usually develop Go code firstly and then modify the format with gofmt. Code development may occur in the process of debugging without a good editor. This issue was also found during debugging.
Thanks!

@erifan
Copy link

erifan commented Aug 6, 2021

IMO, code style and correct code might be different things.

You can think of this as a coding style error in Go. Just like

if a > 0 {
...
}

can't be

if a > 0
{
...
}

This issue was also found during debugging.

This just shows that this check works.

@ianlancetaylor
Copy link
Contributor

code style and correct code might be different things.

Sure. But our rule for the standard library and tools is simple: all files must be formatted using gofmt. No exceptions.

I usually develop Go code firstly and then modify the format with gofmt. Code development may occur in the process of debugging without a good editor. This issue was also found during debugging.

Thanks, but I don't find any of these arguments to be convincing. If cmd/dist supports files that are not formatted with gofmt, then it will be easier to accidentally submit such files to the repo. We don't want that to happen, so let's not change cmd/dist. Yes, this may requires using go fmt and it may occasionally lead to debugging, but it's still the right thing to do.

@XiaodongLoong
Copy link
Contributor Author

@ianlancetaylor I got it. Thank you for answering my questions patiently. I will abandon the CL.

@ianlancetaylor
Copy link
Contributor

Thanks.

@golang golang locked and limited conversation to collaborators Aug 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@ianlancetaylor @gopherbot @seankhliao @erifan @XiaodongLoong