golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
124.07k stars 17.68k forks source link

cmd/go: "leading dot in path element" for previously-undiagnosed paths in go 1.13 #34992

Closed xyt0056 closed 3 years ago

xyt0056 commented 5 years ago

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

$ go version
go version go1.13

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
GO111MODULE=""
GOARCH="amd64"
GOBIN="/Users/me/go/bin"
GOCACHE="/Users/me/Library/Caches/go-build"
GOENV="/Users/me/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY="none"
GONOSUMDB="code.corp.internal"
GOOS="darwin"
GOPATH="/Users/tanx/go-code"
GOPRIVATE="code.corp.internal"
GOPROXY="https://proxy.mycorp.com"
GOROOT="/private/var/tmp/_bazel_tanx/cde87e3334239cff91d2a561f734e9a6/external/go_sdk"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/private/var/tmp/_bazel_tanx/cde87e3334239cff91d2a561f734e9a6/external/go_sdk/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/nn/f16l5j1j2f14ctsnkk305lf00000gp/T/go-build331063970=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

upgrade from go1.12.9 to go1.13 in bazel WORKSPACE run go mod tidy

What did you expect to see?

finish successfully

What did you see instead?

code.corp.internal/repo1/a tested by
    code.corp.internal/repo1/a.test imports
    code.corp.internal/somedependency/a tested by <- external dependency
    code.corp.internal/somedependency/a.test imports
    code.corp.internal/someotherdependency: malformed module path "code.corp.internal/someotherdependency/.gen/somepath...": leading dot in path element

our monorepo setup

gopath/
    src/
        code.corp.internal/
            go.mod
            go.sum
            repo1/
            repo2/

Does go 1.13 scan code of dependency's import path and check for dots as well?

ianlancetaylor commented 5 years ago

CC @bcmills @jayconrod

bcmills commented 5 years ago

Does go 1.13 scan code of dependency's import path and check for dots as well?

Leading dots have been disallowed in module and package paths from the very start of module mode, in Go 1.11. (See https://golang.org/cl/124378.)

The major change in go mod tidy in Go 1.13 is that it now reports errors. (See #27063.)

Is there reason to believe that go mod tidy ever worked successfully for this dependency?

xyt0056 commented 5 years ago

in go.1.12 go mod tidy finishes successfully without any explicit errors. I think importing .gen is such a common case in practice. Do we have any workaround to make go treat it as folder instead of modules? like import "src/code.corp.internal/..../.gen/..."?

jayconrod commented 5 years ago

Is code.corp.internal/someotherdependency/.gen part of a module name, or is .gen just a directory within a module? As @bcmills said, leading dots are not allowed in module paths, but I think they're allowed in package paths.

We made several changes to the code that resolves package paths to modules, and the error checking is more aggressive than it used to be. I wonder if we're reporting an error there instead of simply disqualifying code.corp.internal/someotherdependency/.gen as a possible module path?

bcmills commented 5 years ago

leading dots are not allowed in module paths, but I think they're allowed in package paths.

That's not the behavior of module.CheckImportPath as of https://golang.org/cl/124378, but it's certainly possible that we're not calling CheckImportPath consistently for all import paths.

I believe that part of the purpose there is to prevent the creation of “hidden packages” (since dotfiles are hidden in many platforms) in the user's module cache, GOPATH/src, and (especially) vendor directory. That way, a module's dependencies can be audited, for example, by (visually) walking the packages in the vendor tree.

xyt0056 commented 5 years ago

Is code.corp.internal/someotherdependency/.gen part of a module name, or is .gen just a directory within a module?

It's a directory in the dependency and the .gen folder is checked in VCS. Is package path completely deprecated in GO111MODULE mode? How does the code "resolve package paths to modules"?

bcmills commented 5 years ago

If it's a directory within the dependency, then go mod tidy should not be trying to resolve a module for it in the first place.

Do go list all, go build ./..., and/or go test ./... report the same errors?

xyt0056 commented 5 years ago

go list all returns fine

we use bazel instead of go build

bcmills commented 5 years ago

go mod tidy is intended for use with the go command for builds. bazel intentionally supports code layouts that the go command does not, so it is not surprising that go mod tidy produces an error for those layouts.

If go build can build the program, then go mod tidy should certainly be able to tidy it, but if go build cannot build it then we do not expect go mod tidy to work either.

xyt0056 commented 5 years ago

If it's a directory within the dependency, then go mod tidy should not be trying to resolve a module for it in the first place.

Ok I just tried. go build repo1 shoots out error same as #35048. Then I fixed with instructions there by

  1. moving generated code from vendor/thriftrw to src/code.corp.internal/
  2. replacing import path of thriftrw/... to code.corp.internal/thriftrw

now go build succeeds. But go mod tidy still errors

code.corp.internal/repo1/project1 imports
    code.corp.internal/some/dependency tested by
    code.corp.internal/some/dependency.test imports
    code.corp.internal/some/dependency/.gen/config: malformed module path "code.corp.internal/some/dependency/.gen/config": leading dot in path element
bcmills commented 5 years ago

Does go test -c code.corp.internal/some/dependency succeed?

Bear in mind that go mod tidy scans the full transitive dependencies of the packages within your module, whereas go build only scans the non-test dependencies.

xytan0056 commented 5 years ago

Does go test -c code.corp.internal/some/dependency succeed?

I got

can't load package: package code.corp.internal/some/dependency: module code.corp.internal/some/dependency@latest (vx.x.x) found, but does not contain package code.corp.internal/some/dependency
xytan0056 commented 5 years ago

Let's say we have an internal module that has import xxx/.gen/xxx. Is this not allowed at all in go1.13, if we depend on this module? So we have to fix the internal module's import?

I wonder if this is true for all the other github projects, that they're all enforced to not have .gen import

bcmills commented 5 years ago

@xytan0056, again, such a path has never been allowed in module mode. Is there reason to believe that this was ever a common pattern for packages buildable with the go command?

xyt0056 commented 5 years ago

such a path has never been allowed in module mode. Is there reason to believe that this was ever a common pattern for packages buildable with the go command?

@bcmills I understand. Surprised that it's disallowed all this time. It works when we were in package mode. I'm trying to make it work in module mode. I'm wondering if this requirement is enforced for all dependent modules as well. Or just the imports in main module? Does it mean if I depend on any external module that is not module-aware and has .gen in its import statements (I'm pretty sure there're some in github), I'll see this error in my project?

If it's a directory within the dependency, then go mod tidy should not be trying to resolve a module for it in the first place.

I'm confused. .gen folder is part of a module downloaded. and it has code importing this .gen. Do you mean this is a go toolchain issue?

Is it possibly because I have the same module prefix as my dependency? repo layout

$GOPATH
--pkg  <--- dependency also imports "code.corp.internal/somepath/.gen/somepath"
--src
----code.corp.internal
------repo1
------repo2
------go.mod  <--- module name "code.corp.internal"
xytan0056 commented 4 years ago

Update: It looks like go 1.13 mod tidy starts to check imports format in dependency's test files.

example

  1. https://github.com/xytan0056/gomodexp is the happy case where it imports both depwithgen and depwithgen/.gen. go mod tidy succeeds.

  2. https://github.com/xytan0056/gomodnogen simply imports depwithoutgen/api, which doesn't have .gen checked in. Although go build/run/testsucceeds, go mod tidy fails with

    $ go1.13 mod tidy
    go: finding github.com/xytan0056/depwithoutgen latest
    github.com/xytan0056/gomodnogen imports
    github.com/xytan0056/depwithoutgen/api imports
    github.com/xytan0056/depwithoutgen/.gen/client: malformed module path "github.com/xytan0056/depwithoutgen/.gen/client": leading dot in path element
  3. github.com/xytan0056/gomodnorequire imports depwithgen's .gen in its test , same as test in gomodexp(happy case). But doesn't require it in go.mod. Although, go build/run succeeds, go test and go mod tidy fail with

    $ go1.13 mod tidy
    go: finding github.com/xytan0056/depwithgen latest
    github.com/xytan0056/gomodnorequire tested by
    github.com/xytan0056/gomodnorequire.test imports
    github.com/xytan0056/depwithgen/.gen/client: malformed module path "github.com/xytan0056/depwithgen/.gen/client": leading dot in path element

All of the above succeeds on go1.12 with GO111MODULE=on

What is the recommendation for generated code with leading dots in general? What is the recommendation for generated code used in dependency's test files?

jayconrod commented 4 years ago

@xytan0056 Thanks for investigating and publishing those repositories.

It looks like go1.13 does not check import paths all the time. It reports this error when looking up a module for an imported package that is not provided by any module. In this case, there is no module that provides github.com/xytan0056/depwithgen/.gen/client, and go1.13 verifies the module path before requesting a potentially invalid module from a proxy.

I think this error is correct, and actually we ought to be reporting it in more changes. However, I don't think we should make a change like that without understanding how many people that will break, especially at this point in the freeze. We could improve the error message though. It should say malformed import path or malformed package path instead of malformed module path.

What is the recommendation for generated code with leading dots in general? What is the recommendation for generated code used in dependency's test files?

Avoid leading or trailing dots in path names. module.CheckImportPath is the code that should check this.

Generated code is generally fine. It should just be checked into a directory that follows these rules.

A valid import path consists of one or more valid path elements separated by slashes (U+002F). (It must not begin with nor end in a slash.)

A valid path element is a non-empty string made up of ASCII letters, ASCII digits, and limited ASCII punctuation: + - . _ and ~. It must not begin or end with a dot (U+002E), nor contain two dots in a row.

ianlancetaylor commented 4 years ago

@jayconrod Is there anything to do for this issue for the 1.14 release? Any doc updates?

jayconrod commented 4 years ago

@ianlancetaylor I think we just need to clarify the wording in an error message. Doesn't need to go into the release notes.

ianlancetaylor commented 4 years ago

Moving milestone to backlog.

liasece commented 3 years ago

Our project does not have this problem with go1.15.7 darwin/amd64, but it occurs with go1.16beta1 darwin/arm64. @ianlancetaylor @dmitshur

bcmills commented 3 years ago

@liasece, please file a separate issue with steps to reproduce (but first see the changes to defaults described at https://tip.golang.org/doc/go1.16#go-command).

perj commented 3 years ago

Unfortunate decision, that I also noticed now in go 1.16 rc1. Oh well, I'll learn to live with it I guess.

bcmills commented 3 years ago

Barring any surprising new information, this behavior of the go command is unlikely to change. See https://github.com/golang/go/issues/43985#issuecomment-777809802 for more detail.

gopherbot commented 3 years ago

Change https://golang.org/cl/297530 mentions this issue: cmd: upgrade golang.org/x/mod to relax import path check

gopherbot commented 3 years ago

Change https://golang.org/cl/297634 mentions this issue: cmd/go: test remote lookup of packages with leading dots in path elements

gopherbot commented 3 years ago

Change https://golang.org/cl/297912 mentions this issue: [release-branch.go1.16] cmd: upgrade golang.org/x/mod to relax import path check