Pulp

Do we want to use http://www.sphinx-doc.org/en/stable/man/sphinx-apidoc.html or explicitly call out to http://www.sphinx-doc.org/en/stable/ext/autodoc.html#module-sphinx.ext.autodoc?

I don't have much experience with these things, but I think the integrated one with the commands in the .rst structure will be the best. I think that would be this[0]. The binary one would have us need to be more involved in organization of the produced content. I don't know much about these things.

Will there need to be a conf.py change? Maybe we need to enable the apidoc module? If so, FYI this conf.py[1] is the one for the hosted site. Each plugin has its own conf.py, which would need to be updated to build the docs locally without errors when apidoc commands are encountered.

[0]: http://www.sphinx-doc.org/en/stable/ext/autodoc.html#module-sphinx.ext.autodoc
[1]: https://github.com/pulp/pulp/blob/81ea84bb73364b39b398d91ada61c87595cd6267/docs/conf.py

Having the autodoc enabled in plugins as-needed sounds fine.

+1 to you maybe sorta accidentally doing this. Should we add this to the sprint?

This went fairly easily, except for one pretty major problem: We can't compile docs strictly (warnings become errors) due to a bug in sphinx[0].

The bug references :ivar:, but the same problem also affects :cvar: (which we use extensively), and I assume also :var:.

I personally like what we're doing with fields and relations called out explicitly with :cvar:, but it looks like if we want to keep it, our options are (in order of best-ness):

1. Invest the time and either fix this issue upstream or otherwise move the issue forward by helping in some way. Build docs with warnings disabled in the meantime.
2. Build docs with warnings disabled.
3. Stop using :*var: to represent object attributes. I don't like this because it makes the docs a lot harder to parse (human-parse, I mean).
4. Don't use autodoc to document our modules, instead documenting our modules separately from their code.
5. Never use the same model field name more than once in the project. (definitely least best, but only barely).

So assuming there are no major disagreements with what I think is the best option, I'll take a little bit of time and poke around in sphinx's guts and see about putting together a PR to stop these crazy warnings and allow us to build with -W again in a future version of sphinx. (sadface)

As a crazy side-effect of this investigation, I made a customized version of napoleon[1] that made it really easy to switch different rendering methods for Django-specific object attributes, and demonstrate how crappy everything that isn't :cvar: looks.

[0]: https://github.com/sphinx-doc/sphinx/issues/2549
[1]: http://www.sphinx-doc.org/en/stable/ext/napoleon.html

@smyers thank you for such a great investigation.

+1 to your top proposal which is to fix the issue upstream and temporarily disable strict Sphinx builds. What needs to be done to fix sphinx? Any ideas there?

Yeah, I have some ideas. For starters, it helps that there's already an open bug to ref.

Here's the sphinx code throwing the warnings that I'm seeing:
https://github.com/sphinx-doc/sphinx/blob/ea86d23845c9179d35420e0f353b47fa83c780f0/sphinx/domains/python.py#L738-L759

I think there's enough info local to that function (or its caller) to know whether or not to xref based on the current directive. I think it's entirely inappropriate for this xref function to be called in the case of :var:, :ivar:, and :cvar: since their role is explicitly local to their container, so either handling that case in the linked function or finding a place to bail out before it is called is a likely solution.

I think the upstream issue is here:
https://github.com/sphinx-doc/sphinx/blob/ea86d23845c9179d35420e0f353b47fa83c780f0/sphinx/domains/python.py#L154-L157 (current master ref at time of this post)

The role for :var:, :ivar:, and :cvar: is "obj", which is a cross-referenceable type. Changing this bit to set the role to 'attr' make sphinx correctly handle them as the non-cross-referenced attribute type, and makes all the ugly warnings go away.

So we'll run without -W for now, and I'll get this fix in upstream ASAP.

semyers wrote:

The role for :var:, :ivar:, and :cvar: is "obj", which is a cross-referenceable type. Changing this bit to set the role to 'attr' make sphinx correctly handle them as the non-cross-referenced attribute type, and makes all the ugly warnings go away.

Argh, all the ugly warnings have not gone away, I was just running sphinx in such a way that they weren't being printed. The 'attr' trick didn't actually change anything. :(

The more I learn about this, the more I think that sphinx just shouldn't be trying to link these at all, so I'm getting to know sphinx a little bit better in hopes of figuring out how to make it stop trying to cross-ref them "the sphinx way".

This appears to be a working fix, which I'll be submitting upstream soon:

diff --git a/sphinx/domains/python.py b/sphinx/domains/python.py
index 1639d82..c04c0b4 100644
--- a/sphinx/domains/python.py
+++ b/sphinx/domains/python.py
@@ -89,8 +89,8 @@ class PyXrefMixin(object):
                   contnode=None):
         result = super(PyXrefMixin, self).make_xref(rolename, domain, target,
                                                     innernode, contnode)
-        result['refspecific'] = True
         if target.startswith(('.', '~')):
+            result['refspecific'] = True
             prefix, result['reftarget'] = target[0], target[1:]
             if prefix == '.':
                 text = target[1:]
@@ -126,7 +126,7 @@ class PyObject(ObjectDescription):
                             'keyword', 'kwarg', 'kwparam'),
                      typerolename='obj', typenames=('paramtype', 'type'),
                      can_collapse=True),
-        PyTypedField('variable', label=l_('Variables'), rolename='obj',
+        PyTypedField('variable', label=l_('Variables'), rolename='attr',
                      names=('var', 'ivar', 'cvar'),
                      typerolename='obj', typenames=('vartype',),
                      can_collapse=True),

I'd like to better understand exactly what that 'reftarget' 'refspecific' value is intended to do, but if my current understanding is correct, then this change turns attributes into link (reference) targets only when prefixed with '.' or '~'. This might be a breaking change, but it's worth pointing out that when using :vartype: with :*var:, sphinx does still attempt to reference the var type, leaving us in what I think is the desired state: Class/Instance/Module attrs aren't linked by default but can be turned into links explicitly, and links will be generated to their typedef (if possible) when that type is specified.

Edit: 'refspecific' :)

Despite being MODIFIED, this is not quite done. Here's the problem:

In order to build the apidocs, sphinx needs to be able to import the code. This means, effectively, that the docs builder and everything it depends on need to run in a python3 virtualenv, since we need to install pulp 3 packages somewhere to make them importable by the sphinx builder.

This includes converting some of our building library code to be py3 compatible, which should be fairly easy to do in a cross-compatible way.

@smyers, thank you so much for this. Having 3.0 nightly docs is a big step forward. smyers++