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

feature: Autogenerate man pages #6648

Closed

Conversation

Alizter
Copy link
Collaborator

@Alizter Alizter commented Dec 6, 2022

We add a script to autogenerate man page rules which can be called using:

./dune.exe build @gen-man-rules

When these rules are present, @check, @default and @doc will all update the man pages if there are any changes. Currently we do not delete redundant files.

The motivation here is twofold:

  1. We can have command line documentation present in the repo without the need for users to compile the executables.
  2. We can observe better what the user comes across.

When all the files are out in the open, it is easy to see we have some nonsensical documentation. Some Dune commands are talking about options that don't apply to them etc.

cc @snowleopard who suggested something like this.

The patch for cmdliner: ocaml-dune/cmdliner#2

Notes:

  1. I had to modify cmdliner in order to crawl over the subcommands. cc @emillon does this patch look OK/maintainable? It will probably never make it upstream without good reason.
  2. In the future if we have generated includes we can skip the manual rule generation. I've used directory targets instead now.
  3. I had to move things around so that the top Dune cmdliner command became visible in a library. Is this OK to do or should something else be done? I have used the executables stanza and not exposed any new library now.

@rgrinberg
Copy link
Member

I had to modify cmdliner in order to crawl over the subcommands. cc @emillon does this patch look OK/maintainable? It will probably never make it upstream without good reason.

you need to make a corresponding PR to our cmdliner fork

In the future if we have generated includes we can skip the manual rule generation.

Is this needed? Can't you use directory targets for now?

I had to move things around so that the top Dune cmdliner command became visible in a library. Is this OK to do or should something else be done?

It's not necessary. You can define multiple executables with the executables stanza.

@Alizter Alizter force-pushed the ps/rr/feature__autogenerate_man_pages branch from dd8092e to e22205e Compare December 6, 2022 21:49
bin/man_page_gen.ml Outdated Show resolved Hide resolved
bin/man_page_gen.ml Outdated Show resolved Hide resolved
@@ -1,4 +1,4 @@
let executables = [ "main" ]
let executables = [ "main"; "man_page_gen" ]
Copy link
Member

Choose a reason for hiding this comment

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

why do we need this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think bootstrapping did this automatically.

doc/dune Outdated Show resolved Hide resolved
doc/dune Outdated Show resolved Hide resolved
@Alizter Alizter force-pushed the ps/rr/feature__autogenerate_man_pages branch 2 times, most recently from 172941a to 52192d7 Compare December 6, 2022 22:35
@Alizter Alizter requested a review from snowleopard December 6, 2022 22:40
@Alizter Alizter marked this pull request as ready for review December 6, 2022 23:03
Copy link
Collaborator

@christinerose christinerose left a comment

Choose a reason for hiding this comment

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

There’s more for me to go through here, but before I do, I’d like clarification on my questions (so I don’t end up suggesting a ton of things you have to reject because I misunderstood!)

Also, how do you write monospace in .1 files?

bin/dune_cmd.ml Show resolved Hide resolved
bin/dune_cmd.ml Show resolved Hide resolved
bin/dune_cmd.ml Show resolved Hide resolved
doc/man/dune-build.1 Show resolved Hide resolved
doc/man/dune-build.1 Show resolved Hide resolved
doc/man/dune-build.1 Show resolved Hide resolved
doc/man/dune-build.1 Show resolved Hide resolved
doc/man/dune-build.1 Show resolved Hide resolved
doc/man/dune-build.1 Show resolved Hide resolved
doc/man/dune-build.1 Show resolved Hide resolved
@Alizter
Copy link
Collaborator Author

Alizter commented Dec 6, 2022

@christinerose Thanks for taking a look at this, however I don't think it will be too useful to review the documentation here. The documentation here is just added to the repository and is what the user will see when they do dune some command --help. We should definitely improve the documentation but that should be in a future PR. The goal of this PR is to just expose it.

@christinerose
Copy link
Collaborator

christinerose commented Dec 6, 2022

@christinerose Thanks for taking a look at this, however I don't think it will be too useful to review the documentation here. The documentation here is just added to the repository and is what the user will see when they do dune some command --help. We should definitely improve the documentation but that should be in a future PR. The goal of this PR is to just expose it.

Ah! Thanks for letting me know. I was pinged to review as code owner automatically, I guess. It was in my notifications, so I thought it was necessary! Thanks for clarifying.

(I’m glad I didn’t go through all 55 pages then!)

@emillon
Copy link
Collaborator

emillon commented Dec 7, 2022

I'll make a more complete review today, but before I dig into the implementation: we're already doing something like that to install man pages, see doc/dune.inc. I don't think you're reusing that mechanism. You should be able to turn these rules into (mode promote) and get them in the source tree "for free".

@Alizter
Copy link
Collaborator Author

Alizter commented Dec 7, 2022

@emillon I see that there is a similar script for the installed versions. Two problems:

  1. These are not the plain man pages but rather the groff ones.
  2. The script is not as extensive as what I am doing currently. (In fact there is even a CR there talking about what I have done).

(mode promote) and get them in the source tree "for free".

I know about this and I had it originally however it doesn't seem right for us. We don't want the runtest alias to actually promote anything and that should really be left up to the user. For this reason, I am investigating adding diff and promotion for directory targets to allow us to promote them separately.

@Alizter Alizter requested a review from christinerose December 7, 2022 11:52
open! Stdune
open Import

let all : _ Cmdliner.Cmd.t list =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it necessary to move code around? We have some internal patches (like adding new subcommands) that will require some extra work. If we can avoid moving the code that would be helpful.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately this is the only way I could think of. The entry point for Dune is the same as the cmdliner definition which means the cmdliner cmd cannot be shared with other executables without placing it in its own file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, thanks. This is certainly not a blocker, just an extra cost of the current implementation to keep in mind (perhaps, when comparing with alternatives).

<!-- ps-id: 6961205d-dda4-4138-a91f-f104f90fba81 -->

Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter Alizter force-pushed the ps/rr/feature__autogenerate_man_pages branch from 52192d7 to 73f6da8 Compare December 13, 2022 09:09
@Alizter
Copy link
Collaborator Author

Alizter commented Jan 13, 2023

Needs diffing of directories. Had a go at implementing but ran out of time/motivation. Closing for now.

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.

5 participants