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: go mod tidy error when importing std package in later go version #40067

Closed
mvertes opened this issue Jul 6, 2020 · 19 comments
Closed

cmd/go: go mod tidy error when importing std package in later go version #40067

mvertes opened this issue Jul 6, 2020 · 19 comments

Comments

@mvertes
Copy link

@mvertes mvertes commented Jul 6, 2020

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

$ go version
go version go1.13.12 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/marc/.cache/go-build"
GOENV="/home/marc/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/marc/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/marc/sdk/go1.13.12"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/marc/sdk/go1.13.12/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/marc/go/src/github.com/containous/yaegi/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-build070856350=/tmp/go-build -gno-record-gcc-switches

What did you do?

consider a sample module with a following file sample.go:

// +build go1.14

package sample

include _ "hash/maphash"       // this package exists in go1.14 but not in go1.13

and a go.mod:

module example.com/sample

go 1.12

And then in this module, running the following command:

go1.13.12 mod tidy

What did you expect to see?

no errors

As hash/maphash package is not present in go1.13 (introduced in go1.14), but the sample.go files should be skipped by go1.13 thanks to the build tag // +build go1.14

What did you see instead?

github.com/containous/yaegi/stdlib imports
        hash/maphash: malformed module path "hash/maphash": missing dot in first path element

I believe that a similar issue occurs in the latest release, maybe with a different error message.

@jayconrod jayconrod changed the title go mod tidy not following go version build constraints cmd/go: go mod tidy not following build constraints for go version Jul 6, 2020
@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Jul 6, 2020

This is mostly working as intended, but the error seems very frustrating.

go mod tidy ignores build constraints when deciding what dependencies are needed. If it didn't, it would remove platform-specific dependencies (for example, modules providing packages only imported from files with // +build windows). That would be frustrating for teams working in mixed environments. (As an exception, files with the ignore tag are still excluded, as are files in testdata directories or directories starting with . or _).

In this case though, tidy in 1.13 treats hash/maphash as a regular import and reports an error. Since there's no dot in the first path element, and there's no required module that could provide that package, perhaps tidy should assume it's a standard package in a future version of Go and ignore it.

cc @bcmills @matloob

@jayconrod jayconrod changed the title cmd/go: go mod tidy not following build constraints for go version cmd/go: go mod tidy error when importing std package in later go version Jul 6, 2020
@jayconrod jayconrod added this to the Go1.16 milestone Jul 6, 2020
@mpl
Copy link
Contributor

@mpl mpl commented Jul 8, 2020

By the way, this problem (as expected) also occurs with go mod vendor.

@bcmills
Copy link
Member

@bcmills bcmills commented Jul 17, 2020

I'm not sure that burying the error would be a good idea: we can't in general tell whether the missing hash/maphash is a new standard-library package, or a missing dependency that should be slotted in using a replace directive.

I would be more comfortable with a more narrowly targeted fix. For example, perhaps we should only ignore plausibly-standard-library imports if they occur in files that would normally be excluded by a build constraint.

@bcmills
Copy link
Member

@bcmills bcmills commented Jul 17, 2020

example.com$ go1.13.14 build

example.com$ go1.14.6 build

example.com$ go1.13.14 mod tidy
example.com imports
        hash/maphash: malformed module path "hash/maphash": missing dot in first path element

example.com$ go1.14.6 mod tidy

-- all.go --
package example
-- example114.go --
// +build go1.14

package example

import _ "hash/maphash"
-- go.mod --
module example.com

go 1.14
@bcmills
Copy link
Member

@bcmills bcmills commented Sep 18, 2020

I'm planning to add a -e flag to mod tidy in 1.16. That won't completely cover this use-case (it won't heuristically suppress the errors when the -e flag is not set), but I think it will mitigate it enough that we can lower the priority of a more tailored fix.

@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Nov 21, 2020

Hit this in golang-migrate, which added an io/fs migration driver for 1.16, but then fails to tidy both in the repo itself or in importers:

github.com/golang-migrate/migrate/v4/source/iofs imports
	io/fs: package io/fs is not in GOROOT (/usr/lib/go/src/io/fs)
github.com/golang-migrate/migrate/v4/source/iofs tested by
	github.com/golang-migrate/migrate/v4/source/iofs.test imports
	embed: package embed is not in GOROOT (/usr/lib/go/src/embed)
github.com/hortbot/hortbot/internal/db/migrations imports
	github.com/golang-migrate/migrate/v4/database/postgres tested by
	github.com/golang-migrate/migrate/v4/database/postgres.test imports
	github.com/golang-migrate/migrate/v4/source/file imports
	github.com/golang-migrate/migrate/v4/source/iofs imports
	io/fs: package io/fs is not in GOROOT (/usr/lib/go/src/io/fs)
github.com/hortbot/hortbot/internal/db/migrations imports
	github.com/golang-migrate/migrate/v4/database/postgres tested by
	github.com/golang-migrate/migrate/v4/database/postgres.test imports
	github.com/golang-migrate/migrate/v4/source/file imports
	github.com/golang-migrate/migrate/v4/source/iofs tested by
	github.com/golang-migrate/migrate/v4/source/iofs.test imports
	embed: package embed is not in GOROOT (/usr/lib/go/src/embed)

It probably wasn't a big deal when it was only maphash (a bit niche), but this probably is going to become more prevalent given the impact io/fs and embed are likely to make and the wide range of libs like migration tools / config libraries that are likely to want to use these new packages but also maintain lower minimum Go versions.

@abursavich
Copy link

@abursavich abursavich commented Dec 30, 2020

I recently hit this issue when I made a shim to support io/fs for another project that will be unable to upgrade to Go 1.16 quickly.

I think the real solution is to decouple the standard library (or as much of it as possible) from the language and tooling releases. This would give the standard library much more flexibility to evolve separately from the language and enable API-breaking v2 changes (probably after generics land).

Obviously, this would require a fully thought-out proposal, but the straw man version off the top of my head (ignoring any intricacies of splitting up the stdlib into multiple modules, dealing with their inter-dependencies, and blessed access to runtime internals)... Go 1.x would move to an external stdlib (a snapshot of which would be packaged with the language for self-hosting) and it would implicitly replace usages in pre-Go 1.x modules with their external v1 counterparts (e.g. replace io => golang.org/pkg/io). A go fix rule could update all import paths for modules making the transition.

@bcmills
Copy link
Member

@bcmills bcmills commented Jan 6, 2021

@abursavich, #30241 is a more fleshed-out proposal for standard-library vendoring. Some parts of the standard library (such as embed) are tightly coupled to the compiler, linker, and/or runtime, and so those parts necessarily cannot be released as standalone (and independently-upgradable) modules.

The parts of the standard library that are not tightly coupled to the toolchain or runtime could be rewritten as forwarding shims to modules outside the standard library, which could then (under that proposal) be upgraded above — but not downgraded below — the versions required by the standard library.

See #30241 for more detail and discussion on that topic.

@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Jan 6, 2021

I was thinking about this the other day; would it make sense to add a mechanism to ignore these packages' absence during commands that don't know about build tags (e.g., tidy), then once newer versions of Go are released, backport a list of the new packages to the supported releases so that they know that the new packages are "okay"?

Given only the previous two releases are supported and the number of new packages added per release are small, I can see this working reasonably well. I recall a few releases ago (1.9 and 1.10?) where some changes were backported to help with the introduction of modules in 1.11, and I find that similar in intention.

@tv42
Copy link

@tv42 tv42 commented Jan 11, 2021

@bcmills

or a missing dependency that should be slotted in using a replace directive.

Wouldn't the standard answer be for such replace directives to also be using leading import path segments that don't look like stdlib imports, for exactly this reason?

Anyone outside of stdlib using a dotless import path first segment is just asking for trouble, conflicting with future stdlib additions.

@bcmills
Copy link
Member

@bcmills bcmills commented Jan 11, 2021

Wouldn't the standard answer be for such replace directives to also be using leading import path segments that don't look like stdlib imports, for exactly this reason?

Yes.

Anyone outside of stdlib using a dotless import path first segment is just asking for trouble, conflicting with future stdlib additions.

Agreed, but plenty of existing Go code (especially closed-source Go code) is written in that style. At this point I do not want to make the migration to modules more difficult or confusing than it already is for those users.

@avivataqua
Copy link

@avivataqua avivataqua commented Feb 16, 2021

I've encountered this issue with a somewhat different use case
if anyone can suggest a better solution/work-around for this case, I'd appreciate it
i have a single code base that i compile one with go1.15.6 and once with go1.15.6b5 (dev.boringcrypto branch)
I rely on a build constraint tag to avoid compiling go files that import 'crypto/tls/fipsonly
mod vendor fails with 'not in GOROOT' for go1.15.6

@bcmills
Copy link
Member

@bcmills bcmills commented Feb 17, 2021

@avivataqua, for now you can run go mod tidy -e using Go 1.16.

(Longer term, I think we should probably change go mod tidy to automatically suppress errors for missing imports in build-constrained files.)

@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Feb 18, 2021

This now affects golang.org/x/tools/godoc/vfs, as CL 291669 added io/fs usage guarded by a build tag to an existing package.

Given:

package main

import (
	"golang.org/x/tools/godoc/vfs"
)

func main() {
	var _ vfs.FileSystem
}

And go.mod:

module some.tld/vfsthing

go 1.15

require golang.org/x/tools v0.1.1-0.20210217234408-19ff21fbe961

go mod tidy with Go 1.15 says:

some.tld/vfsthing imports
	golang.org/x/tools/godoc/vfs imports
	io/fs: package io/fs is not in GOROOT (/usr/lib/go/src/io/fs)

This doesn't affect me personally, but I would sort of expect people to see this in the wild.

While this is caused by this issue, I can open a new issue just for x/tools/godoc/vfs if desired, if a new package needs to be created instead, like how mapfs and httpfs exist.

cc @rsc (since it was his CL)

@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Feb 19, 2021

And now x/tools/go/analysis/passes/buildtag depends on the new go/build/constraint package via CL 293834, so the tool I wrote to manage my $GOBIN via modules fails because it cannot tidy when pulling in gopls anymore:

tmpmod imports
	golang.org/x/tools/gopls imports
	golang.org/x/tools/gopls/internal/hooks imports
	golang.org/x/tools/internal/lsp/source imports
	golang.org/x/tools/go/analysis/passes/buildtag imports
	go/build/constraint: package go/build/constraint is not in GOROOT (/usr/lib/go/src/go/build/constraint)

(And I'm still waiting for my distribution to get Go 1.16, which probably won't happen until 1.16.1 to fix some of the ICE/miscompilations ☹️)

@myitcv
Copy link
Member

@myitcv myitcv commented Feb 19, 2021

Longer term, I think we should probably change go mod tidy to automatically suppress errors for missing imports in build-constrained files

I did wonder whether the heuristic could be made more specific so that go mod tidy does not ignore the go* build constraints.

Suppressing errors for missing imports in build-constrained files "feels" too broad.

@myitcv
Copy link
Member

@myitcv myitcv commented Feb 19, 2021

for now you can run go mod tidy -e using Go 1.16.

The snag here being that go mod tidy -e is required for Go 1.15X :)

Edit: the context for me saying that is the following example:

exec go mod tidy
-- go.mod --
module mod.com
go 1.15
require golang.org/x/tools v0.1.1-0.20210219012152-f3748ed8ec89
-- main.go --
package main
import (
	_ "golang.org/x/tools/go/analysis/passes/buildtag"
)

Works fine with go1.16, but fails with go1.15.8:

> exec go mod tidy
[stderr]
mod.com imports
        golang.org/x/tools/go/analysis/passes/buildtag imports
        go/build/constraint: package go/build/constraint is not in GOROOT (/home/myitcv/gos/src/go/build/constraint)
@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Feb 19, 2021

I did wonder whether the heuristic could be made more specific so that go mod tidy does not ignore the go* build constraints.

Suppressing errors for missing imports in build-constrained files "feels" too broad.

The approach I suggested in #40067 (comment) would be more targeted to known new packages, though when I suggested this I didn't realize boringcrypto also had its own special packages too (hopefully that's the end of the line).

@rsc
Copy link
Contributor

@rsc rsc commented Mar 2, 2021

Duplicate of #44557.

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