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: "malformed import path" in Go 1.16 for packages with path elements containing a leading dot #43985

Open
myitcv opened this issue Jan 29, 2021 · 16 comments

Comments

@myitcv
Copy link
Member

@myitcv myitcv commented Jan 29, 2021

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

$ go version
go version devel +076a45acd5 Tue Oct 13 19:15:53 2020 +0000 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
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/myitcv/.cache/go-build"
GOENV="/home/myitcv/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/myitcv/gostuff/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/myitcv/gostuff"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/myitcv/dev/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/myitcv/dev/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
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-build308834115=/tmp/go-build -gno-record-gcc-switches"

What did you do?

testscript -v <<'EOD'
go list ./.github/workflows
stdout 'rubbish/\.github/workflows'

-- go.mod --
module rubbish

-- .github/workflows/gen.go --
package gen
EOD

What did you expect to see?

Passing test

What did you see instead?

malformed import path "rubbish/.github/workflows": leading dot in path element

This has the effect of breaking commands like go generate ./.github/workflows

I bisected this to 3a65abf

cc @bcmills @jayconrod

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Jan 29, 2021

Hmm, this package doesn't have a valid import path in module-aware mode. From module.CheckImportPath:

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.

However, I'm guessing this isn't a package that's ever imported. Perhaps the go command shouldn't enforce this requirement for packages named on the command line using local paths?

@bcmills @matloob WDYT?

@bcmills
Copy link
Member

@bcmills bcmills commented Jan 29, 2021

Hrm, I'm not sure. We disallow a leading dot in general because it can make code harder to audit: in environments that hide dotfiles by default we don't want to hide the source code for the dependencies of a Go package, and path elements beginning with _ and . are ignored by go mod tidy and go list ./... (compare #37376).

@myitcv
Copy link
Member Author

@myitcv myitcv commented Jan 29, 2021

However, I'm guessing this isn't a package that's ever imported

Correct.

@myitcv
Copy link
Member Author

@myitcv myitcv commented Feb 1, 2021

For a bit of context, I'm doing this because having a go:generate directive within $modroot/.github/workflows aligns the location of the directive with the target of the generation step. However, the argument against this is precisely the point that @bcmills makes - a simple go generate ./... doesn't end up running this directive. Rather awkwardly I actually use another go:generate directive in another package to run the "hidden" one, which kind of defeats the purpose. So whilst the original goal was perhaps admirable/sensible, the workaround arguably makes it more confusing/convoluted. Which perhaps points to this being a bad idea generally, and hence one that we should not support.

I'll leave it to @bcmills and @jayconrod to determine whether it's necessary to continue supporting this pattern given it used ti work in 1.15x or not. From my perspective I think you've helped talk me round so this issue can otherwise be closed 😄

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Feb 1, 2021

I think we should keep this open since it used to work, and in most other situations, the go command allows a package with an invalid import path to be named using a file path. Probably not for 1.16 at this point in the freeze though.

@jayconrod jayconrod added this to the Backlog milestone Feb 1, 2021
@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Feb 5, 2021

After looking at this a bit further, I think we should close this. It's pretty ambiguous though. If more folks run into this issue in new situations, we can reconsider.

  • As alluded to above, go generate ./... and go mod tidy won't find these packages, resulting in weirdness. That's working as intended, but it's probably better not to encourage broader use of things that trigger unexpected behavior.
  • This example worked in 1.15.x. It should not have worked. We only validated imports when looking up modules, not when locating packages provided by modules in the build list.
go build m
-- go.mod --
module m

go 1.15
-- .github/workflows/invalid.go --
package invalid
-- use.go --
package use

import _ "m/.github/workflows"
  • All things being equal, it's better to treat paths consistently and to report errors earlier. With the above example, someone might publish a module containing packages with invalid import paths, expecting them to work.
@jayconrod jayconrod closed this Feb 5, 2021
@buyology
Copy link

@buyology buyology commented Feb 11, 2021

Just tried building on 1.16rc1 and this breaks our builds:

$ go build -mod=vendor
../../vendor/go.uber.org/cadence/internal/client.go:30:2: malformed import path "go.uber.org/cadence/.gen/go/cadence/workflowserviceclient": leading dot in path element
../../vendor/go.uber.org/cadence/internal/internal_workflow_testsuite.go:39:2: malformed import path "go.uber.org/cadence/.gen/go/cadence/workflowservicetest": leading dot in path element
../../vendor/go.uber.org/cadence/internal/activity.go:29:2: malformed import path "go.uber.org/cadence/.gen/go/shared": leading dot in path element

Is there any workaround available?

@bcmills bcmills reopened this Feb 11, 2021
@bcmills
Copy link
Member

@bcmills bcmills commented Feb 11, 2021

See previously #34992.

@bcmills bcmills changed the title cmd/go: incorrect malformed import path when directory specified as input cmd/go: "malformed import path" in Go 1.16 for packages with path elements containing a leading dot Feb 11, 2021
@gopherbot gopherbot removed the NeedsFix label Feb 11, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Feb 11, 2021

Change https://golang.org/cl/291389 mentions this issue: doc/go1.16: note that package path elements beginning with '.' are disallowed

@bcmills
Copy link
Member

@bcmills bcmills commented Feb 11, 2021

I did some investigation, and found that the prohibition on leading-dot elements was added in CL 124378, back in July 2018.

We attempted to investigate the prevalence of these paths in the wild using the pkg.go.dev index, but it does not index imports of these paths (which further argues for disallowing them — so far no one seems to have reported the packages as missing there).

The only specific affected module we know of so far is go.uber.org/cadence, but because the affected import statement is an internal package it could be fixed for external consumers by an internal-only change within a patch release for that module (namely, moving the packages currently in the .gen subdirectory to another location and updating the imports within the module).

After discussion with @rsc, @jayconrod, and @matloob, we're planning to leave this validation in place for Go 1.16.

If we find more severe examples after the release, we can consider whether to relax the check in Go 1.16.1. If you are affected, please comment on this issue with the affected module path(s) and versions.

@bcmills bcmills modified the milestones: Backlog, Go1.16 Feb 11, 2021
gopherbot pushed a commit that referenced this issue Feb 12, 2021
…sallowed

For #43985

Change-Id: I1a16f66800c5c648703f0a0d2ad75024525a710f
Reviewed-on: https://go-review.googlesource.com/c/go/+/291389
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@odeke-em
Copy link
Member

@odeke-em odeke-em commented Feb 16, 2021

Thanks for the report Paul, and thanks for the discourse and debugging everyone. @bcmills you mailed and merged the documentation CL https://go-review.googlesource.com/c/go/+/291389, should we thus move this issue out of Go1.16 and roll it forward for Go1.17 and consider a backport if necessary for Go1.16.1, as you raised in #43985 (comment)?

@bcmills
Copy link
Member

@bcmills bcmills commented Feb 16, 2021

@odeke-em, I don't think this issue should be rolled forward to Go 1.17 since we don't plan to do anything more about it. Perhaps it should just be closed, but we discourage folks from commenting on closed issues and we are actually interested in receiving comments about concrete examples here.

Maybe Unplanned is the right milestone for that.

@bcmills bcmills modified the milestones: Go1.16, Unplanned Feb 16, 2021
@odeke-em
Copy link
Member

@odeke-em odeke-em commented Feb 16, 2021

@sagikazarmark
Copy link

@sagikazarmark sagikazarmark commented Feb 24, 2021

The only specific affected module we know of so far is go.uber.org/cadence, but because the affected import statement is an internal package it could be fixed for external consumers by an internal-only change within a patch release for that module (namely, moving the packages currently in the .gen subdirectory to another location and updating the imports within the module).

Actually, consumers of that package might also import packages from the .gen subdirectory, so fixing this in a patch release isn't trivial IMHO.

If it only gets fixed in the latest release then consumers will also be forced to upgrade their Cadence server component (to ensure compatibility between the client and the server) or to stay on Go 1.15 until they do.

@timuthy
Copy link

@timuthy timuthy commented Feb 24, 2021

If we find more severe examples after the release, we can consider whether to relax the check in Go 1.16.1. If you are affected, please comment on this issue with the affected module path(s) and versions.

The Gardener project (https://github.com/gardener) is affected by this change, too.
We use modules to define cross repo dependencies in the Gardener org, also some of the modules don't contain Go code at all:
For example, https://github.com/gardener/gardener/tree/master/.github/ISSUE_TEMPLATE allows us to re-use Github templates across our repositories with the same dependency management tool (go mod) already used for Go dependencies.

timuthy added a commit to gardener/terraformer that referenced this issue Feb 24, 2021
Due to import restrictions: golang/go#43985
rfranzke pushed a commit to gardener/terraformer that referenced this issue Feb 24, 2021
Due to import restrictions: golang/go#43985
@mirogta
Copy link

@mirogta mirogta commented Feb 25, 2021

This is an unexpected breaking change in Go 1.16 for all our builds using magefile/mage.

Raised initially here until I've found out that the issue is in go list rather than in mage.

All our mage build files are in a subdirectory which starts with a dot (.mage) and this has worked in Go 1.15.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
10 participants