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

[RFC] Check for `man -l` since it's not supported on every system. #6766

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@raichoo
Contributor

raichoo commented May 18, 2017

man works differently on FreeBSD. This patch
makes nvim aware that it is running on FreeBSD.

@raichoo

This comment has been minimized.

Contributor

raichoo commented May 18, 2017

It would probably make sense to group all the BSDs (including MacOS) into a 'bsd' flag. This issue might occur on every other BSD as well.

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented May 18, 2017

It would really be logical to check what man supports (e.g. by parsing LANG=C man --version or trying to execute man with various flags) rather then check platform. You can install some BSD utilities on linux after all.

BTW, what about cygwin? I was working on babun for a while and it appeared to not support various GNU features (mainly bothered with ps, specifically absense of ps -C I use in an alias, but there are also other differences).

@jamessan

This comment has been minimized.

Member

jamessan commented May 18, 2017

It would probably make sense to group all the BSDs (including MacOS) into a 'bsd' flag. This issue might occur on every other BSD as well.

OpenBSD supports the -l flag, but you're right that NetBSD and FreeBSD don't.

It would really be logical to check what man supports

👍

Cc @nhooyr

@raichoo

This comment has been minimized.

Contributor

raichoo commented May 18, 2017

Having a 'freebsd' flag probably makes sense as well. There might be some subtle differences. I ran into a problem with xsel. It required me to install xsel-conrad since the regular xsel is not supported by nvim. Knowing that it's running on FreeBSD could emit a better error message on :CheckHealth when checking for clipboard support.

@justinmk

This comment has been minimized.

Member

justinmk commented May 18, 2017

Feature-testing is probably the best policy. Not in favor of extending has(). BSD users may have GNU man, I guess.

@jamessan

This comment has been minimized.

Member

jamessan commented May 18, 2017

BSD users may have GNU man, I guess.

Like on Debian/kFreebsd, for example.

@raichoo

This comment has been minimized.

Contributor

raichoo commented May 18, 2017

Rewriting the patch. Does man -l return with status code 0 when you run it on linux?

@raichoo raichoo force-pushed the raichoo:freebsd branch from e45f5a6 to 4172f76 May 18, 2017

@@ -12,6 +12,11 @@ catch /E145:/
" Ignore the error in restricted mode
endtry

try
call system('env MANPAGER=cat man -l man')

This comment has been minimized.

@ZyX-I

ZyX-I May 18, 2017

Contributor

Do not use system(string) if you can: system(['env', 'MANPAGER=cat', 'man', '-l', 'man']).


I (Gentoo linux) have exit code 1 on man -l man.

@@ -12,6 +12,11 @@ catch /E145:/
" Ignore the error in restricted mode
endtry

try

This comment has been minimized.

@ZyX-I

ZyX-I May 18, 2017

Contributor

What’s the point of try here? It is transforming all errors inside into exceptions and not catching after that, not what you would need at startup. I would suggest just silent! the call and remove try/endtry.

This comment has been minimized.

@raichoo

raichoo May 18, 2017

Contributor

It was a test. I didn't have a way to verify how linux does it but found an online shell interpreter.

@raichoo raichoo force-pushed the raichoo:freebsd branch 2 times, most recently from bf4482b to 5f5dc92 May 18, 2017

@raichoo

This comment has been minimized.

Contributor

raichoo commented May 18, 2017

Changed the -l flag test. I have no idea if that works on every system. Tried it on a shell interpreter I found online and my system. At least these seem to work as expected.

@raichoo raichoo changed the title from freebsd: make nvim aware of FreeBSD to Check for `man -l` since it's not supported on every system. May 18, 2017

@raichoo raichoo force-pushed the raichoo:freebsd branch from 5f5dc92 to 23e79c1 May 18, 2017

@raichoo raichoo changed the title from Check for `man -l` since it's not supported on every system. to [RFC] Check for `man -l` since it's not supported on every system. May 18, 2017

@@ -8,6 +8,8 @@ try
if !has('win32') && $OSTYPE !~? 'cygwin\|linux' && system('uname -s') =~? 'SunOS' && system('uname -r') =~# '^5'
let s:man_find_arg = '-l'
endif
call system('env MANPAGER=cat man -l ' . system('man -w man') . ' > /dev/null')

This comment has been minimized.

@jamessan

jamessan May 18, 2017

Member

I'd suggest using the s:system function that's defined in the file and using an array, instead of a string (as @ZyX-I already mentioned).

This comment has been minimized.

@raichoo

raichoo May 18, 2017

Contributor

s:system fails with "Unknown function: 71_system" probably because it's not in scope yet. But I certainly gave up on understanding how VimL scoping actually works.

@raichoo raichoo force-pushed the raichoo:freebsd branch 5 times, most recently from 6c940e1 to 40875e6 May 18, 2017

@raichoo

This comment has been minimized.

Contributor

raichoo commented May 18, 2017

Had another typo in there '-k' should be '-w'.

@raichoo

This comment has been minimized.

Contributor

raichoo commented May 18, 2017

@jamessan OK, I've moved the s:system function into scope. Turned this change into a separate commit since I'm not sure it this is really the proper way to do it, but it takes your suggestion into regard. Pick whichever solution is better and I can improve on that if necessary.

@raichoo raichoo force-pushed the raichoo:freebsd branch 2 times, most recently from 968ac05 to 66185dd May 18, 2017

let s:man_has_l_flag = 1
let man_file = s:system(['man', '-w', 'man'])
call s:system(['env', 'MANPAGER=cat', 'man', '-l', man_file])
unlet man_file

This comment has been minimized.

@nhooyr

nhooyr May 18, 2017

Contributor

could you just factor this all out into a function instead? it would be a bit more clean than using unlet. Just name it s:setup or something.

This comment has been minimized.

@raichoo

raichoo May 18, 2017

Contributor

Yes that seems reasonable. The thing is, I'm not a big fan of the s:system solution since I'm not quite sure if another exception can get thrown somewhere in there and I'll might end up with 'man_has_l_flag' as a default. I'd prefer the system solution.

This comment has been minimized.

@nhooyr

nhooyr May 18, 2017

Contributor

Yea, you are right, another exception can be thrown. Could you add a default exception check that will call echom to log the error? Include the prefix man.vim:

function! s:get_page(path) abort
" Respect $MANWIDTH or default to window width.
let manwidth = empty($MANWIDTH) ? winwidth(0) : $MANWIDTH
" Force MANPAGER=cat to ensure Vim is not recursively invoked (by man-db).
" http://comments.gmane.org/gmane.editors.vim.devel/29085
let cmd = ['env', 'MANPAGER=cat', 'MANWIDTH='.manwidth, 'man']
" Use -l everywhere except macOS. #6683
return s:system(cmd + (has('mac') ? [a:path] : ['-l', a:path]))
return s:system(cmd + (!s:man_has_l_flag ? [a:path] : ['-l', a:path]))

This comment has been minimized.

@nhooyr

nhooyr May 18, 2017

Contributor

could you reorder this so that the condition isn't a negative?

@raichoo raichoo force-pushed the raichoo:freebsd branch from 66185dd to b62e3f4 May 18, 2017

@raichoo

This comment has been minimized.

Contributor

raichoo commented May 18, 2017

@nhooyr Applied both changes.

@raichoo raichoo force-pushed the raichoo:freebsd branch from 6024c1f to a60e129 May 18, 2017

let l:man_file = s:get_path('', 'man')
" Force MANPAGER=cat to ensure Vim is not recursively invoked (by man-db).
" http://comments.gmane.org/gmane.editors.vim.devel/29085
call s:system(['env', 'MANPAGER=cat', 'man', '-l', l:man_file])

This comment has been minimized.

@nhooyr

nhooyr May 18, 2017

Contributor

whoops. I just realized we could just reuse s:get_page(l:man_file) here, so you don't have to copy any of the s:system code.

let s:man_has_l_flag = 0
catch
let s:man_has_l_flag = 0
echomsg "man.vim: " . v:exception

This comment has been minimized.

@nhooyr

nhooyr May 18, 2017

Contributor

could you use single quotes instead of double here.

This comment has been minimized.

@raichoo

raichoo May 18, 2017

Contributor

Done…

@raichoo raichoo force-pushed the raichoo:freebsd branch 2 times, most recently from 95a86a0 to f4fcb27 May 18, 2017

@raichoo

This comment has been minimized.

Contributor

raichoo commented May 18, 2017

OK great now it's broken again…

let l:man_file = s:get_path('', 'man')
" Force MANPAGER=cat to ensure Vim is not recursively invoked (by man-db).
" http://comments.gmane.org/gmane.editors.vim.devel/29085
call s:get_page(l:man_file)

This comment has been minimized.

@nhooyr

nhooyr May 18, 2017

Contributor

It's this line breaking things I believe. Grab s:get_page from the bottom and putt above. In addition, the comment is not necessary anymore.

This comment has been minimized.

@raichoo

raichoo May 18, 2017

Contributor

Yep, did that already.

This comment has been minimized.

@nhooyr

nhooyr May 18, 2017

Contributor

Is it working now? If not, what is the error or output?

This comment has been minimized.

@raichoo

raichoo May 18, 2017

Contributor

Everything is working now.

@raichoo raichoo force-pushed the raichoo:freebsd branch 2 times, most recently from 16ed193 to 3cbd25d May 18, 2017

@nhooyr

nhooyr approved these changes May 18, 2017

catch /E145:/
" Ignore the error in restricted mode
catch /command error*/

This comment has been minimized.

@ZyX-I

ZyX-I May 19, 2017

Contributor

This condition is false, you are trying to match command erro, command error, command errorrrrrrr: you get the idea. I guess not what you meant, need to write command error .*.


function! s:enable_l_flag()
let s:man_has_l_flag = 1
let l:man_file = s:get_path('', 'man')

This comment has been minimized.

@ZyX-I

ZyX-I May 19, 2017

Contributor

Do not use l: for local scope in this file, other functions don’t use it. Also what’s the point of this variable at all, you can just move s:get_path call into s:get_page call?

return s:system(cmd + (s:man_has_l_flag ? ['-l', a:path] : [a:path]))
endfunction

function! s:enable_l_flag()

This comment has been minimized.

@ZyX-I

ZyX-I May 19, 2017

Contributor

I would also remove the function and move the code back to inside try (additionall note: there need not be any :unset if you used s:man_file to not pollute global scope, though I tend to unlet temporary variables in such cases as well): it hides away some manipulations with global s:man_has_l_flag while other manipulations are still below and it has incorrect name: you are not enabling -l flag, you are checking whether it can be enabled by enabling it and running s:get_page.

if a:event == 'stdout'
let self.stdout .= join(a:data, "\n")
elseif a:event == 'stderr'
let self.stderr .= join(a:data, "\n")

This comment has been minimized.

@ZyX-I

ZyX-I May 19, 2017

Contributor

These two branches may be merged into

if a:event is# 'stdout' || a:event is# 'stderr'
  let self[a:event] .= join(a:data, "\n")
try
call jobstop(jobid)
throw printf('command timed out: %s', join(a:cmd))
catch /^Vim\%((\a\+)\)\=:E900/

This comment has been minimized.

@ZyX-I

ZyX-I May 19, 2017

Contributor

And here /^Vim(call):E900:/, current variant contains unnecessarly complicated regex and could catch E9001 (non-existing currently) in addition to E900.

@raichoo raichoo force-pushed the raichoo:freebsd branch from 3cbd25d to 13e893b May 19, 2017

@raichoo

This comment has been minimized.

Contributor

raichoo commented May 19, 2017

Done. I've moved the changes for the code that I only moved to another location into a separate commit.

let s:man_has_l_flag = 0
catch
let s:man_has_l_flag = 0
echomsg 'man.vim: ' . v:exception
endtry

This comment has been minimized.

@justinmk

justinmk May 19, 2017

Member

Could avoid most of the code churn by putting this in a s:init() and calling it at end of this script.

This comment has been minimized.

@raichoo

raichoo May 19, 2017

Contributor

Moved the initialization code into s:init to avoid future code churn. I hope this is enough.

@ZyX-I

ZyX-I approved these changes May 19, 2017

@raichoo raichoo force-pushed the raichoo:freebsd branch from b67f421 to 67e4095 May 19, 2017

@raichoo

This comment has been minimized.

Contributor

raichoo commented May 19, 2017

Got rid of the code churn. All changes applied.

catch /command error .*/
let s:man_has_l_flag = 0
catch
let s:man_has_l_flag = 0

This comment has been minimized.

@justinmk

justinmk May 19, 2017

Member

I don't think we need this line.

This comment has been minimized.

@ZyX-I

ZyX-I May 19, 2017

Contributor

Is not zero generally safer option?

This comment has been minimized.

@raichoo

raichoo May 19, 2017

Contributor

I guess that depends your the perspective. Not-zero was the default behavior of this plugin (so I kept it that way) for Mac and some other BSDs zero is the value that you need otherwise :Man won't work properly.

This comment has been minimized.

@justinmk

justinmk May 19, 2017

Member

Actually this whole catch should be removed, let the exception bubble up as before.

This comment has been minimized.

@raichoo

raichoo May 19, 2017

Contributor

That catch is the whole point of the patch. I need a way to check if the call to man -l fails and disable this option so it works on my system.

This comment has been minimized.

@raichoo

raichoo May 19, 2017

Contributor

Or do you mean the last catch all clause?

" the list of searched directories.
try
if !has('win32') && $OSTYPE !~? 'cygwin\|linux' && system('uname -s') =~? 'SunOS' && system('uname -r') =~# '^5'
let s:man_find_arg = '-l'

This comment has been minimized.

@justinmk

justinmk May 19, 2017

Member

so -l means something different in this case? Then s:man_has_l_flag should be zero? (And probably renamed: s:localfile_arg)

This comment has been minimized.

@raichoo

raichoo May 19, 2017

Contributor

Looks like '-l' does something different on these platforms but I have no way to verify that so I kept it that way to avoid breakage.

This comment has been minimized.

@raichoo

raichoo May 19, 2017

Contributor

The thing is that Solaris (at least when I was using it) comes with 2 different sets of userland programs (usually GNU + the Solaris userland) so YMMV.

@raichoo raichoo force-pushed the raichoo:freebsd branch from 67e4095 to ae30279 May 19, 2017

@raichoo

This comment has been minimized.

Contributor

raichoo commented May 19, 2017

Changed name of the variable. I hope this is it, since we are slowly approaching bike-shedding territory ;)

@justinmk

This comment has been minimized.

Member

justinmk commented May 19, 2017

@raichoo raichoo force-pushed the raichoo:freebsd branch from ae30279 to a07ca9d May 19, 2017

@raichoo

This comment has been minimized.

Contributor

raichoo commented May 19, 2017

Removed the catch-all case, just like discussed on IRC.

@justinmk

This comment has been minimized.

Member

justinmk commented May 19, 2017

Found some problems, will fix up and merge.

justinmk added a commit that referenced this pull request May 19, 2017

let s:man_find_arg = '-l'
endif
let s:localfile_arg = 1
call s:get_page(s:get_path('', 'man'))

This comment has been minimized.

@justinmk

justinmk May 19, 2017

Member

get_page result has NL char at the end, which needs to be chopped off (see other usage in this file). Yucky, but oh well. This is the last bug fix man.vim will ever need, right? :)

This comment has been minimized.

@nhooyr

nhooyr May 19, 2017

Contributor

I'm confused, what do you mean?

This comment has been minimized.

@nhooyr

nhooyr May 19, 2017

Contributor

Did you mean get_path?

This comment has been minimized.

@justinmk

justinmk May 19, 2017

Member

@nhooyr see https://github.com/raichoo/neovim/blob/a07ca9df1c5cb664a9244ed45561d511130c1935/runtime/autoload/man.vim#L233

And try removing the [0:-2] in my commit to see the error I was seeing.

@justinmk

This comment has been minimized.

Member

justinmk commented May 19, 2017

merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment