Add CLI option to setup favicon #60002
Conversation
rust-highfive
added
the
S-waiting-on-review
jonas-schievink
added
the
T-rustdoc
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@QuietMisdreavus: BTW, I wanted to ask: should I add the resource suffix in here as well? Also: should I keep the original filename? |
There was a problem hiding this comment.
This only kind of solves the problem that docs.rs is having. I have one suggestion for a better solution in the review comments, but here's one more: Instead of taking the plain string or a file path, why not have an option to disable the default favicon altogether? That way, we don't have to worry about paths at all.
(The question remains of what to do with #![doc(html_favicon_url)] in this situation, though. My instinct is to let the crate override the CLI, but then i feel like we'd have to provide an option to really really disable the favicon in that case. Something like --favicon default,no-default,disable? That way we could keep the custom favicon that crates like Rocket and Iron use.)
| favicon = if layout.favicon_path.is_some() { | ||
| format!(r#"<link rel="shortcut icon" href="{static_root_path}{path}">"#, | ||
| static_root_path=static_root_path, | ||
| path=layout.favicon) |
There was a problem hiding this comment.
This doesn't seem like a useful addition to the layout code. If we're going to use the favicon string and the static_root_path anyway, why not just update that path with the right path?
(If we make the favicon path a plain string instead of an actual file path, we can keep this distinction, though, as long as we make sure to not use static_root_path in that situation.)
There was a problem hiding this comment.
Because one generates a full path whereas the other generates a relative one.
| let favicon = matches.opt_str("favicon-path").map(|s| PathBuf::from(&s)); | ||
| if let Some(ref favicon) = favicon { | ||
| if !favicon.is_file() { | ||
| diag.struct_err("option `--favicon-path` argument must be a file").emit(); |
There was a problem hiding this comment.
Since this change was suggested for docs.rs, i should note that this won't work over there. The favicon file isn't available to the docs.rs builder when it runs rustdoc (in fact it's not even in the repository), so it can't point to a filename for it to try to load.
What would be ideal is if we could take an arbitrary string here, which would then be pasted in for the favicon path, much like the #![doc(html_favicon_url)] attribute we're mimicking. That way docs.rs can pass "/favicon.ico" which would link to the correct favicon without having to make sure it can be loaded by the builder.
There was a problem hiding this comment.
Huuuum... We have to differentiate two kind of paths: URL and locals. What do you want to do in both cases?
| @@ -545,7 +546,10 @@ pub fn run(mut krate: clean::Crate, | |||
| issue_tracker_base_url: None, | |||
| layout: layout::Layout { | |||
| logo: String::new(), | |||
| favicon: String::new(), | |||
| favicon: favicon.as_ref() | |||
| .map(|_| format!("favicon-{}.ico", krate.name)) | |||
There was a problem hiding this comment.
Modifying the favicon filename/URL in the doc output will not work for docs.rs, since it will break the caching that is already done for the /favicon.ico URL.
| let s = s.to_string(); | ||
| if scx.layout.favicon_path.is_some() { | ||
| if !s.is_empty() { | ||
| diag.struct_warn("`favicon-path` option has been passed, ignoring \ |
There was a problem hiding this comment.
Since we're already iterating through libsyntax's attributes here, why not use its span in this warning?
| @@ -0,0 +1,6 @@ | |||
| // compile-flags:-Z unstable-options --favicon-path ./src/librustdoc/html/static/favicon.ico | |||
There was a problem hiding this comment.
Loading the file like this won't work on travis, since x.py is called from a different directory there - this path isn't valid.
There was a problem hiding this comment.
Well, I'll just create or use another file. :)
|
☔ The latest upstream changes (presumably #60630) made this pull request unmergeable. Please resolve the merge conflicts. |
Dylan-DPC-zz
added
S-waiting-on-author
|
ping from triage @GuillaumeGomez you need to address the changes requested in the review |
|
Indeed! I'll come back to it soon. |
|
ping from triage @GuillaumeGomez any updates on this? |
|
Still need to come back to it. :) |
|
@rustbot modify labels to -S-waiting-on-author and +S-inactive-closed Hi @GuillaumeGomez - ping from Triage, closing this due to inactivity. Please re-open before pushing changes. Thanks for the PR. |
rustbot
added
S-inactive
Fixes #59948.
r? @QuietMisdreavus