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

Fix issue with readonly go modules #1456

Closed
wants to merge 1 commit into from
Closed

Conversation

Felixoid
Copy link

@Felixoid Felixoid commented Jul 22, 2020

Hello.

As mentioned in issue ycm-core/YouCompleteMe#3721, --go-completer causes the state, when the YCM directory couldn't be deleted.

This is the intended behavior, see golang/go#27161. But in golang/go#31481 the flag -modcacherw was proposed, and in https://golang.org/cl/202079 it was implemented.

To keep backward compatibility, the GOFLAGS ENV is used.

This fix ycm-core/YouCompleteMe#3721


This change is Reviewable

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

THANKS!

Reviewed 1 of 1 files at r1.
Reviewable status: 1 of 2 LGTMs obtained

@codecov
Copy link

codecov bot commented Jul 22, 2020

Codecov Report

Merging #1456 into master will increase coverage by 1.15%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1456      +/-   ##
==========================================
+ Coverage   95.50%   96.66%   +1.15%     
==========================================
  Files          94       94              
  Lines        7324     7674     +350     
  Branches      174      174              
==========================================
+ Hits         6995     7418     +423     
+ Misses        247      205      -42     
+ Partials       82       51      -31     

Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Felixoid Thanks for this pull request.

Reviewable status: 1 of 2 LGTMs obtained (waiting on @Felixoid)


build.py, line 848 at r1 (raw file):

  new_env[ 'GOBIN' ] = p.join( new_env[ 'GOPATH' ], 'bin' )
  new_env[ 'GOFLAGS' ] = new_env.get( 'GOFLAGS', '' ) + ' -modcacherw'
  CheckCall( [ go, 'get', 'golang.org/x/tools/gopls@v0.4.2' ],

Instead of introducing GOFLAGS, is it okay to simply ad -modcacherw to the CheckCall list on the line below?

[ go, 'get', '-modcacherw', 'golang.org/x/tools/gopls@v0.4.2' ]

It seems to work, but I'm not sure if this, for whatever reason, messes up something else.
If there's no problem with the above, I'd prefer it that way, as the leading space in the GOFLAGS version can be missed easily.

@Felixoid
Copy link
Author

Hey @bstaletic.

I've decided to use GOFLAGS because of its description:

A space-separated list of -flag=value settings to apply to go commands by default, when the given flag is known by the current command.

Unfortunately, even when GOFLAGS is used, go elder than 1.14 returns an error:

docker run -it golang:1.12
root@fd1127a7af78:/go# GO111MODULE=on GOFLAGS="$GOFLAGS -modcacherw" go get golang.org/x/tools/gopls@v0.4.2
go: parsing $GOFLAGS: unknown flag -modcacherw

So, the usage of GOFLAGS here is equal to your proposal [ go, 'get', '-modcacherw', 'golang.org/x/tools/gopls@v0.4.2' ]

But now I have a question. How to implement version dependent logic for this part? The flag -modcacherw was implemented only in 1.14, and I don't feel good about breaking older versions.

@puremourning
Copy link
Member

The flag -modcacherw was implemented only in 1.14, and I don't feel good about breaking older versions.

Nor me. I don’t think we should do this then. Faffing with go versions is no it worth this change.

Maybe just document the solution (setenv GOFLAGS ...) in the issue for anyone else hitting it.

@Felixoid
Copy link
Author

Maybe it worths updating YCM README for go support as well? And --go-completer help string?

I like the idea of leaving this optional, and with GOFLAGS it's possible.

@puremourning
Copy link
Member

Maybe it worths updating YCM README for go support as well? And --go-completer help string?

Maybe put something on the faq (wiki) pointing to the GH issue and our suggested resolution

@bstaletic
Copy link
Collaborator

I mean, it should be possible to read the output of go version, parse it with a regex and compare it with 1.14. I wouldn't mind the extra complexity, but @puremourning seems to think that's too much.

@puremourning
Copy link
Member

Yeah it seems a slippery slope to me. The underlying problem is trivial to fix with chmod so I’m not going to lose sleep over it. But look, if the code is simple and tested then I probably won’t say no ...

@Felixoid
Copy link
Author

I think the logic around getting a version and so on is the unnecessary complexity increase. Since the local fix for vim-plug is trivial:

-Plug 'ycm-core/YouCompleteMe', { 'do': 'git submodule update --init --recursive; ./install.py --clangd-completer --go-completer' }
+Plug 'ycm-core/YouCompleteMe', { 'do': 'git submodule update --init --recursive && GOFLAGS=-modcacherw ./install.py --clangd-completer --go-completer' }

I'll add the note to https://github.com/ycm-core/YouCompleteMe/wiki/FAQ about the way to fix it. This PR could be reopened in some time, when versions <1.14 will be old enough.

@Felixoid Felixoid closed this Jul 23, 2020
@puremourning
Copy link
Member

Thanks for your contribution 🙏

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

Successfully merging this pull request may close these issues.

./install.py --go-completer creates files without write permission
3 participants