add link to manpages in HTML builder #4235

anarcat · 2017-11-11T22:55:55Z

Feature
Create actual HTML links (<a href>) when the :manpage: role is
used in RST markup.
Requires a new configuration setting, but is backwards-compatible
with older configuration files.

It seems way more useful to have the HTML documentation builder
actually link to real rendered versions of HTML manpages in its
output. That way people can click on manpages to get the full
documentation. There are a few services offering this online, so we do
not explicitly enable one by default, but the Debian manpages
repository has a lot of them.

No unit tests because I couldn't quite figure out where to fit that
in.

tk0miya

looks good to me for your concept.
Could you add testcase for this please?

tk0miya · 2017-11-12T04:02:01Z

doc/config.rst

@@ -295,6 +295,14 @@ General configuration

   .. versionadded:: 1.3

+.. confval:: manpages_url_pattern


I feel manpages_url is simpler. What do you think?

I don't care - but i felt that "pattern" implied it was a bit more than just a prefix...

If you don't opposite, could you rename this please?

sure. done.

tk0miya · 2017-11-12T04:09:39Z

doc/config.rst

+.. confval:: manpages_url_pattern
+
+   A URL to cross-reference manpages directives. If this is defined to
+   ``https://manpages.debian.org/%s``, the ``:manpage:ls(1)`` role


For example, manpages.org uses only the name part (without section) like http://manpages.org/ls .
So I propose you to use named format arguments like {full}, {name} and {section}.

In addition, it would be very useful to add a hyperlink to here onto an introduction of :manpage: role.

Yeah, I thought about doing a tuple - but how do you parse the manpage? Sometimes people don't put a section, sometimes it's page.section or page(section), it's not really well normalized.

But if you think that's important, i'll try to work on a regex and switch to format-style patterns.

okay, so I got this now:

def visit_manpage(self, node): # type: (nodes.Node) -> None self.visit_literal_emphasis(node) if self.manpages_url_pattern: manpage = ' '.join([str(x) for x in node.children if isinstance(x, nodes.Text)]) pattern = r'^(?P<full>(?P<name>.+)[$\.](?P<section>[1-9]\w*)?$?)$' r = re.match(pattern, manpage) if r: info = r.groupdict() else: info = {'full': manpage} node['refuri'] = self.manpages_url_pattern.format(**info) self.visit_reference(node)

but my concern is that I have to copy this over the html5.py builder again, and then this will be duplicated all over the place. what would be a better place to keep that functionality?

Good work. I like it :-)

what would be a better place to keep that functionality?

I think this is a kind of transform module.

# in sphinx/transform/__init__.py class ManpageTransform(SphinxTransform): def apply(self): for node in self.document.traverse(addnodes.manpage): manpage = ' '.join([str(x) for x in node.children if isinstance(x, nodes.Text)]) pattern = r'^(?P<full>(?P<name>.+)[$\.](?P<section>[1-9]\w*)?$?)$' r = re.match(pattern, manpage) if r: info = r.groupdict() else: info = {'full': manpage} for key, value in info.items(): node[key] = value # => Add this transform class to the transforms list in sphinx/io.py

This allows you to access name, section and full as attributes of nodes.
And it also make writers simple.

i see, done.

tk0miya · 2017-11-12T04:25:13Z

sphinx/config.py

@@ -123,6 +123,7 @@ class Config(object):
        primary_domain = ('py', 'env', [NoneType]),
        needs_sphinx = (None, None, string_classes),
        needs_extensions = ({}, None),
+        manpages_url_pattern = (None, 'html'),


I think this should be effect not to only html, but also all builders (in future). For example, it would be nice if LaTeX builder also support the hyperlinks. ^So last argument should be 'env'. It means the update of settings effects to all builders.

ah. okay. done.

tk0miya · 2017-11-12T04:32:12Z

sphinx/templates/quickstart/conf.py_t

@@ -117,6 +117,9 @@ html_static_path = ['{{ dot }}static']
 #
 # html_sidebars = {}

+# Cross-reference :manpage: roles to actual sites.
+#
+# manpage_url_pattern = 'https://manpages.debian.org/%s


To make default conf.py simple, please do not add new comment-block her.

sorry, what do you mean? should i add it elsewhere? or not at all? i added this because it says that should be done on top of the Config class.

Oh, sorry. Please forget the comment of Config class. I will change it later.
Current conf.py contains a lot of comments. I think it confuses users. So we have a plan to reduce these comments from conf.py.

Could you remove this please?

done. it wasn't clear from your comment you actually wanted it removed (or if you would remove it yourself at a later point).

tk0miya · 2017-11-12T05:00:10Z

sphinx/writers/html.py

@@ -79,6 +79,7 @@ def __init__(self, builder, *args, **kwds):
        self.highlightopts = builder.config.highlight_options
        self.highlightlinenothreshold = sys.maxsize
        self.docnames = [builder.current_docname]  # for singlehtml builder
+        self.manpages_url_pattern = getattr(builder.config, 'manpages_url_pattern')


After adding an entry to sphinx.config, you can access it always. So you don't need to use getattr here.
BTW, why are you storing the value of manpage_url_pattern as an instance variable?

because that's the pattern used for other configuration, like highlightopts.

and the reason why i'm using getattr is so that older configuration files keep working, but maybe that's not necessary?

Yes, it's not necessary. It is always enabled even if with old config-files, because you'd added it to sphinx.config.

because that's the pattern used for other configuration, like highlightopts.

I see, either is okay to me.

okay, fixed.

tk0miya · 2017-11-12T05:01:59Z

sphinx/writers/html.py

+            manpage = ' '.join([str(x) for x in node.children
+                                if isinstance(x, nodes.Text)])
+            node['refuri'] = self.manpages_url_pattern % manpage
+            self.visit_reference(node)


Could you tell me the result if manpage_url_pattern enabled?
It seems this visitor method call both visit_literal_emphasis and visit_reference. So I can't guess it.

This:

:manpage:`hdparm(8)`

... gets turned in:

<em class="manpage"><a class="manpage reference external" href="https://manpages.debian.org/hdparm(8)">hdparm(8)</a></em>

i figured i would keep the existing emphasis style.

anarcat · 2017-11-12T13:54:40Z

Could you add testcase for this please?

See, that's where I'm stuck. Where should i add it?

anarcat · 2017-11-12T14:49:06Z

i re-rolled a new patch with improved pattern support and documentation. i would need help to figure out where to put those eventual tests and how to avoid code duplication.

tk0miya · 2017-11-13T16:47:16Z

doc/config.rst

+
+   A URL to cross-reference :rst:role:`manpage` directives. If this is
+   defined to ``https://manpages.debian.org/{full}``, the
+   ``:manpage:man(1)`` role will like to


Please use

:literal:`:manpage:`man(1)``

instead. It allows to represent role notations correctly.

that's quite a mouthful! but okay, done. :)

tk0miya · 2017-11-13T16:53:01Z

sphinx/templates/quickstart/conf.py_t

@@ -117,6 +117,9 @@ html_static_path = ['{{ dot }}static']
 #
 # html_sidebars = {}

+# Cross-reference :manpage: roles to actual sites.
+#
+# manpage_url_pattern = 'https://manpages.debian.org/%s


Oh, sorry. Please forget the comment of Config class. I will change it later.
Current conf.py contains a lot of comments. I think it confuses users. So we have a plan to reduce these comments from conf.py.

tk0miya · 2017-11-13T16:58:30Z

sphinx/writers/html.py

@@ -79,6 +79,7 @@ def __init__(self, builder, *args, **kwds):
        self.highlightopts = builder.config.highlight_options
        self.highlightlinenothreshold = sys.maxsize
        self.docnames = [builder.current_docname]  # for singlehtml builder
+        self.manpages_url_pattern = getattr(builder.config, 'manpages_url_pattern')


Yes, it's not necessary. It is always enabled even if with old config-files, because you'd added it to sphinx.config.

because that's the pattern used for other configuration, like highlightopts.

I see, either is okay to me.

tk0miya · 2017-11-13T17:03:52Z

See, that's where I'm stuck. Where should i add it?

Please add testcase to tests/test_build_html.py.
test_numfig_with_numbered_toctree is a good example for you. It modifies configuration using confoverrides parameter of @pytest.mark.sphinx, and verifies the generated HTML with @pytest.mark.parametrize decorator.

anarcat · 2017-11-13T19:38:05Z

alright, i pushed a new patch with the recommended changes. next step is for me to figure out unit tests.

anarcat · 2017-11-13T20:26:23Z

i think i got the test suite right. i had some weird failures elsewhere in the test suite, but i suspect this may be my local environment that was broken... we'll see what travis says.

i'm not sure what the second argument in the xpath checks is. i left it empty, but i have tried man(1) and :manpage:... to no avail. any ideas?

anarcat · 2017-11-13T21:39:31Z

whoa, and the test suite actually passes now. woot! :)

tk0miya

LGTM with nits

tk0miya · 2017-11-15T16:07:37Z

doc/config.rst

@@ -295,6 +295,14 @@ General configuration

   .. versionadded:: 1.3

+.. confval:: manpages_url_pattern


If you don't opposite, could you rename this please?

tk0miya · 2017-11-15T16:09:48Z

doc/config.rst

+
+     * ``manpage_name`` - the manpage name (``man``)
+     * ``manpage_section`` - the manpage section (``1``)
+     * ``manpage_full`` - the whole string (``man(1)``)


I feel manpage_ suffix is a bit verbose. It is obvious that these variables are related with manpage, because this settings is used for URL of manpages.

sure, but internally, the node parameter would be ambiguous otherwise. since i just pass the whole node attributes to the formatter, i figured it was better to have unambiguous parameters.

now, i could expand that formatter to have an explicit dictionary with only those values, but that didn't seem very useful. i will do that, however, if you insist on changing the formatter...

I feel manpage_ prefix is also too verbose for node attributes. manpage_node['name'] and manpage_node['section'] are much simpler for me.
What do you think?

er. well i explained this earlier: i thought having a name parameter in the node would be ambiguous. but i don't know how the parsers work - if you're saying it's unambiguous i'm happy to fix this!

I don't think so. Usually, we know the type of nodes when we manipulate them. For example, ManpageLink transform searches manpage node using node.traverse(). And each visit_* methods of writer also know the type of node.
So I feel manpage_name of manpage node is verbose. We already know the node represents a reference for manpage.
I understand any_node['name'] is ambiguous. But we would not access in such way.

In addition, I feel the format params of manpages_url is verbose.

understood, i'll fix this then! :)

tk0miya · 2017-11-15T16:11:09Z

sphinx/templates/quickstart/conf.py_t

@@ -117,6 +117,9 @@ html_static_path = ['{{ dot }}static']
 #
 # html_sidebars = {}

+# Cross-reference :manpage: roles to actual sites.
+#
+# manpage_url_pattern = 'https://manpages.debian.org/%s


Could you remove this please?

tk0miya · 2017-11-15T16:53:26Z

doc/config.rst

+
+   This also supports manpages specified as ``man.1``.
+
+   .. versionadded:: 1.7


Please mention to this confval at an introduction of :manpage: role.
http://www.sphinx-doc.org/en/stable/markup/inline.html#role-manpage

This implementation only effect to HTML output. So we should mention users the limitation (or rename this to html_manpages_url).

added a note.

anarcat · 2017-11-15T18:19:27Z

i believe i fixed all your remaining concerns, except the manpage_ prefix in the node attributes and formatter. let me know if you still think that's important to change.

shimizukawa · 2018-01-07T14:01:06Z

FYI. The code freeze day of 1.7 will come in a week.

Mid of Jan: Feature freeze, Call for translation and release 1.7b1
#4211 (comment)

anarcat · 2018-01-07T16:08:50Z

i remerged with master here to resolve conflicts, and I believe that I addressed all the concerns that were formulated here by @tk0miya - so i'm not sure what the next step is here.

tk0miya · 2018-01-08T00:38:45Z

@anarcat Please check the result of Travis CI and Circle CI. It seems many tests are failed. I will review it after resolved.

tk0miya · 2018-01-08T00:43:00Z

sphinx/config.py

@@ -125,6 +125,7 @@ class Config(object):
        primary_domain = ('py', 'env', [NoneType]),
        needs_sphinx = (None, None, string_classes),
        needs_extensions = ({}, None),
+        manpages_url_pattern = (None, 'env'),


I guess the error comes from here. This is not renamed.

anarcat · 2018-01-10T15:37:20Z

@anarcat Please check the result of Travis CI and Circle CI. It seems many tests are failed. I will review it after resolved.

i pushed another fix and conflict resolution, let's see what CI thinks.

i'd also like to hear from you about the ambiguity question...

anarcat · 2018-01-10T15:49:42Z

@tk0miya CI passes, this is ready for a final review. i think the only question is the manage_ prefix in the node object, if it is redundant or if just having e.g. name there would be ambiguous.

thanks!

It is useful to have the HTML documentation builder actually link to real rendered versions of HTML manpages in its output. That way people can click on manpages to get the full documentation. There are a few services offering this online, so we do not explicitly enable one by default, but the Debian manpages repository has a lot of the manpages pre-rendered, so it is used as an example in the documentation. The parsing work is done by a transformer class that parses manpage objects and extract name/section elements. Those then can be used by writers to cross-reference to actual sites. An implementation is done in the two HTML writers, but could also apply to ePUB/PDF writers as well in the future. This is not enabled by default: the `manpages_url` configuration item needs to be enabled to point to the chosen site. The `page`, `section` and `path` parameters are expanded through Python string formatting in the URL on output. Unit tests are fairly limited, but should cover most common use-cases.

anarcat · 2018-01-11T18:40:33Z

okay, so I did the following renames:

manpage_name is now page, to fit with the naming convention in man(1)
manpage_section is now section
manpage_full is now path because full seemed strange without context

CI seems to pass, so I think this would be ready to merge if you agree with the new naming.

Thanks for the prompt review!

tk0miya

LGTM. Thank you for update :-)

tk0miya · 2018-01-13T05:28:21Z

Merged. Thank you for your contribution!

anarcat · 2018-01-13T22:38:43Z

yay, thanks!

See sphinx-doc/sphinx#4235 for the :manpage: role.

anarcat force-pushed the manpage-links branch from b1e689f to 6cbc60e Compare November 11, 2017 23:03

tk0miya requested changes Nov 12, 2017

View reviewed changes

tk0miya added internals:config markup type:proposal a feature suggestion labels Nov 12, 2017

tk0miya added this to the 1.7 milestone Nov 12, 2017

anarcat force-pushed the manpage-links branch from 6cbc60e to 4a26bb2 Compare November 12, 2017 14:48

tk0miya reviewed Nov 13, 2017

View reviewed changes

anarcat force-pushed the manpage-links branch from 4a26bb2 to 8e31c2e Compare November 13, 2017 19:37

anarcat force-pushed the manpage-links branch 2 times, most recently from 48b83c9 to d7f8552 Compare November 13, 2017 20:25

tk0miya reviewed Nov 15, 2017

View reviewed changes

anarcat force-pushed the manpage-links branch from d7f8552 to a3cb3bd Compare November 15, 2017 18:18

tk0miya reviewed Jan 8, 2018

View reviewed changes

anarcat force-pushed the manpage-links branch from 086f09d to c9773b1 Compare January 10, 2018 15:36

anarcat force-pushed the manpage-links branch from c9773b1 to 90f7c7e Compare January 11, 2018 18:23

tk0miya approved these changes Jan 12, 2018

View reviewed changes

tk0miya merged commit 7e365d9 into sphinx-doc:master Jan 13, 2018

tk0miya added a commit that referenced this pull request Jan 13, 2018

Update CHANGES for PR #4235

2324dee

anarcat deleted the manpage-links branch January 13, 2018 22:38

polyzen mentioned this pull request May 13, 2018

doc: Fix minor typo "like" -> "link" #4964

Merged

keszybz added a commit to systemd/python-systemd that referenced this pull request Jun 16, 2019

docs: use :manpage: role to provide links to systemd man pages

42ec430

See sphinx-doc/sphinx#4235 for the :manpage: role.

github-actions bot locked as resolved and limited conversation to collaborators Sep 7, 2021

		@@ -295,6 +295,14 @@ General configuration

		.. versionadded:: 1.3

		.. confval:: manpages_url_pattern


		This also supports manpages specified as ``man.1``.

		.. versionadded:: 1.7

add link to manpages in HTML builder #4235

add link to manpages in HTML builder #4235

Conversation

anarcat commented Nov 11, 2017

tk0miya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anarcat Nov 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anarcat commented Nov 12, 2017

anarcat commented Nov 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tk0miya commented Nov 13, 2017

anarcat commented Nov 13, 2017

anarcat commented Nov 13, 2017

anarcat commented Nov 13, 2017

tk0miya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anarcat commented Nov 15, 2017

shimizukawa commented Jan 7, 2018

anarcat commented Jan 7, 2018

tk0miya commented Jan 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anarcat commented Jan 10, 2018

anarcat commented Jan 10, 2018

anarcat commented Jan 11, 2018

tk0miya left a comment

Choose a reason for hiding this comment

tk0miya commented Jan 13, 2018

anarcat commented Jan 13, 2018

anarcat Nov 12, 2017 •

edited

Loading