odoc: sherlodoc integration

[EmileTrotignon]

This changes the rules that build the doc to enable search using sherlodoc : https://github.com/art-w/sherlodoc/tree/jsoo
The version of sherlodoc that allows to do so is not released, this is why this is a draft PR for now. I will keep this message up to date regarding to correct version of sherlodoc to use.

There are rules both for @doc and @doc-new.

I have tests but they are not exhaustive. I do not know how to test what happens when sherlodoc is not installed, even though I tested it by hand.

Edit: sherlodoc has been released on opam, I am taking this out of draft.

[EmileTrotignon]

I notice there is an issue with the tests. Sherlodoc can be present or absent, but when it is absent, a warning is printed when the doc is built. This a good thing for the user, but for the tests its quite terrible as the warning shows up in a lot of places, even when it does not on my switch that has sherlodoc. Should I add sherlodoc as a test deps to fix this ?

[emillon]

Sherlodoc can be present or absent, but when it is absent, a warning is printed when the doc is built. This a good thing for the user

I'm not sure about that. I would prefer if the availability of sherlodoc would just result in progressive enhancement. (if it's not installed, do the same as before; if it's installed, users get a new feature).

Should I add sherlodoc as a test deps to fix this ?

The way we deal with this in dune's test suite is by creating a fake sherlodoc binary. It can print its argument, emit fake JS if you like, etc. This will also isolate the test suite from the specific version of sherlodoc that's being used.
You can see how refmt is handled, for example.

Do you want remarks on the implementation itself or is it too early?

[EmileTrotignon]

I'm not sure about that. I would prefer if the availability of sherlodoc would just result in progressive enhancement. (if it's not installed, do the same as before; if it's installed, users get a new feature).

I am just concerned that users would never know about sherlodoc if they are never told to install it.

The way we deal with this in dune's test suite is by creating a fake sherlodoc binary. It can print its argument, emit fake JS if you like, etc. This will also isolate the test suite from the specific version of sherlodoc that's being used.
You can see how refmt is handled, for example.

That makes sense. It does not give a way to hide the binary though (which is only useful if we keep the warning).

Do you want remarks on the implementation itself or is it too early?

Yes please !

[emillon]

I am just concerned that users would never know about sherlodoc if they are never told to install it.

Dune is not a communication channel, it's a build system :)

It does not give a way to hide the binary though (which is only useful if we keep the warning).

You can either make a dedicated test, or make the existing one more modular by making several workspaces in it. You can also remove or chmod the binary as part of your test.

Regarding detecting the binary and setting up rules conditionally on that, we'll have to discuss that with the other maintainers, since there's another feature that'll require that, but in the meantime you can use resolve_program_memo.

[EmileTrotignon]

Dune is not a communication channel, it's a build system :)

I see what you mean. It's not only a communication issue, my opinion is that sherlodoc should be packaged with odoc, I tried this to alleviate this issue. It's not dune's concern though, I'll remove it.

You can either make a dedicated test, or make the existing one more modular by making several workspaces in it. You can also remove or chmod the binary as part of your test.

Even with multiple test, is there a way to prevent dune from finding something that is installed ? I don't want the "sherlodoc is absent" tests to only work if sherlodoc is not installed on the current switch.

Regarding detecting the binary and setting up rules conditionally on that, we'll have to discuss that with the other maintainers, since there's another feature that'll require that, but in the meantime you can use resolve_program_memo

Fine !

[emillon]

Even with multiple test, is there a way to prevent dune from finding something that is installed ? I don't want the "sherlodoc is absent" tests to only work if sherlodoc is not installed on the current switch.

Ah yes it can be a bit tricky to isolate from the dev switch. We have a couple tests that do that, for example https://github.com/ocaml/dune/blob/3.13.0/test/blackbox-tests/test-cases/melange/missing-melc.t#L13-L18 (and then we set PATH=_path without refering to the existing $PATH).

[emillon]

I think that the ~search_db business can be considerably simplified since it always evaluates to the same path. so we can make bits refer to that path and it will get built only if necessary. I'll try something in that direction.

[emillon]

OK, I had a look and indeed I think we can simplify this a lot by not making search_db depend on the sherlodoc parts:

• (you can revert "Refactoring of installation checks" since the rest assumes the old API)
• store the location of the search db in a new field in odoc_artefact
• at build time, you can set this to Sherlodoc.Path.db_dot_js ~dir:(Paths.html ctx target)
• Sherlodoc.odoc_args only needs this path, it can determine whether sherlodoc is active
• the rest of the rules have to set up which odocl files are attached to each target. I don't think you have to check if sherlodoc is present, you can set the rule unconditionally.

That should simplify considerably the work you have to do. Do you need help with that or do you want to have a go at it?

[EmileTrotignon]

I looked at it for a while, I think I would appreciate help

[EmileTrotignon]

I have pushed a commit that make the search global in @doc-new. The reasons have been discussed with jon, and they are that the intent behind doc-new is to provide docs for the devs of the current project : it includes private libraries and dependencies by default.
The best search scope for this feature is to have everything be indexed in the same db, this allows to search through deps, and across the whole project.

@doc has more limited scope more suitable for sharing the docs (smaller js files, ability to upload separate parts of the same workspace separately)

[emillon]

One thing you can do with the new fake sherlodoc I added is cat the db.js files that are generated. They contain the arguments passed to sherlodoc so you can see what docs contain which parts.

[emillon]

this is now good to go in my opinion, except for the todo which I'm not sure is about.

[emillon]

Thanks. In my view, this is ready to go.