From 63413cb0d4ff1e31c3081b59b2abe7fe59207b70 Mon Sep 17 00:00:00 2001 From: Matt Clay Date: Mon, 31 Jul 2023 14:04:26 -0700 Subject: [PATCH] [stable-2.14] Fix misrendered sections in manpage generation (#81380) This change fixes bugs in the manpage generator that existed since it was first added. It exposes CLI `ARGUMENTS` value to manpage templates. Before this change, the code contained a typo, causing the `for`-loop iterate over individual characters of the `'ARGUMENTS'` string rather than iterating over a tuple. A missing comma was at fault. The updated code gets rid of the `for`-loop and conditionals since it seems to have been a premature complexity increase and no other things than `'ARGUMENTS'` were ever added into the broken iterable. The functional change is that `arguments` is now always present in the Jinja2 context, unlike being missing sometimes because of the previous design (not that it was ever present, because of the bug! sigh...) The Jinja2 templates perform an `{% if arguments %}` check, letting the template engine silently ignore the missing variable. The clause was always falsy, meaning that the arguments section was not included in the manpages for at least the last 6 years. With this fix, it will be. This patch also deduplicates calling `opt_doc_list` @ generate_man. It was called late in the execution, more times than necessary. This patch makes sure it happens once by putting it at the top of the scope. It fixes rendering library and inventory in manpages. The corresponding Jinja2 templates have blocks wrapped with conditionals like `{% if inventory %}` and `{% if library %}` but said variables were never injected into the context, nor were they even deduced on the Python side of the generator. This means that the conditional clauses were always falsy, never showing the portions of the manpages. The Python script has hints for how the `inventory` variable was to be calculated, which is confirmed through the Git paleontology efforts. The block of code that references to the `inventory` bit was incorrectly checking a variable with a list of nested objects for the presence of a string which was never going to work. This patch fixes this check by verifying the CLI flag against the correct variable containing a list of options and exposes it to the Jinja2 templates. It also exposes the `library` variable in a similar way. The block displaying other binaries in Sphinx CLI docs has been synchronized with the manpage template. Previously, the current binary was displayed also. This patch gets rid of the unwanted trailing comma there too. Finally, the CLI executables list in the manpage template now reuses the same variable as the RST template that doesn't need any post-processing in Jinja2. Before, it was already used in the RST template so this patch aligns both templates to use the same logic as they got out-of-sync over time. PR #80450. (cherry picked from commit a84b3a4e7277084466e43236fa78fc99592c641a) Co-authored-by: Sviatoslav Sydorenko --- docs/templates/cli_rst.j2 | 2 +- .../command_plugins/generate_man.py | 21 +++++++++---------- packaging/pep517_backend/_templates/man.j2 | 9 ++++---- 3 files changed, 15 insertions(+), 17 deletions(-) diff --git a/docs/templates/cli_rst.j2 b/docs/templates/cli_rst.j2 index 0e4c0f16831..8f5a8db3379 100644 --- a/docs/templates/cli_rst.j2 +++ b/docs/templates/cli_rst.j2 @@ -158,4 +158,4 @@ Ansible is released under the terms of the GPLv3+ License. See also ======== -{% for other in cli_bin_name_list|sort %}:manpage:`{{other}}(1)`, {% endfor %} +{% for other in cli_bin_name_list|sort %}{% if other != cli_name %}:manpage:`{{other}}(1)`{% if not loop.last %}, {% endif %}{% endif %}{% endfor %} diff --git a/hacking/build_library/build_ansible/command_plugins/generate_man.py b/hacking/build_library/build_ansible/command_plugins/generate_man.py index a8c668a0c05..a155b28f9b8 100644 --- a/hacking/build_library/build_ansible/command_plugins/generate_man.py +++ b/hacking/build_library/build_ansible/command_plugins/generate_man.py @@ -127,6 +127,7 @@ def opts_docs(cli_class_name, cli_module_name): pass # base/common cli info + cli_options = opt_doc_list(cli.parser) docs = { 'cli': cli_module_name, 'cli_name': cli_name, @@ -135,22 +136,18 @@ def opts_docs(cli_class_name, cli_module_name): 'long_desc': trim_docstring(cli.__doc__), 'actions': {}, 'content_depth': 2, + 'options': cli_options, + 'arguments': getattr(cli, 'ARGUMENTS', None), } option_info = {'option_names': [], - 'options': [], + 'options': cli_options, 'groups': []} - for extras in ('ARGUMENTS'): - if hasattr(cli, extras): - docs[extras.lower()] = getattr(cli, extras) - - common_opts = opt_doc_list(cli.parser) groups_info = get_option_groups(cli.parser) shared_opt_names = [] - for opt in common_opts: + for opt in cli_options: shared_opt_names.extend(opt.get('options', [])) - option_info['options'] = common_opts option_info['option_names'] = shared_opt_names option_info['groups'].extend(groups_info) @@ -211,7 +208,6 @@ def opts_docs(cli_class_name, cli_module_name): action_depth = get_actions(cli.parser, docs) docs['content_depth'] = action_depth + 1 - docs['options'] = opt_doc_list(cli.parser) return docs @@ -292,11 +288,14 @@ class GenerateMan(Command): # add rest to vars tvars = allvars[cli_name] - tvars['cli_list'] = cli_list tvars['cli_bin_name_list'] = cli_bin_name_list tvars['cli'] = cli_name - if '-i' in tvars['options']: + if '-i' in tvars['option_names']: + tvars['inventory'] = True print('uses inventory') + if '-M' in tvars['option_names']: + tvars['library'] = True + print('uses library') manpage = template.render(tvars) filename = os.path.join(output_dir, doc_name_formats[output_format] % tvars['cli_name']) diff --git a/packaging/pep517_backend/_templates/man.j2 b/packaging/pep517_backend/_templates/man.j2 index 8bd3644cbd5..9a8cb1d7931 100644 --- a/packaging/pep517_backend/_templates/man.j2 +++ b/packaging/pep517_backend/_templates/man.j2 @@ -1,6 +1,5 @@ -{% set name = ('ansible' if cli == 'adhoc' else 'ansible-%s' % cli) -%} -{{name}} -{{ '=' * ( name|length|int ) }} +{{ cli_name }} +{{ '=' * ( cli_name|length|int ) }} {{ '-' * ( short_desc|default('')|string|length|int ) }} {{short_desc|default('')}} @@ -14,7 +13,7 @@ SYNOPSIS -------- -{{ usage|replace('%prog', name) }} +{{ usage|replace('%prog', cli_name) }} DESCRIPTION @@ -120,7 +119,7 @@ Ansible is released under the terms of the GPLv3 license. SEE ALSO -------- -{% for other in cli_list|sort %}{% if other != cli %}**ansible{% if other != 'adhoc' %}-{{other}}{% endif %}** (1){% if not loop.last %}, {% endif %}{% endif %}{% endfor %} +{% for other in cli_bin_name_list|sort %}{% if other != cli_name %}**{{ other }}** (1){% if not loop.last %}, {% endif %}{% endif %}{% endfor %} Extensive documentation is available in the documentation site: .