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

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

Closed
xyt0056 opened this issue Oct 18, 2019 · 27 comments
Closed

Comments

@xyt0056
Copy link

xyt0056 commented Oct 18, 2019

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?

@xyt0056 xyt0056 changed the title leading dot in path elementa "leading dot in path element" for generated code in go 1.13 Oct 18, 2019
@ianlancetaylor
Copy link
Contributor

CC @bcmills @jayconrod

@ianlancetaylor ianlancetaylor changed the title "leading dot in path element" for generated code in go 1.13 cmd/go: "leading dot in path element" for generated code in go 1.13 Oct 18, 2019
@ianlancetaylor ianlancetaylor added modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Oct 18, 2019
@ianlancetaylor ianlancetaylor added this to the Go1.14 milestone Oct 18, 2019
@bcmills
Copy link
Contributor

bcmills commented Oct 18, 2019

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
Copy link
Author

xyt0056 commented Oct 18, 2019

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
Copy link
Contributor

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
Copy link
Contributor

bcmills commented Oct 18, 2019

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.

@bcmills bcmills changed the title cmd/go: "leading dot in path element" for generated code in go 1.13 cmd/go: "leading dot in path element" for previously-undiagnosed paths in go 1.13 Oct 21, 2019
@xyt0056
Copy link
Author

xyt0056 commented Oct 21, 2019

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
Copy link
Contributor

bcmills commented Oct 21, 2019

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?

@bcmills bcmills added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Oct 21, 2019
@xyt0056
Copy link
Author

xyt0056 commented Oct 21, 2019

go list all returns fine

we use bazel instead of go build

@bcmills
Copy link
Contributor

bcmills commented Oct 21, 2019

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.

@bcmills bcmills added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Oct 21, 2019
@xyt0056
Copy link
Author

xyt0056 commented Oct 21, 2019

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
Copy link
Contributor

bcmills commented Oct 22, 2019

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.

@bcmills bcmills added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Oct 22, 2019
@xytan0056
Copy link

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
Copy link

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
Copy link
Contributor

bcmills commented Nov 7, 2019

@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
Copy link
Author

xyt0056 commented Nov 7, 2019

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"

@xyt0056 xyt0056 closed this as completed Nov 14, 2019
@xyt0056 xyt0056 reopened this Nov 14, 2019
@xytan0056
Copy link

xytan0056 commented Nov 30, 2019

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
  1. 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
Copy link
Contributor

@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
Copy link
Contributor

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

@jayconrod
Copy link
Contributor

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

@ianlancetaylor
Copy link
Contributor

Moving milestone to backlog.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.15, Backlog Jun 15, 2020
@ianlancetaylor ianlancetaylor removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jun 15, 2020
@liasece
Copy link

liasece commented Jan 23, 2021

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

@bcmills
Copy link
Contributor

bcmills commented Jan 25, 2021

@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
Copy link

perj commented Feb 5, 2021

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

@bcmills bcmills added Unfortunate and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Feb 12, 2021
@bcmills
Copy link
Contributor

bcmills commented Feb 12, 2021

Barring any surprising new information, this behavior of the go command is unlikely to change. See #43985 (comment) for more detail.

@bcmills bcmills closed this as completed Feb 12, 2021
@golang golang locked as resolved and limited conversation to collaborators Feb 12, 2021
@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

9 participants