Write odoc configuration files

[Leonidas-from-XIV]

This PR revives #11716 by rebasing and addressing the outstanding issues.

Closes #13485.

[rgrinberg]

@jonludlam can you confirm that this is the file we need to create and install?

Can we write a test that involves reading this file somehow? To make sure that we're producing what odoc expects.

[Leonidas-from-XIV]

I am also unsure whether just marking all :with-doc dependencies as :post is the right thing to do, because the @doc target which we call when :with-doc is set requires odoc. Thus I think I need to special-case odoc at least to be a regular dependency.

[Leonidas-from-XIV]

@rgrinberg The tests fail because odoc-parser writes its own odoc-config.sexp. Do we have a way to introduce the rule in a way where a user-rule overrides it? Or some way to avoid producing the rule if a rule for the file already exists?

[rgrinberg]

I don't think so. Maybe you could add some sort of an odoc specific hack and ask @jonludlam to remove this rule?

[jonludlam]

Does this PR always create an odoc-config.sexp, or only when you've actually specified doc dependencies? I'm surprised by the number of changes it's making to the existing tests.

[jonludlam]

I am also unsure whether just marking all :with-doc dependencies as :post is the right thing to do, because the @doc target which we call when :with-doc is set requires odoc. Thus I think I need to special-case odoc at least to be a regular dependency.

I don't think this is the right thing to do. I think post dependencies ought to be explicitly marked as such.

[Leonidas-from-XIV]

@jonludlam I've incorporated this comment to always generate the odoc-config.sexp, which required a lot of changes to tests.

[rgrinberg]

Yes, many of our tests don't put much effort into making sure they're observing the output they need. I don't think we should shy away from doing the simplest thing here.

Does this PR always create an odoc-config.sexp

Yes. Detecting whether odoc is a dependency or not is actually not trivial in the presence of dependency disjunctions. It's not worth opening a can of form to save ourselves generating a 1 line file.

[jonludlam]

Yes, many of our tests don't put much effort into making sure they're observing the output they need. I don't think we should shy away from doing the simplest thing here.

Does this PR always create an odoc-config.sexp

Yes. Detecting whether odoc is a dependency or not is actually not trivial in the presence of dependency disjunctions. It's not worth opening a can of form to save ourselves generating a 1 line file.

It seems a bit silly to me to be generating a file that just contains (packages) in the vast majority of cases.

[jonludlam]

Also if it were conditional, we wouldn't need the odoc-parser hack (which also will break the build of odoc and anything else that's following the current guidance in the odoc documentation).

[rgrinberg]

I might have misunderstood the purpose of this config, but why would it be empty? Why wouldn't it include every single package in the project?

[Leonidas-from-XIV]

I've discussed solutions with @Alizter and we had a number of proposals to solve the odoc-config.sexp issue with odoc.

1. We could allow something like we do when generating OPAM files. Basically odoc-config.sexp.template files which the rules would incorporate into the generated odoc-config.sexp. This however requires changing odoc to migrate to use a template file (and it would silently do nothing on older Dune versions).
2. We could add a new stanza in the (package) to opt out of dune generating the rule, if the user wants to avoid the auto-generation and create the odoc-config.sexp on their own. This could be done e.g. by gating on the dune-lang of the project, so 3.22+ would generate the rule with an opt-out and it would not be generated on older versions.

[jonludlam]

I might have misunderstood the purpose of this config, but why would it be empty? Why wouldn't it include every single package in the project?

The purpose of this file is to let odoc_driver figure out what extra packages need to be available in order to produce complete documentation. It can figure out the 'usual' documentation dependencies itself - ie, the union of everything in the dependency cone of all of the libraries in the package.

Basically we've got by without this file for years by only allowing links to docs in your dependency cone, but that's quite a big restriction. The reason odoc-parser has one of these is because it has a link to odoc's docs. This is also the reason why we need to be able to have 'post' dependencies in the opam file - to avoid creating dependency cycles.

[jonludlam]

I've discussed solutions with @Alizter and we had a number of proposals to solve the odoc-config.sexp issue with odoc.

1. We could allow something like we do when generating OPAM files. Basically `odoc-config.sexp.template` files which the rules would incorporate into the generated `odoc-config.sexp`. This however requires changing odoc to migrate to use a `template` file (and it would silently do nothing on older Dune versions).

2. We could add a new stanza in the `(package)` to opt out of dune generating the rule, if the user wants to avoid the auto-generation and create the `odoc-config.sexp` on their own. This could be done e.g. by gating on the dune-lang of the project, so 3.22+ would generate the rule with an opt-out and it would not be generated on older versions.

I'm not really sure what problem you're trying to solve here. The only reason odoc-parser is generating its own odoc-config.sexp is that dune doesn't currently generate it for us. As soon as this PR is merged I'll be dropping that logic and adopting the new syntax. I can't see a reason why anyone would want to have any template?

If we make the generation of this file conditional on there being anything in it at all, don't all the current problems go away?

[rgrinberg]

Basically we've got by without this file for years by only allowing links to docs in your dependency cone, but that's quite a big restriction. The reason odoc-parser has one of these is because it has a link to odoc's docs. This is also the reason why we need to be able to have 'post' dependencies in the opam file - to avoid creating dependency cycles.

I see. How do you make sure that odoc-parser cannot link to packages outside of its dependency cone and this limited set of packages? Moreover, don't our odoc rules need dependencies on these packages to produce the links?

[jonludlam]

I see. How do you make sure that odoc-parser cannot link to packages outside of its dependency cone and this limited set of packages?

Include paths, more-or-less. Links don't resolve unless we can verify the destination is there, and this means that the odoc files have to be present at link time.

Moreover, don't our odoc rules need dependencies on these packages to produce the links?

Yes - this is all included in the odoc-v3 PR I made before xmas, though in that case it was using the older mechanism of specifying the dependencies separately in the dune-project file. It should be straightforward to adapt that to use the normal deps filtered by 'with-doc'.

[rgrinberg]

Okay, that all make sense. Whether the empty file is installed makes no difference, because its contents is determined by these dependency specifications anyway. I would have liked to see if there's a design that allowed folks to produce package installations without the reliance on the package stanza. Dune tries to accommodate other package managers when it is possible. So far, by having them mostly ignore dune's package management/opam features. With this change, I think they'll need to make sure this package metadata is maintained to produce a build that supports odoc.

I still don't get this point:

Also if it were conditional, we wouldn't need the odoc-parser hack (which also will break the build of odoc and anything else that's following the current guidance in the odoc documentation).

I don't see how guarding the generation of this file behind constraints solves this problem though. There's already a tension here:

1. The current guidance recommends adding a rule to install this file
2. Many packages already depend on odoc for generating documentation. I guess this means they can also reference odoc's docs?

So I think you should drop your recommendation ASAP, or we should properly accommodate it in dune.

[Leonidas-from-XIV]

I'm not really sure what problem you're trying to solve here.

I'm trying to solve the problem of what happens if project generate their own odoc-config.sexp. What odoc does is pretty much a valid thing to do and if Dune starts generating odoc-config.sexp automatically, this means that we have prevented users from writing and installing their own files or customixing what they want the configuration to say. Thus I was thinking of ways that we can generate files automatically but leave users that need customization an escape hatch.

[Leonidas-from-XIV]

I would have liked to see if there's a design that allowed folks to produce package installations without the reliance on the package stanza.

We could include a similar solution as the original PR, adding a doc depends stanza:

(doc_depends
  ((name my)
   (depends brr jsoo))
  ((name another)
   (depends wasmoo)))

However that seems to be reinventing the wheel as one could also use the package stanza without Dune-generated opam files and Dune would still write the correct odoc-config.sexp files.

[rgrinberg]

@jonludlam I just realized that this feature is completely unavailable to those users that prefer to write their metadata in opam files. Is that intentional?

[shonfeder]

@rgrinberg if people are not using dune to declare package dependencies, then isn't it correct that dune not generate the odoc-config.sexp, since that just includes information about packages?

If users are only using opam for package definition, they can write their odoc-config.sexp by hand, right?