argmin icon indicating copy to clipboard operation
argmin copied to clipboard

LaTeX support for argmin-testfunctions docs

Open airwoodix opened this issue 1 year ago • 2 comments

This patch adds LaTeX/KaTeX support to the argmin-testfunctions crate documentation.

The HTML code in argmin-testfunctions/katex-header.html is added to the header of all the documentation pages in that crate and loads the KaTeX auto-render extension from the recommended CDN. It may be possible to serve the KaTeX code from local files but I didn't investigate this.

As an example, the documentation of a couple of test function is migrated to using KaTeX rendering. For example that of the ackley function:

image

If it is acceptable, the migration of other documentation pages shall be done in subsequent PRs.

As a fly-by, I added some missing cross links and fixed some typos. This patch doesn't contain any Rust code changes.

Resolves #418.

Local build

The workspace's top-level Cargo configuration sets the RUSTDOCFLAGS to include the HTML header in all docs pages. This enables KaTeX support for the documentation of all crates for local builds. This is misleading because there's no support in the docs.rs build for crates other than argmin-testfunctions (see below). Unfortunately, Cargo does not read config files from crates within the workspace so there's no easy way to restrict that configuration to a single crate while keeping the unified workspace build.

docs.rs build

The docs.rs build was tested on a docs.rs development build. The docs.rs build is configured through data in the package.metadata.docs.rs table in the crate's manifest. Workspace metadata is not supported. Also, the referenced files must be part of the crate, so enabling support for other crates would probably require duplicating the katex-header.html file to the other crates, which is a bit of a maintenance burden.

Error handling

Syntax errors in the KaTeX code unfortunately only lead to broken rendering. Since the KaTeX code is interpreted in the browser, it may be quite involved to automatically propagate errors to the rustdoc call.

Reference

https://github.com/victe/rust-latex-doc-minimal-example (and many other publicly available crates)

airwoodix avatar Oct 21 '24 20:10 airwoodix

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 92.10%. Comparing base (98fa6a3) to head (e03140d).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #530      +/-   ##
==========================================
- Coverage   92.10%   92.10%   -0.01%     
==========================================
  Files         177      177              
  Lines       23675    23675              
==========================================
- Hits        21807    21806       -1     
- Misses       1868     1869       +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Oct 22 '24 04:10 codecov-commenter

Setting build.rustdocflags in the workspace root's Cargo configuration file breaks the book doctests workflow, because the path to katex-header.html doesn't resolve from media/book/tests. As a workaround, I added an override in the workflow step, such that the RUSTDOCFLAGS are empty when running the book doctests.

There's a similar issue with a similar workaround for the argmin-math tests (and anything that is not invoked with the workspace root as working directory).

Open point: is it preferable to set RUSTDOCFLAGS='--html-in-header ...' only during the actual docs build (instead of the global setting in .cargo/config.toml)?

airwoodix avatar Oct 22 '24 06:10 airwoodix

Firstly, this is an absolutely magnificent PR, both in terms of work done and description! Thank you for the massive effort you have taken to make this work!

Do I understand it correctly that the main problem is that both docs.rs and local builds have to be configured/set up separately? If that is the case, I think I can live with it.

Open point: is it preferable to set RUSTDOCFLAGS='--html-in-header ...' only during the actual docs build (instead of the global setting in .cargo/config.toml)?

Would this fix the discrepancy between docs.rs and local builds, or would this avoid the work arounds for argmin-math and media/book/tests (and instead requires setting RUSTDOCSFLAGS for argmin-testfunctions)?

The failing clippy lints should be fixed in main.

stefan-k avatar Oct 27 '24 06:10 stefan-k

Thanks for the feedback!

Do I understand it correctly that the main problem is that both docs.rs and local builds have to be configured/set up separately?

Yes, that's correct.

Would this fix the discrepancy between docs.rs and local builds, or would this avoid the work arounds for argmin-math and media/book/tests (and instead requires setting RUSTDOCSFLAGS for argmin-testfunctions)?

It would remove the workarounds for media/book/tests and argmin-math. Setting RUSTDOCFLAGS for the CI step would work and remove the new .cargo/config.toml and the mentioned workarounds:

diff --git a/.github/workflows/docs.yml b/.github/workflows/docs.yml
index bbd3a70c..affe0c1f 100644
--- a/.github/workflows/docs.yml
+++ b/.github/workflows/docs.yml
@@ -19,6 +19,8 @@ jobs:
           components: rustfmt,rust-src

       - name: Build documentation
+        env:
+          RUSTDOCFLAGS: "--html-in-header ./crates/argmin-testfunctions/katex-header.html"
         run: cargo doc --features "argmin-math/latest_all,argmin/full" --no-deps --workspace --exclude "example-*"

       - name: Forwarding

Caveats:

  • building locally during development is more cumbersome (one also needs to set RUSTDOCFLAGS in the local environment)
  • the HTML header is again applied to all the crates, not just to argmin-testfunctions. I'm not aware of a way to achieve the latter while keeping the single doc build command issued from the workspace root.

The failing clippy lints should be fixed in main.

Thanks! I rebased the branch on main.

airwoodix avatar Oct 28 '24 21:10 airwoodix

Apologies once again for the long wait!

Do I understand it correctly that the main problem is that both docs.rs and local builds have to be configured/set up separately?

Yes, that's correct.

This is fine with me.

Caveats:

* building locally during development is more cumbersome (one also needs to set `RUSTDOCFLAGS` in the local environment)

* the HTML header is again applied to all the crates, not just to `argmin-testfunctions`. I'm not aware of a way to achieve the latter while keeping the single doc build command issued from the workspace root.

Caveat 2 seems fine with me, but caveat 1 does sound cumbersome and it would be nice if this could be avoided.

Unfortunately I lost track a bit of this PR (apologies again, because I can't really express how excited I am about this!). Are there any decisions you need from me, or can this be merged?

stefan-k avatar Jan 03 '25 08:01 stefan-k

caveat 1 does sound cumbersome

I agree. The options I currently see are either an opinionated way of setting the environment (e.g. direnv) or introducing a task runner (e.g. just or make), both or which would be quite a change.

Are there any decisions you need from me, or can this be merged?

I rebased the branch and would stick with the initial (current) version:

  • locally, cargo doc --no-deps works from the workspace root, at the expense of the not-so-nice workarounds for book and argmin-math in the CI.
  • building the argmin-testfunctions crate by docs.rs works too.

airwoodix avatar Jan 26 '25 18:01 airwoodix

I rebased the branch and would stick with the initial (current) version:

* locally, `cargo doc --no-deps` works from the workspace root, at the expense of the not-so-nice workarounds for `book` and `argmin-math` in the CI.

* building the `argmin-testfunctions` crate by docs.rs works too.

Thanks, this sounds very good to me! Thanks for rebasing. Unfortunately the CI was still broken. Could you do another rebase, please? It should hopefully be fixed now :)

stefan-k avatar Mar 04 '25 13:03 stefan-k

@stefan-k Thanks for the feedback and the main branch fix. I rebased again and all CI jobs pass now on this branch too.

airwoodix avatar Mar 05 '25 06:03 airwoodix