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

feat(docs): generate cli docs directly from the struct #12717

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

akaladarshi
Copy link
Contributor

Related Issues

Fixes: #12706

@akaladarshi akaladarshi marked this pull request as ready for review November 26, 2024 15:13
@akaladarshi akaladarshi requested review from masih and rvagg November 26, 2024 15:14
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
cli/miner/actor_test.go Outdated Show resolved Hide resolved
@rvagg
Copy link
Member

rvagg commented Nov 28, 2024

Looks like a lot of the logic for this lives in https://github.com/urfave/cli/blob/main/help.go and the templates in here (unfortunately not exported!) are here: https://github.com/urfave/cli/blob/main/template.go

Use this as an exercise in minimising the diff, that's your challenge - how can you get to near zero red/green lines in the diff here for the documentation output, aiming to only make changes where the existing script is getting it wrong (mostly missing some things I believe).

@akaladarshi akaladarshi force-pushed the akaladarshi/refactor-docsgen-cli branch from 1f2c2c4 to f616568 Compare December 1, 2024 14:24
@BigLep
Copy link
Member

BigLep commented Dec 3, 2024

@akaladarshi : I assume you'll re-request review when you're ready for it to be looked at again.

@akaladarshi akaladarshi requested a review from rvagg December 3, 2024 12:00
documentation/en/cli-lotus.md Outdated Show resolved Hide resolved
cli/pprof.go Outdated Show resolved Hide resolved
documentation/en/cli-lotus.md Outdated Show resolved Hide resolved
@rvagg
Copy link
Member

rvagg commented Dec 4, 2024

Very close @akaladarshi, good work so far. I haven't done an in-depth review of the actual code, I'm mainly looking at the output and assuming that the code is good, I'll look over that next.

cli/clicommands/cmd.go Outdated Show resolved Hide resolved
@akaladarshi
Copy link
Contributor Author

@rvagg I think I found a minimum solution for this we can directly use the urface cli package for generating, Initially I though we need to print everything (hidden), but this will directly print the correct format value.

     customAppData := func() map[string]interface{} {
		return map[string]interface{}{
			"ExtraInfo": g.app.ExtraInfo,
		}
	}

	cli.HelpPrinterCustom(header, cli.AppHelpTemplate, g.app, customAppData())

@akaladarshi akaladarshi requested a review from rvagg December 4, 2024 14:11
@akaladarshi
Copy link
Contributor Author

@rvagg I think I found a minimum solution for this we can directly use the urface cli package for generating, Initially I though we need to print everything (hidden), but this will directly print the correct format value.

     customAppData := func() map[string]interface{} {
		return map[string]interface{}{
			"ExtraInfo": g.app.ExtraInfo,
		}
	}

	cli.HelpPrinterCustom(header, cli.AppHelpTemplate, g.app, customAppData())

Removed the above change in favour of app.Run(args[]string), because it was not printing the default values of flags. Anyway app.Run internally uses cli.HelpPrinter to print the commands.

Makefile Show resolved Hide resolved
scripts/docsgen-cli/main.go Outdated Show resolved Hide resolved
@akaladarshi akaladarshi requested a review from rvagg December 19, 2024 11:05
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@rvagg rvagg force-pushed the akaladarshi/refactor-docsgen-cli branch from b9a161b to 7fe23b0 Compare January 6, 2025 03:00
@rvagg
Copy link
Member

rvagg commented Jan 6, 2025

Squashed down to a single commit and rebased onto current master in this branch.

@rvagg rvagg requested a review from rjan90 January 6, 2025 03:18
@rvagg
Copy link
Member

rvagg commented Jan 6, 2025

After a make clean deps, the current master has make docsgen-cli running at 25s. This branch does the same in 17s. The catch is that docsgen-config is a separate target and still requires the lotus and lotus-miner binaries (just not lotus-worker), so make docsgen-cli docsgen-config, being now a two-step process that first requires a partial compile (for go run) and a full compile for the two binaries needed for config, so it ends up ebing 30s, i.e. longer. But at least you can choose, and the config files are just another batch of work to be done that I want to separate from this PR.

@rjan90 please have a look at the changes I made in the head commit here, I've put docsgen-config as a separate required target to be run on releases and I also put it in CI.

still requires compiled binaries to produce the config, separate from
docsgen-cli which does not require them
@rvagg rvagg force-pushed the akaladarshi/refactor-docsgen-cli branch from 7c96bf3 to 14ca842 Compare January 6, 2025 03:31
@rvagg
Copy link
Member

rvagg commented Jan 6, 2025

I think follow-up work here to make the config output should be straightforward but it would be worth testing whether it should be part of the same binary or not, it might be faster to keep it in the same thing rather than have two binaries it has to setup and execute.

@rvagg
Copy link
Member

rvagg commented Jan 6, 2025

OK, nevermind about the follow-up task, I just bundled it in here and got rid of the need to lotus config > out at all, so now it's all part of the docsgen-cli app so it runs relatively quickly in the same binary. It's still not super-fast because it does need to load and compile a lot of code in the various paths that the CLI app touches, it just doesn't need to form a proper binary with all of the inclusions that a full make does.

make docsgen-cli after a clean deps takes 17s now. So we've shaved off approximately 1/3rd of the time for this task. I've got rid of the docsgen-config task I introduced.

@rjan90 if you wouldn't mind reviewing that would be appreciated. You're welcome to focus on the two additional commits I added since they are minimal and I already reviewed the larger initial commit.

@rvagg rvagg force-pushed the akaladarshi/refactor-docsgen-cli branch from 7ce784c to db0dc42 Compare January 6, 2025 03:58
Comment on lines +2211 to +2214
lotus-miner log list - List log systems

USAGE:
lotus log list [command options] [arguments...]
lotus-miner log list [command options] [arguments...]
Copy link
Contributor

@rjan90 rjan90 Jan 6, 2025

Choose a reason for hiding this comment

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

I do not think this is correct? If I understand the intent of the changes/PR in general, it seems like we will still have lotus auth api-info, lotus log list, lotus log set-level and lotus log alerts commands, and therefor having these being written as lotus-miner commands in the Lotus CLI docs seems wrong?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, you're right, good catch; the annoyance is that the command objects are used in both places and they get a certain amount of internal state built up over use, and we use them twice so this name gets sticky; I'll figure out a fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ⌨️ In Progress
Development

Successfully merging this pull request may close these issues.

Use urfav Command struct directly to generate docsgen-cli
4 participants