Skip to content
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

Move from asciidoc to rst2man to generate man pages #37861

Merged
merged 1 commit into from
Mar 26, 2018

Conversation

aerostitch
Copy link
Contributor

@aerostitch aerostitch commented Mar 23, 2018

SUMMARY

Asciidoc is EOL and only supports python2.
Upstream clearly encourages people to move to an equivalent such as asciidoctor, see: https://github.com/asciidoc/asciidoc/releases/tag/8.6.10
As the maintainer of asciidoc in Debian I'm working with the different projects to help the transition, hence the current PR.

ISSUE TYPE
  • Prevent a bug that will happen when asciidoc will be removed from the distributions
COMPONENT NAME

Makefile => make docs command and corresponding docker installations

ANSIBLE VERSION
2.6.0 0.0.devel
ADDITIONAL INFORMATION

Note that in the Makefile, the ASCII2HTMLMAN seems to be unused (and the command itself was sending an error when trying it directly in the shell. I modified it to have a working command but I doubt it's useful.
Let me know if I should remove it.

Thanks for your help!
Joseph

@ansibot
Copy link
Contributor

ansibot commented Mar 23, 2018

@ansibot ansibot added bug This issue/PR relates to a bug. docker needs_triage Needs a first human triage before being processed. new_contributor This PR is the first contribution by a new community member. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests. labels Mar 23, 2018
@@ -4,7 +4,7 @@ RUN yum clean all && \
yum -y install epel-release && \
yum -y install \
acl \
asciidoc \
asciidoctor \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and the other containers in the same directory are no longer used to build docs, so they shouldn't be updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we just remove asciidoc from there then? I mean it brings a LOT of stuffs with dblatex by default. This would lighten the images too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave them as-is for now. I'm going to be working on newer images and things can be cleaned up then.

@mattclay mattclay removed docker needs_triage Needs a first human triage before being processed. test This PR relates to tests. labels Mar 23, 2018
@aerostitch
Copy link
Contributor Author

aerostitch commented Mar 23, 2018

Thanks for the review @mattclay :) I removed the changes on the Dockerfile as discussed.
Did you see my note on ASCII2HTMLMAN ?

@mattclay
Copy link
Member

It looks like ASCII2HTMLMAN was never used, so it should be safe to remove.

@aerostitch
Copy link
Contributor Author

Thanks, done.

@mattclay
Copy link
Member

/cc @nitzmahone @abadger

@abadger
Copy link
Contributor

abadger commented Mar 24, 2018

I think we should move away from asciidoc in favour of using rst or jinja2 templates to generate man pages. The one problem I see with asciidoctor is that it means bringing ing introduces a ruby stack for building docs. I'm not sure what the overlap of people who install from source and people who want docs but those who do probably won't be entirely pleased by that.

Dependencies on Fedora:

 rubygem-asciidoctor                noarch               1.5.6.1-1.fc27                  fedora                217 k
Installing dependencies:
 ruby                               x86_64               2.4.3-87.fc27                   updates                80 k
 ruby-irb                           noarch               2.4.3-87.fc27                   updates                97 k
 ruby-libs                          x86_64               2.4.3-87.fc27                   updates               2.8 M
 rubygem-openssl                    x86_64               2.0.5-87.fc27                   updates               181 k
 rubygem-psych                      x86_64               2.2.2-87.fc27                   updates                90 k
 rubygems                           noarch               2.6.14.1-87.fc27                updates               294 k
 rubypick                           noarch               1.1.1-7.fc27                    fedora                 10 k
Installing weak dependencies:
 rubygem-bigdecimal                 x86_64               1.3.0-87.fc27                   updates                91 k
 rubygem-did_you_mean               x86_64               1.1.0-87.fc27                   updates                83 k
 rubygem-io-console                 x86_64               0.4.6-87.fc27                   updates                61 k
 rubygem-rdoc                       noarch               5.1.0-2.fc27                    fedora                447 k

@alikins I think you were the one who worked on moving the html program documentation away from asciidoc; do you think it would be easy to generate the man pages using the same method? Do you have time to work on that?

@aerostitch
Copy link
Contributor Author

Thanks for the reply @abadger. If you want I can convert your asciidoc man pages to .rst and use either rst2man or sphynx to convert them to man pages. Those 2 are python and would avoid having to maintain a custom converter.

@abadger
Copy link
Contributor

abadger commented Mar 24, 2018 via email

@aerostitch aerostitch changed the title Move from asciidoc to asciidoctor to generate man pages Move from asciidoc to rst2man to generate man pages Mar 26, 2018
@aerostitch
Copy link
Contributor Author

Sorry for the late reply @abadger .
I moved the generation of man pages to rst2man instead of asciidoc.
Let me know if there's anything else you want me to change there.

Thanks,
Joseph

@ansibot
Copy link
Contributor

ansibot commented Mar 26, 2018

@ansibot ansibot added the docs This issue/PR relates to or includes documentation. label Mar 26, 2018
@abadger
Copy link
Contributor

abadger commented Mar 26, 2018

@aerostitch Looks good.

@dharmabumstead alikins, bcoca, and I like this change. Feel free to merge once you've reviewed.

Makefile Outdated
ASCII2HTMLMAN = a2x -L -D docs/html/man/ -d manpage -f xhtml
MANPAGES ?= $(patsubst %.rst.in,%,$(wildcard ./docs/man/man1/ansible*.1.rst.in))
ifneq ($(shell which rst2man 2>/dev/null),)
ASCII2MAN = rst2man $< $@
else
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add an extra conditional for rst2man.py?

If you just pip install docutils you get a file dropped in bin with the .py extension.

Something like:

diff --git a/Makefile b/Makefile
index 112a997e7c..734613e05f 100644
--- a/Makefile
+++ b/Makefile
@@ -24,6 +24,8 @@ PREFIX ?= '/usr/local'
 MANPAGES ?= $(patsubst %.rst.in,%,$(wildcard ./docs/man/man1/ansible*.1.rst.in))
 ifneq ($(shell which rst2man 2>/dev/null),)
 ASCII2MAN = rst2man $< $@
+else ifneq ($(shell which rst2man.py 2>/dev/null),)
+ASCII2MAN = rst2man.py $< $@
 else
 ASCII2MAN = @echo "ERROR: rst2man command is not installed but is required to build $(MANPAGES)" && exit 1
 endif

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review @sivel . The problem with that is that if you install it using your package distro you will not get the .py extension. Example on Debian:

$ dpkg -L python-docutils | grep rst2man
/usr/share/docutils/scripts/python2/rst2man
$ dpkg -L python3-docutils | grep rst2man
/usr/share/docutils/scripts/python3/rst2man

So should we specify a fallback with the non-extension version?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about something like:

ifneq ($(shell which rst2man.py 2>/dev/null),)
ASCII2MAN = rst2man.py $< $@
else ifneq ($(shell which rst2man 2>/dev/null),)
ASCII2MAN = rst2man $< $@
else
ASCII2MAN = @echo "ERROR: rst2man from docutils command is not installed but is required to build $(MANPAGES)" && exit 1
endif

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is what I recommended in my diff; Adding a 2nd branch to check for .py. I just put the .py 2nd in my example. Preferring rst2man then rst2man.py

Either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, thanks, sorry I misread :\ . Amended.

@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Mar 26, 2018
@aerostitch
Copy link
Contributor Author

aerostitch commented Mar 26, 2018

@sivel I amended the commit so that it takes in account both rst2man.py (provided by pip) and rst2man (provided by distribution packages). Let me know if that's fine with you.
Thanks for the review! :)
Joseph

Copy link
Member

@sivel sivel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with the additional branch to look for rst2man.py and it works as expected.

@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Mar 26, 2018
Copy link
Contributor

@dharmabumstead dharmabumstead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@dharmabumstead dharmabumstead merged commit 00a7ff7 into ansible:devel Mar 26, 2018
@aerostitch aerostitch deleted the move_out_of_asciidoc branch March 26, 2018 23:53
@aerostitch
Copy link
Contributor Author

Thanks guys! :)

ryancurrah pushed a commit to ryancurrah/ansible that referenced this pull request Apr 4, 2018
@ansible ansible locked and limited conversation to collaborators Apr 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug This issue/PR relates to a bug. docs This issue/PR relates to or includes documentation. new_contributor This PR is the first contribution by a new community member. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants