Skip to content

Fixed issue Vim-Go broken on freebsd #1224 #1276

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
May 7, 2017
Merged

Fixed issue Vim-Go broken on freebsd #1224 #1276

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e9bb5ea
Fixed issue Vim-Go broken on freebsd #1224
creaky May 5, 2017
d92a4f6
Updated code based on feedback and further analysis
creaky May 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion autoload/go/util.vim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,13 @@ endfunction
function! go#util#System(str, ...) abort
let l:shell = &shell
if !go#util#IsWin() && executable('/bin/sh')
let &shell = '/bin/sh'
" Having the [t]csh shells set as the login shell results in temporary
" file access errors when calling ksh or dash system shells. /bin/sh is
" often aliased to these shells on modern Linux and BSD systems including
" VoidLinux, Debian and FreeBSD.
if match(l:shell, 'csh$') == -1
let &shell = '/bin/sh'
endif
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused. This will not reset the shell when using csh, right?

This will break commands that assume Bourne shell syntax. Resetting the shell to /bin/sh is a feature (I added this in #988).

Copy link
Contributor

@arp242 arp242 May 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

By the way, another way to do this is to introduce a new g:go_shell setting or some such. That's what Syntastic does (see vim-syntastic/syntastic#1355).

Copy link
Contributor

@arp242 arp242 May 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Also related: junegunn/vim-plug#159 – looks like you may ran in to the shellredir problem described there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, this change will NOT reset the shell when running under *csh family. So far this has not caused any compatibility problems.

The feature added in #988 breaks functionality and limits the portability of vim-go. The existing Windows bypass is evidence of this, and in the same vein, a bypass for *csh users (on systems using dash, zsh or ksh as /bin/sh) is needed until something more sophisticated is coded.

An option is to allow the user to specify an appropriate bourne like shell to drop down to ala g:go_shell specifying bash however do note FreeBSD and Solaris does not have bash installed by default. Further, this requires the user to do more configuring and really impacts the user experience of "it just works".

Alternatively, if someone has time, a very deep dive into why temporary file creation fails under dash/zsh/ksh as subshell to *csh and fix it would address a root cause of needing this workaround.

Best option is to keep command system calls simple, compatible with common shells and not be dependent on any shell specific behaviour, thus removing any need to override the user's shell.

So far, commands observed using this system call falls into the best option category and thus using the *csh skip is better than the alternative of doing nothing and breaking vim-go on a number of systems which is what is happening today.

Copy link
Contributor

@arp242 arp242 May 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This patch will almost certainly break things. I didn't add it just for fun, I added it because stuff broke ;-) I don't remember what exactly though, and unfortunately I neglected to mention what exactly in the commit/PR description.

What problems are you having? Did you try setting the shellredir option as mentioned? Because I think this will fix it, and should be what we're changing?

The feature added in #988 breaks functionality and limits the portability of vim-go.

?

It does exactly the opposite, by setting a known shell we ensure that all commands always work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What problems are you having?

The regression introduced by #988 causes the following problems in a *csh, zsh login environment on a system with /bin/sh pointing to dash, ksh or BSD sh.

Problem 1. Running 'vim test.go' results in errors and no template.
Problem 2. Unable to save a modified *.go file. Errors in save process prevents successful write.
Problem 3. Most vim-go commands resulting in calling the sub-shell fail with error.

Example error message produced from 'vim test.go':
Error detected while processing function <SNR>27_template_autocreate[3]..go#temp late#create[8]..go#tool#PackageName[2]..go#tool#ExecuteInDir[4]..go#util#env[7]. .go#util#goroot[1]..go#util#System: line 17: E484: Can't open file /tmp/vQAaQRG/1 Error detected while processing function <SNR>27_template_autocreate[3]..go#temp late#create[8]..go#tool#PackageName[2]..go#tool#ExecuteInDir[4]..go#util#env: line 7: E171: Missing :endif Error detected while processing function <SNR>27_template_autocreate: line 3: E171: Missing :endif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The shellredir is the right way to go. Expanding on the deep dive option mentioned above:

The 'shellredir' option controls what is captured by system(). From Vim help page: "The default is ">". For Unix, if the 'shell' option is "csh", "tcsh" or "zsh" during initializations, the default becomes ">&". If the 'shell' option is "sh", "ksh" or "bash" the default becomes ">%s 2>&1". This means that stderr is also included.

The initialization of this option is done after reading the ".vimrc" and the other initializations, so that when the 'shell' option is set there, the 'shellredir' option changes automatically unless it was explicitly set before."

So from #988 the shell was changed without updating shellredir to match which causes the error for the csh/tcsh/zsh users.

I have tested this successfully and will update the pull request.

endif

try
Expand Down