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

Add #![doc(html_in_header)] #95691

Closed
wants to merge 1 commit into from
Closed

Conversation

CAD97
Copy link
Contributor

@CAD97 CAD97 commented Apr 5, 2022

Addressing rust-lang/cargo#331 with a specific solution rather than general.

Since the injection via doc(html_favicon_url) stopped working, it's not possible for crates to specify extra HTML that should be included just for that crate. --html-in-header, --html-before-content, and --html-after-content are good for handling HTML which should be present in all crates being documented, but something like KaTeX wants to be injected for only specific crates. Thus I believe #![doc(html_in_header)] is worth adding alongside html_logo_url and other #![doc] attribute customization points. A general solution to put metadata like this in Cargo.toml is generally preferable, but this is a targeted need with a targeted solution using the existing way of handling per-crate doc annotation.


I don't actually know the protocol for adding an unstable attribute like this. Help appreciated. If this is deemed as a reasonable unstable addition, I'll file the tracking issue (and whatever other unstable stuff needs to be done -- the unstable book?)

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Apr 5, 2022
@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 5, 2022
@CAD97
Copy link
Contributor Author

CAD97 commented Apr 5, 2022

(I've already had two tools break on me for the branch name containing parenthesis. It was not a good idea.)

@m-ou-se
Copy link
Member

m-ou-se commented Apr 5, 2022

Does #![doc(html_in_header = some_crate::some_macro!())] work?

@CAD97
Copy link
Contributor Author

CAD97 commented Apr 5, 2022

I'm trying to remember how to set up a local rustc for testing, and then I'll be able to tell you.

@rust-log-analyzer

This comment has been minimized.

@CAD97
Copy link
Contributor Author

CAD97 commented Apr 5, 2022

@m-ou-se No, with this simple implementation, it is required to be a string literal. I know some other attributes support include!, so it should be possible to do so here as well, but I don't know how or where to look 😅

I can, however, confirm that a local test works!

#![feature(doc_html_in_header)]
#![doc(html_in_header = r#"
<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/katex@0.15.3/dist/katex.min.css" integrity="sha384-KiWOvVjnN8qwAZbuQyWDIbfCLFhLXNETzBQjA/92pIowpC0d2O3nppDGQVgwd2nB" crossorigin="anonymous">
<script defer src="https://cdn.jsdelivr.net/npm/katex@0.15.3/dist/katex.min.js" integrity="sha384-0fdwu/T/EQMsQlrHCCHoH10pkPLlKA1jL5dFyUOvB3lfeT2540/2g6YgSi2BL14p" crossorigin="anonymous"></script>
<script>
document.addEventListener("DOMContentLoaded", function () {
    let to_do = [];
    for (let e of document.getElementsByTagName('pre')) {
        if (e.classList.contains('language-math')) {
            to_do.push(function () {
                let x = document.createElement('p');
                katex.render(e.innerText, x, {displayMode: true, throwOnError: false});
                e.parentNode.parentNode.replaceChild(x, e.parentNode);
            });
        }
    }
    for (let e of document.getElementsByTagName('code')) {
        let n = e.nextSibling; let p = e.previousSibling;
        if (n && p && /^\$/.test(n.data) && /\$$/.test(p.data)) {
            to_do.push(function () {
                let n = e.nextSibling; let p = e.previousSibling;
                let x = document.createElement('span');
                katex.render(e.innerText, x, {throwOnError: false});
                e.parentNode.replaceChild(x, e);
                n.splitText(1); n.remove();
                p.splitText(p.data.length - 1).remove();
            });
        }
    }
    for (let f of to_do) f();
});
</script>
"#)]

//! This crate is an example of using $`\LaTeX`$ math with rustdoc.
//!
//! This demo uses the unstable `#[doc(html_in_header = ..)]` attribute to
//! inject a KaTeX script into the generated documentation.
//!
//! This way, it works both on docs.rs and with `cargo doc` without extra settings.
//!
//! # Usage
//!
//! Look at the source of `lib.rs` of this crate, and copy the doc attribute
//! containing the `<link>` and `<script>` tags.
//!
//! Then, write ``$`\frac 1 2 + 3`$`` for inline math, which renders as
//! $`\frac 1 2 + 3`$.
//!
//! Or, write
//!
//! ````markdown
//! ```math
//! \int_{-\infty}^\infty f(x)\,dx
//! ```
//! ````
//!
//! for display math, which renders as:
//!
//! ```math
//! \int_{-\infty}^\infty f(x)\,dx
//! ```

image

@rust-log-analyzer

This comment has been minimized.

@petrochenkov
Copy link
Contributor

r? rust-lang/rustdoc

@rust-highfive rust-highfive assigned jsha and unassigned petrochenkov Apr 5, 2022
@clarfonthey
Copy link
Contributor

I like this change and would almost say that making macros work for the value is a must-have, since being able to include_str! would make maintaining things that are more than just a short tag much easier.

@CAD97
Copy link
Contributor Author

CAD97 commented Apr 25, 2022

#![doc(html_in_header = include_str!())] seems to not be something we have the capability of supporting currently:

https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/.23.5Bdoc.20.3D.20include_str!.5D/near/280005808

it's handled in the resolver during macro expansion, and it's not really possible to support it for nested attributes. See https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/Allow.20expression.20for.20doc.28alias.29.20too and https://internals.rust-lang.org/t/macro-expansion-points-in-attributes/11455 for more context. [ — Joshua Nelson ]

@clarfonthey
Copy link
Contributor

Oh, sad days. Honestly, it might even be worth only allowing it with file paths, then, since it seems like a much more common use case than string literals. But maybe I'm biased and someone else has more insight.

@apiraino
Copy link
Contributor

I'll untag T-compiler since these changes seem more relevant to rustdoc

@rustbot -T-compiler

@apiraino apiraino removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 28, 2022
@CAD97
Copy link
Contributor Author

CAD97 commented May 25, 2022

Actually... the --html-in-header flag takes a path, so it'd make sense for #![doc(html_in_header = "")] to also take a path. I'm unfortunately currently blocked on testing rustc changes,

change doc(html_in_header) to take a filepath
@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
configure: rust.debug-assertions := True
configure: rust.overflow-checks := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
* highest error code: E0787
Found 504 error codes
Found 0 error(s) in error codes
Done!
tidy error: /checkout/compiler/rustc_feature/src/active.rs:379: no tracking issue for feature doc_html_in_header
some tidy checks failed

@jsha
Copy link
Contributor

jsha commented Jun 3, 2022

something like KaTeX wants to be injected for only specific crates

Can you say more about this? Does KaTeX produce an error if loaded on crates that don't need it? Or is the concern about performance impact of loading a large, unneeded JS file?

What about injecting a small inline JS snippet that checks if the page being loaded needs KaTeX (for instance, by a list of known crates needing KaTeX, or by searching for documentation that needs it), and conditionally loads the main KaTeX script?

@CAD97
Copy link
Contributor Author

CAD97 commented Jun 3, 2022

There's three reasons to prefer loading KaTeX per crate rather than globally:

  • Incompatible choices for handling rendering. Extant you have snippets for writing inline math as $x+y$, $`x+y`$, and `$x+y$`, all of which have different tradeoffs w.r.t. writing, previewing, and surviving the markdown processor.
  • Injecting auto math delimiter detection into a page not written with it in mind can treat spans incorrectly as math; the canonical example is "item A costs $5 and item B costs $10;" recognizers have gotten better at using whitespace to avoid false matches, but they still exist.
  • Providing it at a crate level means that cargo doc just works, and also importantly, it works when your crate is a dependency rather than a root crate.

@DanielJoyce
Copy link

I would like this too. Currently it's a bit of gotcha to use cargo rustdoc for one thing and cargo doc for another, or setting a flag and having it apply to every crate

I want to add mermaid js support.

@CAD97
Copy link
Contributor Author

CAD97 commented Jun 22, 2022

(triaging my open PRs)

Note: I'm effectively blocked from making further compiler changes by an MSVC linking bug until Visual Studio 17.2.5 / 17.3 Preview 3 are released or I give in and tell config.toml to build LLVM locally.

The big question for the T-rustdoc reviewer: Is html injection for-just-this-crate something we want? If so, is an attribute an acceptable way to do it, or should I pursue adding keys in a [lib.doc] section of the cargo manifest to control just-this-crate configuration?

@jsha
Copy link
Contributor

jsha commented Jun 27, 2022

I started https://rust-lang.zulipchat.com/#narrow/stream/266220-rustdoc/topic/.23!.5Bdoc.28html_in_header.29.5D for discussion with the rest of the rustdoc team.

By the way, since it hasn't actually been mentioned in this thread, are you aware of the possibility to set args on a per-crate basis with respect to docs.rs?

[package.metadata.docs.rs]
rustdoc-args = ["--html-in-header", "...]

Example: https://docs.rs/crate/pwnies/latest

This isn't a perfect solution, since it doesn't cover local docs, but if your primary goal is publishing on docs.rs, it might be good enough for now.

@CAD97
Copy link
Contributor Author

CAD97 commented Jun 27, 2022

Yes, and I'm the publisher of https://docs.rs/katex-doc/latest/katex_doc/, another demo of the pwnies technique.

The goal of this is to provide the support for a cargo doc to apply the header html to just one crate.

@JohnCSimon
Copy link
Member

Ping from triage:
@CAD97 - can you please address the build failure?

@Dylan-DPC Dylan-DPC removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 24, 2022
@Dylan-DPC Dylan-DPC added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 12, 2022
@CAD97
Copy link
Contributor Author

CAD97 commented Aug 13, 2022

Closing in favor of pursuing a cargo solution for passing rustdoc flags. I don't have the bandwidth to see this through atm, unfortunately.

@CAD97 CAD97 closed this Aug 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet