diff mbox

[ovs-dev] Documentation: Report errors for use of features not in Sphinx 1.1.3.

Message ID 20170307185434.31774-1-blp@ovn.org
State Superseded
Headers show

Commit Message

Ben Pfaff March 7, 2017, 6:54 p.m. UTC 
Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 Documentation/automake.mk              | 15 ++++++++++++++-
 Documentation/sphinx-version-blacklist |  2 ++
 2 files changed, 16 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/sphinx-version-blacklist

Comments

Stephen Finucane March 8, 2017, 9:15 a.m. UTC | #1 
On Tue, 2017-03-07 at 10:54 -0800, Ben Pfaff wrote:
> Signed-off-by: Ben Pfaff <blp@ovn.org>

I'm not sure if this is completely necessary, but I imagine it won't
slow the build down too much and looks about right so:

Acked-by: Stephen Finucane <stephen@that.guru>

I'd also like to see a TODO describing the path to removing support for
Sphinx 1.1 (which is pretty old now) but what that path looks like, I
don't know.

Some other small points below.

> ---
>  Documentation/automake.mk              | 15 ++++++++++++++-
>  Documentation/sphinx-version-blacklist |  2 ++
>  2 files changed, 16 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/sphinx-version-blacklist
> 
> diff --git a/Documentation/automake.mk b/Documentation/automake.mk
> index a74807fde532..f7f1fe61d1b7 100644
> --- a/Documentation/automake.mk
> +++ b/Documentation/automake.mk
> @@ -86,7 +86,8 @@ EXTRA_DIST += \
>  	Documentation/internals/contributing/documentation-style.rst 
> \
>  	Documentation/internals/contributing/libopenvswitch-abi.rst
> \
>  	Documentation/internals/contributing/submitting-patches.rst
> \
> -	Documentation/requirements.txt
> +	Documentation/requirements.txt \
> +	Documentation/sphinx-version-blacklist

This still won't build locally for me so I can't validate it. Could you
just make sure this doesn't cause a failed doc build due to a warning
about documents that are not included in any table of contents. I think
only 'rst' files are included in that check, but I can't be sure.

>  
>  # You can set these variables from the command line.
>  SPHINXOPTS =
> @@ -120,3 +121,15 @@ endif
>  .PHONY: htmldocs
>  .PHONY: check-docs
>  .PHONY: clean-docs
> +
> +ALL_LOCAL += sphinx-version-check
> +sphinx-version-check: $(EXTRA_DIST)
> +	@if grep -n -f $(srcdir)/Documentation/sphinx-version-
> blacklist $?; \
> +	then \
> +	  echo "See above for list of uses of features that Sphinx
> 1.1.3"; \

Assuming Sphinx do semantic versioning correctly (they do), we can
simply say Sphinx 1.1 here.

> +	  echo "does not support.  Please avoid using these
> features.."; \
> +	  exit 1; \
> +	else \
> +          : > $@; \
> +	fi
> +CLEANFILES += sphinx-version-check
> diff --git a/Documentation/sphinx-version-blacklist
> b/Documentation/sphinx-version-blacklist
> new file mode 100644
> index 000000000000..a67339bf2758
> --- /dev/null
> +++ b/Documentation/sphinx-version-blacklist
> @@ -0,0 +1,2 @@
> +code-block:: *ps1con
> +code-block:: *doscon
Ilya Maximets March 9, 2017, 3:15 p.m. UTC | #2 
On 07.03.2017 21:54, Ben Pfaff wrote:
> Signed-off-by: Ben Pfaff <blp@ovn.org>
> Acked-by: Stephen Finucane <stephen@that.guru>
> ---
>  Documentation/automake.mk              | 15 ++++++++++++++-
>  Documentation/sphinx-version-blacklist |  2 ++
>  2 files changed, 16 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/sphinx-version-blacklist
> 
> diff --git a/Documentation/automake.mk b/Documentation/automake.mk
> index a74807fde532..f7f1fe61d1b7 100644
> --- a/Documentation/automake.mk
> +++ b/Documentation/automake.mk
> @@ -86,7 +86,8 @@ EXTRA_DIST += \
>  	Documentation/internals/contributing/documentation-style.rst \
>  	Documentation/internals/contributing/libopenvswitch-abi.rst \
>  	Documentation/internals/contributing/submitting-patches.rst \
> -	Documentation/requirements.txt
> +	Documentation/requirements.txt \
> +	Documentation/sphinx-version-blacklist
>  
>  # You can set these variables from the command line.
>  SPHINXOPTS =
> @@ -120,3 +121,15 @@ endif
>  .PHONY: htmldocs
>  .PHONY: check-docs
>  .PHONY: clean-docs
> +
> +ALL_LOCAL += sphinx-version-check
> +sphinx-version-check: $(EXTRA_DIST)
> +	@if grep -n -f $(srcdir)/Documentation/sphinx-version-blacklist $?; \
> +	then \
> +	  echo "See above for list of uses of features that Sphinx 1.1.3"; \
> +	  echo "does not support.  Please avoid using these features.."; \
> +	  exit 1; \
> +	else \
> +          : > $@; \
> +	fi
> +CLEANFILES += sphinx-version-check
> diff --git a/Documentation/sphinx-version-blacklist b/Documentation/sphinx-version-blacklist
> new file mode 100644
> index 000000000000..a67339bf2758
> --- /dev/null
> +++ b/Documentation/sphinx-version-blacklist
> @@ -0,0 +1,2 @@
> +code-block:: *ps1con
> +code-block:: *doscon

I don't feel this patch is fully correct, because it's not the features of
sphinx. And its version not really connected with version of 'pygments' library.

What do you think?

Best regards, Ilya Maximets.
Ben Pfaff March 9, 2017, 11:27 p.m. UTC | #3 
On Thu, Mar 09, 2017 at 06:15:13PM +0300, Ilya Maximets wrote:
> On 07.03.2017 21:54, Ben Pfaff wrote:
> > Signed-off-by: Ben Pfaff <blp@ovn.org>
> > Acked-by: Stephen Finucane <stephen@that.guru>
> > ---
> >  Documentation/automake.mk              | 15 ++++++++++++++-
> >  Documentation/sphinx-version-blacklist |  2 ++
> >  2 files changed, 16 insertions(+), 1 deletion(-)
> >  create mode 100644 Documentation/sphinx-version-blacklist
> > 
> > diff --git a/Documentation/automake.mk b/Documentation/automake.mk
> > index a74807fde532..f7f1fe61d1b7 100644
> > --- a/Documentation/automake.mk
> > +++ b/Documentation/automake.mk
> > @@ -86,7 +86,8 @@ EXTRA_DIST += \
> >  	Documentation/internals/contributing/documentation-style.rst \
> >  	Documentation/internals/contributing/libopenvswitch-abi.rst \
> >  	Documentation/internals/contributing/submitting-patches.rst \
> > -	Documentation/requirements.txt
> > +	Documentation/requirements.txt \
> > +	Documentation/sphinx-version-blacklist
> >  
> >  # You can set these variables from the command line.
> >  SPHINXOPTS =
> > @@ -120,3 +121,15 @@ endif
> >  .PHONY: htmldocs
> >  .PHONY: check-docs
> >  .PHONY: clean-docs
> > +
> > +ALL_LOCAL += sphinx-version-check
> > +sphinx-version-check: $(EXTRA_DIST)
> > +	@if grep -n -f $(srcdir)/Documentation/sphinx-version-blacklist $?; \
> > +	then \
> > +	  echo "See above for list of uses of features that Sphinx 1.1.3"; \
> > +	  echo "does not support.  Please avoid using these features.."; \
> > +	  exit 1; \
> > +	else \
> > +          : > $@; \
> > +	fi
> > +CLEANFILES += sphinx-version-check
> > diff --git a/Documentation/sphinx-version-blacklist b/Documentation/sphinx-version-blacklist
> > new file mode 100644
> > index 000000000000..a67339bf2758
> > --- /dev/null
> > +++ b/Documentation/sphinx-version-blacklist
> > @@ -0,0 +1,2 @@
> > +code-block:: *ps1con
> > +code-block:: *doscon
> 
> I don't feel this patch is fully correct, because it's not the features of
> sphinx. And its version not really connected with version of 'pygments' library.

OK, can you explain the real problem then?  We're making changes to the
documentation on the basis that old versions of Sphinx does not support
features.
Ilya Maximets March 10, 2017, 7:47 a.m. UTC | #4 
On 10.03.2017 02:27, Ben Pfaff wrote:
> On Thu, Mar 09, 2017 at 06:15:13PM +0300, Ilya Maximets wrote:
>> On 07.03.2017 21:54, Ben Pfaff wrote:
>>> Signed-off-by: Ben Pfaff <blp@ovn.org>
>>> Acked-by: Stephen Finucane <stephen@that.guru>
>>> ---
>>>  Documentation/automake.mk              | 15 ++++++++++++++-
>>>  Documentation/sphinx-version-blacklist |  2 ++
>>>  2 files changed, 16 insertions(+), 1 deletion(-)
>>>  create mode 100644 Documentation/sphinx-version-blacklist
>>>
>>> diff --git a/Documentation/automake.mk b/Documentation/automake.mk
>>> index a74807fde532..f7f1fe61d1b7 100644
>>> --- a/Documentation/automake.mk
>>> +++ b/Documentation/automake.mk
>>> @@ -86,7 +86,8 @@ EXTRA_DIST += \
>>>  	Documentation/internals/contributing/documentation-style.rst \
>>>  	Documentation/internals/contributing/libopenvswitch-abi.rst \
>>>  	Documentation/internals/contributing/submitting-patches.rst \
>>> -	Documentation/requirements.txt
>>> +	Documentation/requirements.txt \
>>> +	Documentation/sphinx-version-blacklist
>>>  
>>>  # You can set these variables from the command line.
>>>  SPHINXOPTS =
>>> @@ -120,3 +121,15 @@ endif
>>>  .PHONY: htmldocs
>>>  .PHONY: check-docs
>>>  .PHONY: clean-docs
>>> +
>>> +ALL_LOCAL += sphinx-version-check
>>> +sphinx-version-check: $(EXTRA_DIST)
>>> +	@if grep -n -f $(srcdir)/Documentation/sphinx-version-blacklist $?; \
>>> +	then \
>>> +	  echo "See above for list of uses of features that Sphinx 1.1.3"; \
>>> +	  echo "does not support.  Please avoid using these features.."; \
>>> +	  exit 1; \
>>> +	else \
>>> +          : > $@; \
>>> +	fi
>>> +CLEANFILES += sphinx-version-check
>>> diff --git a/Documentation/sphinx-version-blacklist b/Documentation/sphinx-version-blacklist
>>> new file mode 100644
>>> index 000000000000..a67339bf2758
>>> --- /dev/null
>>> +++ b/Documentation/sphinx-version-blacklist
>>> @@ -0,0 +1,2 @@
>>> +code-block:: *ps1con
>>> +code-block:: *doscon
>>
>> I don't feel this patch is fully correct, because it's not the features of
>> sphinx. And its version not really connected with version of 'pygments' library.
> 
> OK, can you explain the real problem then?  We're making changes to the
> documentation on the basis that old versions of Sphinx does not support
> features.

The real problem is the version of 'pygments' library. Sphinx uses this library
to highlight code blocks.
So, RHEL7.3 contains package 'python-pygments-2.0.2', but lexers 'ps1con' and
'doscon' was introduced only in 'pygments-2.1'. That is why build fails.

'''
class pygments.lexers.shell.MSDOSSessionLexer
    Short names:	doscon
    Filenames:	None
    MIME types:	None

    Lexer for simplistic MSDOS sessions.

    New in version 2.1.

class pygments.lexers.shell.PowerShellSessionLexer
    Short names:	ps1con
    Filenames:	None
    MIME types:	None

    Lexer for simplistic Windows PowerShell sessions.

    New in version 2.1.
'''

On page [1] of 'pygments' project you can check the minimal version required
for every lexer.

Maybe we need to add minimal version of 'pygments' to requirements.txt .
In this case we will be able to create a whitelist of all supported lexers.

Another option:
Do we need the code highlighting at all?
We can just replace all the '.. code-block:: <something>' with simple '::' [2].
In this case, we will not have any external dependencies other than sphinx.

P.S. My previous patch [3] is just about ability to build documentation
     with sphinx 1.1 because there is no any reason to block it.

[1] http://pygments.org/docs/lexers/
[2] http://www.sphinx-doc.org/en/stable/rest.html#source-code
[3] https://mail.openvswitch.org/pipermail/ovs-dev/2017-March/329590.html

Best regards, Ilya Maximets.
Ilya Maximets March 10, 2017, 9:31 a.m. UTC | #5 
I've sent the patch for removing highlighting at all here:
https://mail.openvswitch.org/pipermail/ovs-dev/2017-March/329651.html

Only 'windows.rst' uses this functionality. So, I think, it's better
to just remove it and forbid the highlighting to avoid any issues
with external dependencies.

Best regards, Ilya Maximets.

On 10.03.2017 10:47, Ilya Maximets wrote:
> On 10.03.2017 02:27, Ben Pfaff wrote:
>> On Thu, Mar 09, 2017 at 06:15:13PM +0300, Ilya Maximets wrote:
>>> On 07.03.2017 21:54, Ben Pfaff wrote:
>>>> Signed-off-by: Ben Pfaff <blp@ovn.org>
>>>> Acked-by: Stephen Finucane <stephen@that.guru>
>>>> ---
>>>>  Documentation/automake.mk              | 15 ++++++++++++++-
>>>>  Documentation/sphinx-version-blacklist |  2 ++
>>>>  2 files changed, 16 insertions(+), 1 deletion(-)
>>>>  create mode 100644 Documentation/sphinx-version-blacklist
>>>>
>>>> diff --git a/Documentation/automake.mk b/Documentation/automake.mk
>>>> index a74807fde532..f7f1fe61d1b7 100644
>>>> --- a/Documentation/automake.mk
>>>> +++ b/Documentation/automake.mk
>>>> @@ -86,7 +86,8 @@ EXTRA_DIST += \
>>>>  	Documentation/internals/contributing/documentation-style.rst \
>>>>  	Documentation/internals/contributing/libopenvswitch-abi.rst \
>>>>  	Documentation/internals/contributing/submitting-patches.rst \
>>>> -	Documentation/requirements.txt
>>>> +	Documentation/requirements.txt \
>>>> +	Documentation/sphinx-version-blacklist
>>>>  
>>>>  # You can set these variables from the command line.
>>>>  SPHINXOPTS =
>>>> @@ -120,3 +121,15 @@ endif
>>>>  .PHONY: htmldocs
>>>>  .PHONY: check-docs
>>>>  .PHONY: clean-docs
>>>> +
>>>> +ALL_LOCAL += sphinx-version-check
>>>> +sphinx-version-check: $(EXTRA_DIST)
>>>> +	@if grep -n -f $(srcdir)/Documentation/sphinx-version-blacklist $?; \
>>>> +	then \
>>>> +	  echo "See above for list of uses of features that Sphinx 1.1.3"; \
>>>> +	  echo "does not support.  Please avoid using these features.."; \
>>>> +	  exit 1; \
>>>> +	else \
>>>> +          : > $@; \
>>>> +	fi
>>>> +CLEANFILES += sphinx-version-check
>>>> diff --git a/Documentation/sphinx-version-blacklist b/Documentation/sphinx-version-blacklist
>>>> new file mode 100644
>>>> index 000000000000..a67339bf2758
>>>> --- /dev/null
>>>> +++ b/Documentation/sphinx-version-blacklist
>>>> @@ -0,0 +1,2 @@
>>>> +code-block:: *ps1con
>>>> +code-block:: *doscon
>>>
>>> I don't feel this patch is fully correct, because it's not the features of
>>> sphinx. And its version not really connected with version of 'pygments' library.
>>
>> OK, can you explain the real problem then?  We're making changes to the
>> documentation on the basis that old versions of Sphinx does not support
>> features.
> 
> The real problem is the version of 'pygments' library. Sphinx uses this library
> to highlight code blocks.
> So, RHEL7.3 contains package 'python-pygments-2.0.2', but lexers 'ps1con' and
> 'doscon' was introduced only in 'pygments-2.1'. That is why build fails.
> 
> '''
> class pygments.lexers.shell.MSDOSSessionLexer
>     Short names:	doscon
>     Filenames:	None
>     MIME types:	None
> 
>     Lexer for simplistic MSDOS sessions.
> 
>     New in version 2.1.
> 
> class pygments.lexers.shell.PowerShellSessionLexer
>     Short names:	ps1con
>     Filenames:	None
>     MIME types:	None
> 
>     Lexer for simplistic Windows PowerShell sessions.
> 
>     New in version 2.1.
> '''
> 
> On page [1] of 'pygments' project you can check the minimal version required
> for every lexer.
> 
> Maybe we need to add minimal version of 'pygments' to requirements.txt .
> In this case we will be able to create a whitelist of all supported lexers.
> 
> Another option:
> Do we need the code highlighting at all?
> We can just replace all the '.. code-block:: <something>' with simple '::' [2].
> In this case, we will not have any external dependencies other than sphinx.
> 
> P.S. My previous patch [3] is just about ability to build documentation
>      with sphinx 1.1 because there is no any reason to block it.
> 
> [1] http://pygments.org/docs/lexers/
> [2] http://www.sphinx-doc.org/en/stable/rest.html#source-code
> [3] https://mail.openvswitch.org/pipermail/ovs-dev/2017-March/329590.html
> 
> Best regards, Ilya Maximets.
>
Ben Pfaff March 17, 2017, 8:18 p.m. UTC | #6 
OK, thanks, we'll go with that approach then.

On Fri, Mar 10, 2017 at 12:31:20PM +0300, Ilya Maximets wrote:
> I've sent the patch for removing highlighting at all here:
> https://mail.openvswitch.org/pipermail/ovs-dev/2017-March/329651.html
> 
> Only 'windows.rst' uses this functionality. So, I think, it's better
> to just remove it and forbid the highlighting to avoid any issues
> with external dependencies.
> 
> Best regards, Ilya Maximets.
> 
> On 10.03.2017 10:47, Ilya Maximets wrote:
> > On 10.03.2017 02:27, Ben Pfaff wrote:
> >> On Thu, Mar 09, 2017 at 06:15:13PM +0300, Ilya Maximets wrote:
> >>> On 07.03.2017 21:54, Ben Pfaff wrote:
> >>>> Signed-off-by: Ben Pfaff <blp@ovn.org>
> >>>> Acked-by: Stephen Finucane <stephen@that.guru>
> >>>> ---
> >>>>  Documentation/automake.mk              | 15 ++++++++++++++-
> >>>>  Documentation/sphinx-version-blacklist |  2 ++
> >>>>  2 files changed, 16 insertions(+), 1 deletion(-)
> >>>>  create mode 100644 Documentation/sphinx-version-blacklist
> >>>>
> >>>> diff --git a/Documentation/automake.mk b/Documentation/automake.mk
> >>>> index a74807fde532..f7f1fe61d1b7 100644
> >>>> --- a/Documentation/automake.mk
> >>>> +++ b/Documentation/automake.mk
> >>>> @@ -86,7 +86,8 @@ EXTRA_DIST += \
> >>>>  	Documentation/internals/contributing/documentation-style.rst \
> >>>>  	Documentation/internals/contributing/libopenvswitch-abi.rst \
> >>>>  	Documentation/internals/contributing/submitting-patches.rst \
> >>>> -	Documentation/requirements.txt
> >>>> +	Documentation/requirements.txt \
> >>>> +	Documentation/sphinx-version-blacklist
> >>>>  
> >>>>  # You can set these variables from the command line.
> >>>>  SPHINXOPTS =
> >>>> @@ -120,3 +121,15 @@ endif
> >>>>  .PHONY: htmldocs
> >>>>  .PHONY: check-docs
> >>>>  .PHONY: clean-docs
> >>>> +
> >>>> +ALL_LOCAL += sphinx-version-check
> >>>> +sphinx-version-check: $(EXTRA_DIST)
> >>>> +	@if grep -n -f $(srcdir)/Documentation/sphinx-version-blacklist $?; \
> >>>> +	then \
> >>>> +	  echo "See above for list of uses of features that Sphinx 1.1.3"; \
> >>>> +	  echo "does not support.  Please avoid using these features.."; \
> >>>> +	  exit 1; \
> >>>> +	else \
> >>>> +          : > $@; \
> >>>> +	fi
> >>>> +CLEANFILES += sphinx-version-check
> >>>> diff --git a/Documentation/sphinx-version-blacklist b/Documentation/sphinx-version-blacklist
> >>>> new file mode 100644
> >>>> index 000000000000..a67339bf2758
> >>>> --- /dev/null
> >>>> +++ b/Documentation/sphinx-version-blacklist
> >>>> @@ -0,0 +1,2 @@
> >>>> +code-block:: *ps1con
> >>>> +code-block:: *doscon
> >>>
> >>> I don't feel this patch is fully correct, because it's not the features of
> >>> sphinx. And its version not really connected with version of 'pygments' library.
> >>
> >> OK, can you explain the real problem then?  We're making changes to the
> >> documentation on the basis that old versions of Sphinx does not support
> >> features.
> > 
> > The real problem is the version of 'pygments' library. Sphinx uses this library
> > to highlight code blocks.
> > So, RHEL7.3 contains package 'python-pygments-2.0.2', but lexers 'ps1con' and
> > 'doscon' was introduced only in 'pygments-2.1'. That is why build fails.
> > 
> > '''
> > class pygments.lexers.shell.MSDOSSessionLexer
> >     Short names:	doscon
> >     Filenames:	None
> >     MIME types:	None
> > 
> >     Lexer for simplistic MSDOS sessions.
> > 
> >     New in version 2.1.
> > 
> > class pygments.lexers.shell.PowerShellSessionLexer
> >     Short names:	ps1con
> >     Filenames:	None
> >     MIME types:	None
> > 
> >     Lexer for simplistic Windows PowerShell sessions.
> > 
> >     New in version 2.1.
> > '''
> > 
> > On page [1] of 'pygments' project you can check the minimal version required
> > for every lexer.
> > 
> > Maybe we need to add minimal version of 'pygments' to requirements.txt .
> > In this case we will be able to create a whitelist of all supported lexers.
> > 
> > Another option:
> > Do we need the code highlighting at all?
> > We can just replace all the '.. code-block:: <something>' with simple '::' [2].
> > In this case, we will not have any external dependencies other than sphinx.
> > 
> > P.S. My previous patch [3] is just about ability to build documentation
> >      with sphinx 1.1 because there is no any reason to block it.
> > 
> > [1] http://pygments.org/docs/lexers/
> > [2] http://www.sphinx-doc.org/en/stable/rest.html#source-code
> > [3] https://mail.openvswitch.org/pipermail/ovs-dev/2017-March/329590.html
> > 
> > Best regards, Ilya Maximets.
> >
diff mbox

Patch

diff --git a/Documentation/automake.mk b/Documentation/automake.mk
index a74807fde532..f7f1fe61d1b7 100644
--- a/Documentation/automake.mk
+++ b/Documentation/automake.mk
@@ -86,7 +86,8 @@  EXTRA_DIST += \
 	Documentation/internals/contributing/documentation-style.rst \
 	Documentation/internals/contributing/libopenvswitch-abi.rst \
 	Documentation/internals/contributing/submitting-patches.rst \
-	Documentation/requirements.txt
+	Documentation/requirements.txt \
+	Documentation/sphinx-version-blacklist
 
 # You can set these variables from the command line.
 SPHINXOPTS =
@@ -120,3 +121,15 @@  endif
 .PHONY: htmldocs
 .PHONY: check-docs
 .PHONY: clean-docs
+
+ALL_LOCAL += sphinx-version-check
+sphinx-version-check: $(EXTRA_DIST)
+	@if grep -n -f $(srcdir)/Documentation/sphinx-version-blacklist $?; \
+	then \
+	  echo "See above for list of uses of features that Sphinx 1.1.3"; \
+	  echo "does not support.  Please avoid using these features.."; \
+	  exit 1; \
+	else \
+          : > $@; \
+	fi
+CLEANFILES += sphinx-version-check
diff --git a/Documentation/sphinx-version-blacklist b/Documentation/sphinx-version-blacklist
new file mode 100644
index 000000000000..a67339bf2758
--- /dev/null
+++ b/Documentation/sphinx-version-blacklist
@@ -0,0 +1,2 @@ 
+code-block:: *ps1con
+code-block:: *doscon