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

Refactor odoc generation and fix the private doc alias #864

Merged
merged 14 commits into from
Jun 7, 2018

Conversation

rgrinberg
Copy link
Member

private-doc needed to collect the html alias of each library rather than the odoc alias. The rest are just small refactorings.

@diml I think I addressed the comment that you've left for me in the source. Failing when the transitive closure is absent indeed makes sense. Even if we can still technically continue and generate suboptimal docs.

There are still many inefficiencies abound in the doc generation, but I'm thinking that this isn't an issue for people in practice. As long as the rules minimize the weight they add on normal builds.

@rgrinberg rgrinberg requested a review from a user June 7, 2018 07:31
@rgrinberg rgrinberg force-pushed the doc-private branch 2 times, most recently from a1b8d2b to 9864a12 Compare June 7, 2018 07:37
@rgrinberg
Copy link
Member Author

@diml in the last commit, I'm using a slightly different approach for calculating the closure for private and public libs. This is because we want libraries in the same package to refer to each other in the docs (even if there's no dependency)

@rgrinberg
Copy link
Member Author

This should fix the private doc generation @bobbypriambodo @bluddy

@ghost
Copy link

ghost commented Jun 7, 2018

@diml I think I addressed the comment that you've left for me in the source. Failing when the transitive closure is absent indeed makes sense. Even if we can still technically continue and generate suboptimal docs.

This makes sense to me.

@ghost
Copy link

ghost commented Jun 7, 2018

BTW, I have only a very superficial knowledge of odoc. @rgrinberg if there are specific commits or parts you'd like additional review on let me know, otherwise as long as it produce the expected results I think we can merge this without additional review.

@bobbypriam
Copy link
Contributor

bobbypriam commented Jun 7, 2018

Thanks @rgrinberg! I can try it out in a few hours if @bluddy doesn't beat me to it.

@rgrinberg
Copy link
Member Author

@diml the review request was mostly for your comment that I addressed here. The private docs are generated as expected on my end.

@ghost
Copy link

ghost commented Jun 7, 2018

Ok, I looked at this commit and I agree with it.

rgrinberg added 13 commits June 7, 2018 17:59
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
cc @diml

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
Separate package from html docs

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
private-doc should collect html rather than odoc aliases

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
This allows for circular dependencies for libraries in the same package

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@bobbypriam
Copy link
Contributor

I just tried the changes in this PR, and it generated the private docs correctly.

One behavior I find a bit odd is that build @doc and build @doc-private seem to be mutually exclusive, in that running one alias means removing the result of the other. Is that intended? My initial expectation is that either they leave each other alone, or somehow @doc-private implies @doc.

@rgrinberg
Copy link
Member Author

I believe this from this bit: https://github.com/rgrinberg/jbuilder/blob/doc-private/src/odoc.ml#L166-L173

@trefis did you write this initially? How come we need to clear the html directory?

@rgrinberg rgrinberg merged commit 79b8caf into ocaml:master Jun 7, 2018
@trefis
Copy link
Collaborator

trefis commented Jun 7, 2018

Err... can't remember.

Without much thinking I'd say that there can (theoretically) be some interaction between the two (e.g. some {!reference} in the public libraries that are left unresolved when private libraries are not available to odoc, but are otherwise).

@ghost
Copy link

ghost commented Jun 7, 2018

@trefis did you write this initially? How come we need to clear the html directory?

Actually that was me. It is hard to predict in advance what files odoc will produce. Odoc has a command to list the files it will create, but we don't have dynamic targets. So what we do instead is delete the directory where odoc produces the files every time before running odoc and we add this special keepme file to tell Dune to not eagerly delete the directory; since it doesn't know the files in it are targets, it would otherwise thinks they are stale artifacts.

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.

3 participants