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

highlight.pack.js script removed after first build #153

Closed
rizo opened this issue Jun 14, 2018 · 13 comments
Closed

highlight.pack.js script removed after first build #153

rizo opened this issue Jun 14, 2018 · 13 comments
Labels
bug Something isn't working command line output
Milestone

Comments

@rizo
Copy link
Collaborator

rizo commented Jun 14, 2018

Currently when generating docs with dune (dune build @doc) the generated HTML uses the highlight.pack.js javascript library for syntax highlighting. This script is copied to _build/default/_doc/_html/ along with odoc.css by the odoc/assets.ml/write function.

For some reason the second, and all consecutive builds, remove the highlight.pack.js file.

To reproduce run the following in any dune-managed package:

$ dune clean
$ dune build @doc
$ ls _build/default/_doc/_html/
highlight.pack.js  index.html  odoc.css  <package>
$ dune build @doc
$ ls _build/default/_doc/_html/
index.html  odoc.css  <package>        # <-- highlight.pack.js is missing

I find this strange because both the css and the javascript files seem to be handled equally in odoc, but only the javascript file disappears.

@aantron aantron added the bug Something isn't working label Oct 12, 2018
@aantron aantron added this to the 1.3.1 milestone Oct 25, 2018
@aantron
Copy link
Contributor

aantron commented Oct 31, 2018

This happens because odoc.css is a known target to Dune. By contrast, from Dune's point of view, it is an accident that odoc also emits highlight.pack.js. Dune then ends up deleting highlight.pack.js here.

I see several ways to solve this:

  1. List highlight.pack.js as a target (and dependency) in Dune (here and here).

    However, this way is a bad idea, because it will have to be repeated every time we change support files, resulting in an unreasonable coupling between odoc and Dune (and odig, and everything else). That will produce lots of version incompatibilities, constraints, etc. See also Add odoc support-files-targets command #174.

  2. Add an odoc support-files-targets command (Add odoc support-files-targets command #174) that emits the names of the files odoc wants to generate.

  3. We could instead have the convention that odoc writes all of its support files into a subdirectory of the HTML root, like assets or static, whose contents are opaque to the build system, and whose presence is all that is tested.

I suggest (3) because, in the future, odoc might need to output different sets of files depending on, for example, the theme. So, the implementation of odoc support-files-targets could become complicated. It seems to me that (3) is the solution that is easiest on both build tools and on odoc.

If the contents of the directory are affected by theme or other conditions, it might need to be regenerated when the theme or other condition changes. I'm not sure, at this point, if this will necessarily be detectable by the build system. If not, we should probably go with (2).

@dbuenzli, @rgrinberg, @rizo, what do you think?

@rgrinberg
Copy link
Member

I think all the solutions are fine. Replying to each individual solution:

  1. To be honest, I don't think this is so bad. In practice, it doesn't seem to change very often and we already have to do this in dune for menhir, jsoo, etc. anyways.

  2. This is alright too. It could even be extended to have odoc spit out the inter dependencies of various targets.

  3. A fine solution as well. In dune, we already discussed adding this feature as "directory targets" in the context of integrating sphinx doc generation as part of a dune build. This solution is useful in other contexts, but also requires the most work.

@lpw25
Copy link
Contributor

lpw25 commented Oct 31, 2018

Personally, I prefer option 2. It is how the rest of odoc works, and it is the easiest option for build systems. Depending on the details of the build system "directory targets" can be awkward to implement in a way that is completely reliable and deterministic.

@dbuenzli
Copy link
Contributor

I also would really prefer option 2, that's the way build system friendly tools work.

aantron added a commit that referenced this issue Oct 31, 2018
@rgrinberg
Copy link
Member

Btw, one issue is that I'm not sure how it would work when composing odoc into builds that you'd like to generate docs for. The issue in this case is that dune would like to know the list of target statically. That is, without building anything. With odoc inside the build, this can no longer be done as dune will need to build odoc first.

@aantron
Copy link
Contributor

aantron commented Oct 31, 2018

I noticed that Dune doesn't call compile-targets or html-targets. Is this the reason, or perhaps it's just a matter of Dune having been able to get away without it so far?

@aantron
Copy link
Contributor

aantron commented Oct 31, 2018

Anyway, it seems that Dune would have to be able to stratify its build to use support-files-targets (EDIT: in builds that include odoc).

@aantron
Copy link
Contributor

aantron commented Nov 20, 2018

@diml, we are thinking to add odoc support-files-targets (#232), so that odoc can tell Dune which files it intends to generate. @rgrinberg suggested above that depending on the odoc command being built in order to run support-files-targets may be difficult in builds that include odoc.

What is your opinion on this? Is support-files-targets good enough for use in Dune, or do we need to find some other solution?

@ghost
Copy link

ghost commented Nov 20, 2018

We wouldn't be able to use it straight away, as Dune doesn't support specifying dynamic targets. However, we are doing some work on the internals of Dune and we will eventually be able to support this.

Currently we just assume that the output of odoc is a whole directory.

@aantron
Copy link
Contributor

aantron commented Nov 20, 2018

In that case, I'll merge in support-files-targets, but solving this issue is blocked on Dune making use of support-files-targets in the future.

@dbuenzli
Copy link
Contributor

@aantron Formally that's not an issue with odoc that's an issue with dune. I think you can close this.

@aantron
Copy link
Contributor

aantron commented Nov 20, 2018

Yes, I will do so after I open an issue in ocaml/dune about it. I want to leave this open for a bit as a reminder.

@aantron
Copy link
Contributor

aantron commented Nov 20, 2018

Opened ocaml/dune#1556.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working command line output
Projects
None yet
Development

No branches or pull requests

5 participants