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

Closed
myitcv opened this issue Jan 29, 2021 · 22 comments
Labels
FrozenDueToAge GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done. Unfortunate
Milestone

Comments

@myitcv
Copy link
Member

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

@myitcv myitcv added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. GoCommand cmd/go labels Jan 29, 2021
@jayconrod
Copy link
Contributor

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
Contributor

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 commented Jan 29, 2021

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

Correct.

@myitcv
Copy link
Member Author

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

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 NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Feb 1, 2021
@jayconrod jayconrod added this to the Backlog milestone Feb 1, 2021
@jayconrod
Copy link
Contributor

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.

@buyology
Copy link

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
Contributor

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
@bcmills bcmills added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Feb 11, 2021
@gopherbot gopherbot removed the NeedsFix The path to resolution is known, but the work has not been done. label Feb 11, 2021
@gopherbot
Copy link

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

@bcmills
Copy link
Contributor

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
@bcmills bcmills added Unfortunate Documentation and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Feb 11, 2021
@dmitshur dmitshur added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Feb 12, 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

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)?

@mirogta
Copy link

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.

@jayconrod
Copy link
Contributor

It sounds like this issue is affecting more people than we'd hoped. I'll look at relaxing the constraint that import paths can't have path elements starting with dots. We'll aim to backport that to 1.16.1.

We'll continue to disallow trailing dots in import path elements, since those names aren't allowed on Windows. We'll also continue disallowing leading dots in module path elements.

@jayconrod jayconrod added NeedsFix The path to resolution is known, but the work has not been done. and removed Documentation WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Feb 26, 2021
@jayconrod jayconrod modified the milestones: Unplanned, Go1.16.1, Go1.17 Feb 26, 2021
@jayconrod
Copy link
Contributor

@gopherbot Please backport to 1.16.

@gopherbot
Copy link

Backport issue(s) opened: #44647 (for 1.16).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@jayconrod jayconrod self-assigned this Feb 26, 2021
@gopherbot
Copy link

Change https://golang.org/cl/297089 mentions this issue: module: allow leading dots in import path elements

gopherbot pushed a commit to golang/mod that referenced this issue Mar 1, 2021
These were always disallowed, but the restriction wasn't enforced in
most cases until Go 1.16. That's broken more projects than we hoped.

This change allows leading dots in import path elements. Leading dots
are still not allowed in module path elements. Leading dots were
always allowed in file path elements. Trailing dots are still
forbidden in all cases.

For golang/go#43985

Change-Id: Id9cf728a341931565ab9e81f600b2341aa178683
Reviewed-on: https://go-review.googlesource.com/c/mod/+/297089
Trust: Jay Conrod <jayconrod@google.com>
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@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 pushed a commit that referenced this issue Mar 10, 2021
…ents

Follow-up to CL 297530.

For #43985
For #34992

Change-Id: I2cfa6c41c013e627c3464c383ca42f5c9ebe521a
Reviewed-on: https://go-review.googlesource.com/c/go/+/297634
Trust: Jay Conrod <jayconrod@google.com>
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@golang golang locked and limited conversation to collaborators Mar 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done. Unfortunate
Projects
None yet
Development

No branches or pull requests

10 participants