-
Notifications
You must be signed in to change notification settings - Fork 3
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
Make topkg doc
work, so that topkg publish doc
also works.
#3
Conversation
src/topkg_jbuilder.ml
Outdated
OS.Cmd.run @@ Cmd.(jbuilder % "build" % "--root" % ".")) | ||
Log.debug (fun l -> l "files=%s" (String.concat ", " files)); | ||
match List.rev files with | ||
| "doc/api.docdir/index.html" :: _ -> (* topkg doc *) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty fragile. @dbuenzli is there a better way to know that we are building the docs? Something in the build context maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last time I looked at the topkg API I didn't see anything about it. But I think @dbuenzli wants to rework the handling of documentation in topkg.
In the meantime, let's just add a comment explaining what's happening. We can probably use List.mem
as well rather than checking only the last target.
|
||
let uerror = Unix.error_message | ||
|
||
let rec mkdir dir = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dbuenzli: any objection into moving mkdir
and copy
upstream into Topkg.OS.Dir
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Topkg.OS
doesn't depend on unix
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hum good point
This seems to be working fine, I managed to publish:
The only missing thing, from an usability point-of-view, is a missing top-level index, a la odoc that I could put on http://moby.github.io/datakit/index.html |
I think it would be fine to have a hand-written |
src/topkg_jbuilder.ml
Outdated
>>= fun () -> | ||
let src = "_build" // "default" // "_doc" in | ||
let dst = "_build" // "doc" // "api.docdir" in | ||
copy src dst |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why copy instead of rename? AFAIU, this is essentially a one time build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want to use Unix
initially but I had to due to mkdir
. So yes, I guess I could use rename.
I've force-pushed a new version. |
Note: this does not (yet) create a global index for all the documented packages
>>= fun () -> | ||
let src = "_build" // "default" // "_doc" in | ||
let dst = "_build" // "doc" // "api.docdir" in | ||
rmdir dst >>= fun () -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to remove the directory before? Is the build directory kept between run?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you do topkg doc
yes, it is kept.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. this is a lot of code for a workaround :(, but hopefully it's just temporary
@diml would it be possible to have a new release with this change? This is needed to have |
Note: this does not (yet) create a global index for all the documented packages