search

neovim / neovim

Vim-fork focused on extensibility and usability
https://neovim.io
Other
83.25k stars 5.7k forks source link

:Man doesn't work with mandoc #11794

Closed mcepl closed 4 years ago

mcepl commented 4 years ago

Steps to reproduce using nvim -u NORC

  1. Start LANG=en_GB nvim -u NORC

  2. run :Man bash

Actual behaviour

Error message:

Error detected while processing function man#open_page:
line   39:
E987: invalid return value from tagfunc
E426: tag not found: bash(1)
Press ENTER or type command to continue

Expected behaviour

Displayed man page of bash(1)

justinmk commented 4 years ago

can't reproduce on ubuntu, but I don't have mandoc installed. cc @bobrippling

bobrippling commented 4 years ago

I'm afraid I'm also unable to reproduce. Is there anything about your environment that's mandoc-specific? And are you able to reproduce this bug on commit 53b025887e28888f8dba78ff57afc001d1a6428b or prior, by any chance? I'm wondering if the change is the 'tagfunc' work or whether it's the code that parses man output and wrangles paths.

justinmk commented 4 years ago

Guessing this is related to https://github.com/neovim/neovim/issues/9631#issuecomment-465660416

Though in this report :Man is being used instead of invoking as MANPAGER.

mandoc is buggy. Not going to spend time on this. Send a PR if you find a fix.

mcepl commented 4 years ago

@justinmk @bobrippling Would it help anything if I told that I cannot reproduce this bug when I reverse commit c6afad7 on autoload/man.vim? autoload/man.vim is the edited version I have.

neovim-mandoc-log.txt is the output of -V20 when running just :Man SSL_read with the original autoload/man.vim from 14a8b3b98. What seem interesting to me that the last function calling any external command (s:verify_exists) seems to exit correctly and it seems to return exactly the same list as when running with man-db man (i.e., ['3ssl', 'SSL_peek', '/usr/share/man/man3/SSL_peek.3ssl.gz']).

ischwarze commented 4 years ago

Mandoc maintainer speaking here. To help investigation, i have (locally, on my machine, of course not globally for all OpenBSD users) updated the OpenBSD neovim port to v0.5.0-350-g14a8b3b98 and run it under ktrace(1) -i, then typed ":Man bash". This stands out in the ktrace shortly before nvim gives up:

91859 nvim     CALL  execve(0x1b92dfb83580,0x1b9273f356c0,0x1b928d47b400)
91859 nvim     NAMI  "/usr/bin/man"
91859 nvim     ARGS  
    [0] = "/usr/bin/man"
    [1] = "-w"
91859 man      NAMI  "/usr/libexec/ld.so"

So it appears something calls "man -w" without any further argument, which agrees with this part from mcepl's neovim-mandoc-log.txt:

man.vim: command error (6)
man -w: usage: man [-acfhklw] [-C file] [-M path] [-m path] [-S subsection]
   [[-s] section] name ...

Indeed the "name" on the man(1) command line is not optional, so it seems correct to me that "man -w" fails. The question is why that command gets executed...

mcepl commented 4 years ago

Well, considering which implementation of man(1) is the correct one is above my pay-grade, but it is true, that man-db man has an undocumented behaviour when running man -w:

~@milic$ man -w
/usr/share/man:/home/matej/.local/share/man
~@milic$

It apparently prints list of directories where manpages are located. I don’t know if there is more official standard for man(1) behaviour, so I cannot say whether neovim has right to rely on such.

bobrippling commented 4 years ago

Interesting. I can confirm this man -w behaviour and I think man.vim shouldn't be relying on it.

This does mean we'd have to hardcode the man-path as something like /usr/share/man:/usr/local/man:/usr/local/share/man:~/.local/share/man, since we can't get it from man any more.

We'd then have to remove s:find_arg and replace it with code that would a) assume the man-path and b) manually search for the man pages themselves under said man path. We'd also need to handle man on SunOS 5, going off the code here.

Unfortunately I'm short on time at the moment but may be able to resume this later down the line.

justinmk commented 4 years ago

This does mean we'd have to hardcode the man-path as something like /usr/share/man:/usr/local/man:/usr/local/share/man:~/.local/share/man, since we can't get it from man any more.

@ischwarze know of an alternative way to get this info ?

mcepl commented 4 years ago

We'd then have to remove s:find_arg and replace it with code that would a) assume the man-path and b) manually search for the man pages themselves under said man path. We'd also need to handle man on SunOS 5, going off the code here.

Or at least when the call to man -w fails, then we should default to something (and yes, we should check the content of the environment variable $MANPATH as well, I guess).

ischwarze commented 4 years ago

@bobrippling @justinmk Why do you need the manpath at all, what do you want to do with it? (Sorry for not finding the file man.vim particularly readable.) When application programs manually inspect the manpath, that smells like a design error to me.

Manually searching for manual pages in the manpath is certainly a terrible idea. At least the following aspects differ among operating systems:

  1. Naming of sections and section directies. These differ in multiple respects.
  2. Presence, naming, and organisation of architecture-specific manual pages.
  3. Presence, naming, and organisation of manual pages for other versions of the operating system or even other operating systems.
  4. Presence, naming, organisation, and encoding of manual pages in languages other than English.
  5. Existence of splits inside manual page trees separating manual pages for different libraries, subsystems, and software vendors. No, this is not the necessarily exactly the same as manual sections in item 1.
  6. Naming of individual manual page files.
  7. Whether manual pages documenting more than one program or function are installed once and the information what they document is stored in a database in a system-specific format and location, or whether all the names exist as file names in the file system. In the latter case, there are at least four different systems: copies with cp(1), hard links with ln(1), symbolic links with ln -s, or roff .so inclusion requests.
  8. Whether preformatted files exist, whether they are automatically generated or static, whether application programs are supposed to automatically generate them on demand, how to generate them, where they are located and how they are named, whether they are supposed to be preferred or merely a fallback, and whether a preformatted version exists for each source version and vice versa. Many systems have some pages installed source only and other pages installed preformatted only.
  9. Whether (all or some) pages are compressed and in which way.
  10. Even the expected format of the contents of the $MANPATH environment variable might vary slightly from system to system. For example, on OpenBSD, you can include the language but not the section, while on Oracle Solaris, you can include sections, but not languages.

You are supposed to use the man(1) program to locate and format manual pages on any given operating system. That program will know all the above specifics for its own platform. You can capture the output from commands like "man nvim" and display that inside the editor, but i doubt the wisdom of trying to second-guess the man(1) program itself.

Sure, the user interface of man(1) differs considerably among operating systems (for example, commercial Solaris man(1) does not have any -w option at all, and -l means something totally different there compared to man-db and mandoc; or with mandoc, -w implies -a while with man-db, it does not, and so on) - but the man(1) user interface still doesn't vary nearly as much as the internal organization of manual page directories.

It hardly looks reasonable (nor even feasible) to me to try and re-implement all that inside a mere text editor, for each and every operating system on the planet... :-( Getting all of that right isn't simple even for one single system, and for most systems, the rules keep evolving over time.

All that said, i'm not necessarily totally opposed to making "man -w" without arguments in mandoc compatible with the behaviour of man-db, it might be feasible and acceptable, but i'd still prefer to also understand what your goal is when using it.

bobrippling commented 4 years ago

You are supposed to use the man(1) program to locate and format manual pages on any given operating system. That program will know all the above specifics for its own platform.

Agreed - I suppose the question is why neovim displays man pages like this, which is primarily to do with disabling shelling out to a subcommand (I imagine).

I think we have two options here, since the existing man.vim logic seems too tied to a specific man implementation.

I'm afraid I only added tagfunc support to man.vim, I'm not really clued up on how the rest of it works. @nhooyr - are you the current maintainer?

nhooyr commented 4 years ago

I am the current maintainer, I'll look at this over the weekend.

nhooyr commented 4 years ago

The reason we need the list of paths is for completion.

See https://github.com/neovim/neovim/blob/31614d3eb0b6e563eab9a76acf6e33c725b92e60/runtime/autoload/man.vim#L331-L339

The problem is tagfunc calling this function, it should only be completion.

I didn't review any of those PRs so I don't know how/why but it looks like it's getting called in #open_page which is causing the error to be thrown. That's why @mcepl cannot reproduce before https://github.com/neovim/neovim/commit/c6afad78d39aa77a4d372759336018ef6e101dab

Can we remove the call to s:get_paths in https://github.com/neovim/neovim/blob/31614d3eb0b6e563eab9a76acf6e33c725b92e60/runtime/autoload/man.vim#L387-L413 @bobrippling?

mcepl commented 4 years ago

Can we remove the call to s:get_paths in

Is this the reason why I cannot even follow man hyperlink in the displayed manpage (e.g., SEE ALSO section in most manpages)?

man.vim: command error (9) man -w: usage: man [-acfhklw] [-C file] [-M path] [-m path] [-S su
bsection]
           [[-s] section] name ...
Chyba při zpracování function man#goto_tag:
řádek    6:
E714: List required
Press ENTER or type command to continue

I don’t understand why Man.vim needs for following vi(1) anything else than call man -c 1 vi.

ischwarze commented 4 years ago

Hi,

so i'm now supporting bare "man -w" in the HEAD version of mandoc, and it will be in the next mandoc release:

https://cvsweb.bsd.lv/mandoc/main.c#rev1.343
https://cvsweb.bsd.lv/mandoc/man.1.diff?r1=1.36&r2=1.37

Even though it is then supported by man-1.6, man-db, and mandoc (i.e. essentially Linux and OpenBSD), many operating systems still don't support it, in particular not FreeBSD (using its own man implementation written in sh(1)), NetBSD (still using traditional BSD CSRG man), DragonFly BSD (using FreeBSD man), Minix (using NetBSD man), Illumos (using modified CSRG/NetBSD man), Oracle Solaris, and i have serious doubts about commercial systems like AIX, HP-UX, and MacOS.

If you want to use it for autocompletion, you should probably make sure that you have some fallback or just disable autocompletion when it doesn't work, rather than becoming unable to view manual pages at all.

Regarding tagging, the mandoc implementation of man(1) already has tagging support built in and uses markup in the mdoc(7) and man(7) source code to set reasonable tags. That's almost certainly better tagging than whatever you might be able to guess from the formatted manual page. Then again, man-1.6, man-db, FreeBSD man, NetBSD man and so on can't do this because they format the page first (typically with groff, or some with mandoc), and then, when they come to the point where they might decide about tags, the source information has already been lost.

By the way, the mandoc implementation also supports automatic generation of a table of contents (-O toc), but only in -T html output mode. It isn't enabled by default because some OpenBSD developers dislike it. We do not support it for terminal output because generating hyperlinks wouldn't work there, so it wouldn't really be useful. But with mandoc installed and enabled as man(1), try something like:

MANPAGER="lynx -force_html" man -T html -O toc pf.conf

then move the cursor to one of the lines in the table of contents and hit the "ENTER" key. Not sure how you could leverage that in neovim, though. Using man(1) directly will inevitably be much more powerful than calling it from inside nvim(1), i guess.

bobrippling commented 4 years ago

The problem is tagfunc calling this function, it should only be completion. [...] it looks like it's getting called in #open_page which is causing the error to be thrown. That's why @mcepl cannot reproduce before c6afad7

Thanks for looking into this @nhooyr - I think the best approach in this case is for the tag completion just to work with what it's given, instead of trying to guess at further man page paths then. Essentially the solution we use for when 'cscopetag' is enabled, with a little less magic. I think as part of this fix, I'll also add error handling so we don't entirely bail out if man -w gives us back an error.

Is this the reason why I cannot even follow man hyperlink in the displayed manpage

Most likely, yes.

so i'm now supporting bare "man -w" in the HEAD version of mandoc

Thanks for your work and the potential autocompletion support this could provide. The tagging support could also be useful, however for now I'm going to revert man.vim back to a more basic state, and then we'll see what feedback we get on how useful it would be to have such extra functionality.

nhooyr commented 4 years ago

Ok sounds good, feel free to tag me for review in the PR.

justinmk commented 4 years ago

I think the best approach in this case is for the tag completion just to work with what it's given,

what's wrong with a "best effort" attempt? Certainly we shouldn't fail if man -w doesn't give us what we want, but if we can also discover more options for autocomplete, that's strictly a good thing.

ischwarze commented 4 years ago

Just to give you an idea how different things are: On systems using the mandoc implementation of man(1), you would only find a fraction of the possible manual page names that could be used for autocompletion by looking through the file system. The full set of names and the name-to-file mapping is only contained in https://man.openbsd.org/mandoc.db files. On such a system, you could find the full list of names starting with "nv" with this command:

 $ man -k Nm~^nv | sed 's/ -.*//'
nvim(1)
vlc, cvlc, nvlc, qvlc, rvlc, svlc(1)
gre, egre, mgre, nvgre(4)
nv(4)
nviic(4)
nvme(4)
nvram(4/amd64)
nvram(4/i386)
nvt(4)

While that is of course much more efficient than iterating the file system, it won't work at all with the man-db or man-1.6 implementations of man(1).

I don't think supporting that should be anywhere near the top of your list of priorities, but it might be instructive to see that not all man(1) programs have the same features.

justinmk commented 4 years ago

@ischwarze thanks, that's helpful. Certainly we can implement more strategies to discover manpages. The feature is inherently a "best effort" attempt.

mcepl commented 4 years ago

so i'm now supporting bare "man -w" in the HEAD version of mandoc, and it will be in the next mandoc release:

https://cvsweb.bsd.lv/mandoc/main.c#rev1.343
https://cvsweb.bsd.lv/mandoc/man.1.diff?r1=1.36&r2=1.37

@ischwarze When do you plan to do next release of mandoc? It is really difficult dissect just this one patch (git-deps shows me approx 60 commits dependent on this one).

bobrippling commented 4 years ago

what's wrong with a "best effort" attempt?

I was mainly concerned about introducing more bugs, with all the different man implementations out there. I suppose we can fallback to the more basic approach on errors though

justinmk commented 4 years ago

I was mainly concerned about introducing more bugs, with all the different man implementations out there. I suppose we can fallback to the more basic approach on errors though

sure, I just mean we should not remove the current behavior. If the -w hack works then let's continue to use it, if it actually improves the completion results.

bobrippling commented 4 years ago

let's continue to use it, if it actually improves the completion results

Yeah, sounds like a good approach. I believe I have a fix in #11918, I've been testing by manually throwing an exception in place of s:system(["man", ...]), but would appreciate if you'd mind trying out my fix @mcepl, and @nhooyr feel free to let me know what you think of the changes.

And thanks everyone for the above discussion, I feel it was productive in determining the correct fix to make.

:smiley:

mcepl commented 4 years ago

Yeah, sounds like a good approach. I believe I have a fix in #11918, I've been testing by manually throwing an exception in place of s:system(["man", ...]), but would appreciate if you'd mind trying out my fix @mcepl, and @nhooyr feel free to let me know what you think of the changes.

Seems to work for me, even with man before the change mentioned above I have now working :Man command again.