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

x/tools/gopls: go directive added to go.mod with tempModfile = true #36247

Closed
myitcv opened this issue Dec 22, 2019 · 22 comments
Closed

x/tools/gopls: go directive added to go.mod with tempModfile = true #36247

myitcv opened this issue Dec 22, 2019 · 22 comments
Assignees
Milestone

Comments

@myitcv
Copy link
Member

@myitcv myitcv commented Dec 22, 2019

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

$ go version
go version devel +5c6f42773c Thu Dec 19 22:16:18 2019 +0000 linux/amd64
$ go list -m golang.org/x/tools
golang.org/x/tools v0.0.0-20191220234730-f13409bbebaf
$ go list -m golang.org/x/tools/gopls
golang.org/x/tools/gopls v0.1.8-0.20191220234730-f13409bbebaf

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="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/myitcv/.cache/go-build"
GOENV="/home/myitcv/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/myitcv/gostuff"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/myitcv/gos"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/myitcv/gos/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/myitcv/gostuff/src/github.com/myitcv/govim/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-build670917404=/tmp/go-build -gno-record-gcc-switches"

What did you do?

govim/govim#634 adds support to govim for the tempModfile option in gopls.

As part of that PR we have added a test, only run for Go 1.14, to verify that setting tempModfile = true leaves a go.mod untouched.

However we are seeing that despite this option setting, a go language directive is being added to the go.mod.

The test setup is as follows:

-- go.mod --
module mod.com

-- main.go --
package main

import "example.com/blah"

func main() {
	println(blah.Name)
}

We:

  • open main.go
  • wait for a diagnostic to be received for main.go
  • save main.go (with no changes)

What did you expect to see?

go.mod to be unchanged, with no go directive and no require directive (for example.com/blah) added.

What did you see instead?

The go.mod file changed with a go directive added, meaning we end up with a go.mod file that looks like:

module mod.com

go 1.14

cc @stamblerre @ridersofrohan

@gopherbot gopherbot added this to the Unreleased milestone Dec 22, 2019
@gopherbot gopherbot added the Tools label Dec 22, 2019
@ridersofrohan

This comment has been minimized.

Copy link

@ridersofrohan ridersofrohan commented Dec 23, 2019

I'll start looking into this.

@ridersofrohan

This comment has been minimized.

Copy link

@ridersofrohan ridersofrohan commented Dec 23, 2019

@myitcv it looks like #34842 has a discussion about adding the "go" directive whenever we run "go list". I think this is intended behavior on the part of that command.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Dec 23, 2019

Change https://golang.org/cl/212477 mentions this issue: internal/lsp: modify how we check for go 1.14 for modfiles

@ridersofrohan ridersofrohan self-assigned this Dec 23, 2019
@stamblerre stamblerre modified the milestones: Unreleased, gopls v1.0 Dec 23, 2019
@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Dec 28, 2019

I think this is intended behavior on the part of that command.

@ridersofrohan yes, that matches my understanding of cmd/go.

But this is not expected behaviour when running with tempModfile = true. Because in that mode (per #34506) it was my understanding we are not expecting any changes to be made to the go.mod file at all.

@ridersofrohan

This comment has been minimized.

Copy link

@ridersofrohan ridersofrohan commented Jan 3, 2020

Ahh I see what you mean, the above CL should fix this issue when it gets merged in.

@ridersofrohan

This comment has been minimized.

Copy link

@ridersofrohan ridersofrohan commented Jan 8, 2020

@myitcv let me know if this is working as you expect it to now and I will close the issue.

@ridersofrohan ridersofrohan reopened this Jan 8, 2020
@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jan 9, 2020

Thanks @ridersofrohan

Per #36463 (comment) it's a little tricky for us to test this right now because your fix landed after https://go-review.googlesource.com/c/tools/+/212102 which introduced the problems described in #36463

I'll see whether I can cherry-pick this commit and test.

@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jan 9, 2020

@ridersofrohan I just tested with 53017a39 plus 11e9d9cc and acbc0e3b cherry-picked on top. Following the exact same test as before, I am now seeing go.mod be updated to include not only the go directive but also the requirement. i.e. it's almost as if the configuration value is being ignored entirely.

Is there a test on the gopls side that verifies this is working?

It might well be my cherry-picking has failed...

@ridersofrohan

This comment has been minimized.

Copy link

@ridersofrohan ridersofrohan commented Jan 9, 2020

@myitcv while working on a different issue, I found a potential problem with the previous CL. This should be addressed in commit/f270e23f which just got merged in. Let me know if this fixes that issue, I will also look into adding a test in gopls to check for this.

@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jan 9, 2020

Thanks - checked against 700ee261 and all seems to be sorted now.

I will also look into adding a test in gopls to check for this

Thanks - otherwise it's really impossible to know if/when this gets broken.

@myitcv myitcv closed this Jan 9, 2020
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 10, 2020

Change https://golang.org/cl/214260 mentions this issue: internal/lsp/mod: test go.mod unchanged when tempModfile=true

gopherbot pushed a commit to golang/tools that referenced this issue Jan 10, 2020
This change adds a test to ensure that your go.mod file remains unchanged when the tempModfile flag is activated. Specifically, it adds a test to ensure that a go directive does not get added to a user's go.mod file when there was not one included before.

Updates golang/go#36247

Change-Id: If8db5508ace5b7222112408255ffa66e4d38797f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/214260
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jan 23, 2020

@ridersofrohan we're seeing an issue here again against x/tools and gopls d456b1cd8c86.

Repro via dockexec:

cd $(mktemp -d)
git clone https://github.com/govim/govim
cd govim
git fetch origin pull/634/head && git checkout FETCH_HEAD
sed -i '/skip/d' cmd/govim/testdata/scenario_tempmodfile/modfile_changes.txt
go test -exec="dockexec govim/govim:go1.14beta1_vim_v8.2.0025_v1 -t -e VIM_FLAVOR=vim -e GOVIM_ERRLOGMATCH_WAIT=5s" -count=1 -v -run "TestScripts/scenario_tempmodfile/modfile_changes" ./cmd/govim

gives:

    TestScripts/scenario_tempmodfile/modfile_changes: testscript.go:348:
        WORK=$WORK
        PATH=/vim/bin:/go/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
        HOME=$WORK/.home
        TMPDIR=$WORK/_tmp
        devnull=/dev/null
        :=:
        exe=
        GOPATH=$WORK/.home/gopath
        CCACHE_DISABLE=1
        GOARCH=amd64
        GOOS=linux
        GOROOT=/go
        GOCACHE=/gocache
        GOPROXY=http://127.0.0.1:34127/mod
        goversion=1.14
        GOVIM_ERRLOGMATCH_WAIT=5s
        GOVIM_DETECT_USER_BUSY=false
        GONOSUMDB=*
        PLUGIN_PATH=/start
        GOVIMTEST_SOCKET=127.0.0.1:45987
        GOVIMTESTDRIVER_SOCKET=127.0.0.1:45405

        # A simple test that verifies we get a diagnostic for changes
        # that need to be made to our go.mod file
        # Open main.go and save to ensure we are actually doing something via gopls (0.154s)
        > vim ex 'e main.go'
        > vim ex 'w'
        # Sleep because we don't have an event we are waiting for (5.002s)
        > sleep $GOVIM_ERRLOGMATCH_WAIT
        # Verify that there have been no changes to go.mod (0.006s)
        > cmp go.mod go.mod.golden
        [diff -go.mod +go.mod.golden]
         module mod.com

        -go 1.14

        FAIL: testdata/scenario_tempmodfile/modfile_changes.txt:15: go.mod and go.mod.golden differ

--- FAIL: TestScripts (1.35s)
    --- FAIL: TestScripts/scenario_tempmodfile (0.04s)
        --- FAIL: TestScripts/scenario_tempmodfile/modfile_changes (5.39s)
@ridersofrohan

This comment has been minimized.

Copy link

@ridersofrohan ridersofrohan commented Jan 23, 2020

@myitcv I'm looking into it now.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 23, 2020

Change https://golang.org/cl/215978 mentions this issue: internal/imports: add buildflags to ProcessEnv

@stamblerre stamblerre modified the milestones: gopls/v1.0.0, gopls/v0.3.0 Jan 23, 2020
gopherbot pushed a commit to golang/tools that referenced this issue Jan 23, 2020
This change adds a buildflags variable to the ProcessEnv inside internal/imports. When you run go list with GO111MODULE=on to get information about the package you are in, it will add a go directive to your go.mod file if there is not one. With the tempModfile=on flag, there should be no changes to a user's go.mod file.

Updates golang/go#36247

Change-Id: I817e4c46b4f433d0665fcb7585fcdf4f87049a38
Reviewed-on: https://go-review.googlesource.com/c/tools/+/215978
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
@ridersofrohan

This comment has been minimized.

Copy link

@ridersofrohan ridersofrohan commented Jan 23, 2020

@myitcv ^ the above CL should have fixed this issue. Sorry for missing this the first time, let me know if it works or still shows up.

@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jan 23, 2020

@ridersofrohan. No problems. Did the repro above work for you?

@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jan 23, 2020

Sorry, that was unclear. Did the test in the repro above pass with the changes?

@ridersofrohan

This comment has been minimized.

Copy link

@ridersofrohan ridersofrohan commented Jan 23, 2020

I'm not sure how to run that test. I just manually tested it using VSCode.

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jan 23, 2020

@myitcv, I tried to repro with your example, but how do I pass it my local version of gopls?

@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik commented Jan 24, 2020

With gopls from master and Go 1.14, the test gets past the cmp and fails later:

        # Verify that there have been no changes to go.mod (0.000s)
        > cmp go.mod go.mod.golden
        # Verify that we have no diagnostics for any files (25.766s)
        > vimexprwait errors.empty getqflist()
        [stderr]
        -[]
        [exit status 1]
        FAIL: testdata/scenario_tempmodfile/modfile_changes.txt:18: unexpected command failure

Dunno what that means but I would think this bug, at least, is fixed.

@stamblerre stamblerre closed this Jan 24, 2020
@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jan 26, 2020

I would think this bug, at least, is fixed.

It is. Thanks all

@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jan 26, 2020

@stamblerre

how do I pass it my local version of gopls?

Not currently possible, but will be once govim/govim#720 lands

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
5 participants
You can’t perform that action at this time.