Conversation
2eecc6f to
dd8092e
Compare
you need to make a corresponding PR to our cmdliner fork
Is this needed? Can't you use directory targets for now?
It's not necessary. You can define multiple executables with the |
dd8092e to
e22205e
Compare
| @@ -1,4 +1,4 @@ | |||
| let executables = [ "main" ] | |||
| let executables = [ "main"; "man_page_gen" ] | |||
There was a problem hiding this comment.
I think bootstrapping did this automatically.
172941a to
52192d7
Compare
christinerose
left a comment
There was a problem hiding this comment.
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?
|
@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 |
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!) |
|
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 |
|
@emillon I see that there is a similar script for the installed versions. Two problems:
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. |
| open! Stdune | ||
| open Import | ||
|
|
||
| let all : _ Cmdliner.Cmd.t list = |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
52192d7 to
73f6da8
Compare
|
Needs diffing of directories. Had a go at implementing but ran out of time/motivation. Closing for now. |
We add a script to autogenerate man page rules which can be called using:
When these rules are present,
@check,@defaultand@docwill all update the man pages if there are any changes. Currently we do not delete redundant files.The motivation here is twofold:
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:
In the future if we have generated includes we can skip the manual rule generation.I've used directory targets instead 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?I have used the executables stanza and not exposed any new library now.