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

Add manual page. #36

Merged
merged 2 commits into from
Sep 13, 2023
Merged

Add manual page. #36

merged 2 commits into from
Sep 13, 2023

Conversation

ckreibich
Copy link
Member

This uses the argparse_manpage Python package, which does a reasonably good job at turning the argparse parser into a manpage. The setup is manual (you have to run man/build.py to update the manpage), and installation happens only when bundled with Zeek (like for zkg etc).

Resolves #20.

Comment on lines +14 to +21
LOCALDIR = os.path.dirname(os.path.realpath(__file__))
ROOTDIR = os.path.normpath(os.path.join(LOCALDIR, ".."))

# Prepend the project's toplevel directory to the search path so we import the
# zeekclient package locally.
sys.path.insert(0, ROOTDIR)

import zeekclient.cli # pylint: disable=wrong-import-position,import-error
Copy link
Member

@bbannier bbannier Sep 11, 2023

Choose a reason for hiding this comment

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

Nit: This is needed since the script lives in a subdirectory which is not a submodule of zeekclient. If you moved the file to e.g., the root directory (maybe renamed as build_manpage.py; also requires changing the write below) you would be able to simply use

import zeekclient.cli

This then also wouldn't require suppressing any pylint errors.

Comment on lines +8 to +12
try:
from argparse_manpage.manpage import Manpage
except ImportError:
print("The manpage builder needs the argparse_manpage package.")
sys.exit(1)
Copy link
Member

Choose a reason for hiding this comment

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

I think the default exception text for ImportError is already pretty useful, and creating a custom one isn't usually needed, especially for tooling intended for developers.

If you added this to work around pylint failing to import this, instead add it to the list of pylint dependencies. Regardless of motivation, you should consider adding the dependency to the pylint pre-commit hook so it can check the code,

- repo: https://github.com/PyCQA/pylint
  rev: v3.0.0a7
  hooks:
  - id: pylint
    additional_dependencies:
      - argparse_manpage
      - websocket-client

.SS \fBzeek\-client get\-instances\fR
Show instances connected to the controller.

usage: build.py get\-instances [\-h]
Copy link
Member

Choose a reason for hiding this comment

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

It looks like setting parser.prog in man/build.py is insufficient, not exactly sure how to fix that.

    # The parser currently has this script as program name, so replace:
    parser.prog = "zeek-client"

Here and below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh nice — I missed that! I'm fixing it by changing the name in sys.argv, prior to the parser generation.

Makefile Outdated
Comment on lines 21 to 24
.PHONY: man
man:
cd man && ./build.py

Copy link
Member

@bbannier bbannier Sep 11, 2023

Choose a reason for hiding this comment

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

The argparse-manpage docs document a way to set up building manpages automatically as part of setup.py. I think that would preferable over having the output in the repo.

If you want to track them in the repo, let's add a pre-commit hook which makes sure that the manpage is up-to-date:

- repo: local
  hooks:
  - id: generate-manpage
    name: Generate manpage
    language: python
    additional_dependencies:
      - argparse_manpage
      - websocket-client
    entry: ./build_manpage.py

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't really want to generate the manpage on the fly because we're not doing that for any of the other manpages in the project either, but I like the idea of tracking whether the current one is outdated. Will do.

Copy link
Member

@bbannier bbannier left a comment

Choose a reason for hiding this comment

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

LGTM module my new comment on checking whether the manpage is up-to-date with a pre-commit hook.

.pre-commit-config.yaml Show resolved Hide resolved
@@ -0,0 +1,166 @@
.TH ZEEK\-CLIENT "1" "2023\-09\-11" "zeek\-client" "Generated Python Manual"
Copy link
Member

Choose a reason for hiding this comment

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

The argparse_manpage module by defaults embeds the time the build job was run into the generated manpage. This probably makes sense, but interferes with running this from a pre-commit hook as the output is not reproducible.

The module supports reproducible builds with SOURCE_DATE_EPOCH. We could set this in build.py before calling Manpage, e.g.,

os.environ['SOURCE_DATE_EPOCH'] = "1694507965"

This is unfortunately pretty opaque and would need a comment.

Git does not preserve mtime, so e.g., the following unfortunately would not work:

os.environ['SOURCE_DATE_EPOCH'] = f"{int(os.path.getmtime(__file__))}"

If you believe this is too much hassle to implement, we should not use this as a pre-commit hook.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I just removed the date altogether.

Comment on lines +36 to +43
parser.description = """A command-line client for Zeek's Management Framework.

Use this client to push cluster configurations to a cluster controller, retrieve
running state from the system, restart nodes, and more.

For details about Zeek's Management Framework, please consult the documentation
at https://docs.zeek.org/en/master/frameworks/management.html.
"""
Copy link
Member

Choose a reason for hiding this comment

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

This could arguably be in the default description.

This uses the argparse_manpage Python package, which does a reasonably good job
at turning the argparse parser into a manpage. Patch up the manpage object in a
few ways prior to rendering to ensure consistent output and additional detail
over --help output.
@ckreibich ckreibich merged commit 969280f into master Sep 13, 2023
6 checks passed
@ckreibich ckreibich deleted the topic/christian/manpage branch September 13, 2023 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Man page missing :)
2 participants