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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Man pages #7424

Closed
Closed

Conversation

mauricepoirrier
Copy link

Paired with @lightningorb

Change Description

This commits aim to generate man pages docs of lnd and lncli using formatting tools from already used packages.

To do this we:

  1. add a flag on lnd that given that generates documentation and exits
  2. add a hidden Command to lncli that generates documentation
  3. add a makefile command to generate both docs depending on the installation / building

Haven't checked tests in these files.

closes #5605

Steps to Test

make build
make docs

or

make build
./lncli-debug generatemanpage
./lnd-debug --manpage

Pull Request Checklist

Testing

  • Your PR passes all CI checks.
  • Tests covering the positive and negative (error paths) are included.
  • Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

馃摑 Please see our Contribution Guidelines for further guidance.

This commit aid to help in installation of man page for users. Running lnd --manpage generates the page using the same configuration as the application.
this commit aids to help generation of man page on installation of lnd. The command uses cli package to parse same app docs into a lncli.1 file.
this commit adds a new command to makefile to trigger the generation of man pages on installed lnd
Copy link
Collaborator

@ellemouton ellemouton left a comment

Choose a reason for hiding this comment

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

Hi @mauricepoirrier and @lightningorb, Super cool feature!! 馃殌

A few comments/suggestions from me:

  • At the moment, the man pages have to be manually moved to usr/local/shared/man/man1 in order for the man command to work. I think this should be automated.
  • Currently the lnd-debug and lncli-debug binaries are used to gen the man files which is not ideal since those are the names that appear in the man file.
  • Building on the above, I think make install should take care of everything: it should install lnd & lncli, build the man pages for both, and move the man pages to the correct locations.
  • the commit titles look good but the commit messages need to pls be wrapped correctly
  • The PR is also missioning a release notes entry

Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. This should be quite useful for package maintainers.

Do you know I tell the man command to correctly show the generated files? It looks scrambled if I do man -l lnd for example. Probably just a format/encoding issue.

docs:
@$(call print, "Generating man pages documents.")
if [ -f ./lnd-debug ] && [ -f ./lncli-debug ]; then \
./lnd-debug --manpage; \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we just depend on the build step to make sure the lnd-debug and lncli-debug files are present?

cmd/lncli/commands.go Show resolved Hide resolved
cmd/lncli/commands.go Show resolved Hide resolved
@@ -686,6 +688,17 @@ func LoadConfig(interceptor signal.Interceptor) (*Config, error) {
os.Exit(0)
}

if preCfg.GenerateManPage {
fileParser := flags.NewParser(&preCfg, flags.Default)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Couldn't this also be done in the lncli generatemanpage command? Doesn't look like we need the actual content of the preCfg, so we can just use the default config.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @guggero ! could you please elaborate more on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I figured it out while working on the coin-selection-strategy option issue. I just needed more time to get hands-on with the codebase!

config.go Show resolved Hide resolved
fileParser := flags.NewParser(&preCfg, flags.Default)
var buf bytes.Buffer
fileParser.WriteManPage(&buf)
err := ioutil.WriteFile("lnd.1", buf.Bytes(), 0644)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should use os.WriteFile instead of the deprecated ioutil one.

@lightninglabs-deploy
Copy link

@mauricepoirrier, remember to re-request review from reviewers when ready

@lightninglabs-deploy
Copy link

Closing due to inactivity

7 similar comments
@lightninglabs-deploy
Copy link

Closing due to inactivity

@lightninglabs-deploy
Copy link

Closing due to inactivity

@lightninglabs-deploy
Copy link

Closing due to inactivity

@lightninglabs-deploy
Copy link

Closing due to inactivity

@lightninglabs-deploy
Copy link

Closing due to inactivity

@lightninglabs-deploy
Copy link

Closing due to inactivity

@lightninglabs-deploy
Copy link

Closing due to inactivity

@guggero guggero closed this Aug 21, 2023
@mohamedawnallah
Copy link
Contributor

  • At the moment, the man pages have to be manually moved to usr/local/shared/man/man1 in order for the man command to work. I think this should be automated.
  • Building on the above, I think make install should take care of everything: it should install lnd & lncli, build the man pages for both, and move the man pages to the correct locations.

Hey @ellemouton moving the man pages of lnd and lncli to /usr/local/share/man/man1 would require the user to enter the password while running make install since the owner of this directory is the root ?

@Kixunil
Copy link
Contributor

Kixunil commented Feb 26, 2024

@mohamedawnallah the canonical way to resolve these things is to write use $DESTDIR and $PREFIX variables. E.g.:

install:
    install -m 755 lnd $(DESTDIR)$(PREFIX)/bin/lnd # same with lncli etc
    install -m 644 lnd.1 $(DESTDIR)$(PREFIX)/share/man/man1/lnd.1
    install -m 644 lncli.1 $(DESTDIR)$(PREFIX)/share/man/man1/lncli.1

This takes care of several issues:

  • Non-packaging users usually want PREFIX to be /usr/local while packaging users want it to be /
  • Packagers want to actually install into a temporary directory which can be set by DESTDIR, this is just empty in non-packaging scenarios
  • Yes, users who want to run make install directly have to run it as root, this is expected.

Note that PREFIX is usually configured with a ./configure script with --prefix= parameter. Though there's probably a simpler solution since LND doesn't need to know the prefix.

@ellemouton ellemouton mentioned this pull request Mar 7, 2024
8 tasks
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.

Feature: generate man pages
6 participants