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 CLI option to setup favicon #60002

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/librustdoc/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,8 @@ pub struct RenderOptions {
pub generate_search_filter: bool,
/// Option (disabled by default) to generate files used by RLS and some other tools.
pub generate_redirect_pages: bool,
/// An optional path to to the favicon file to use.
pub favicon: Option<PathBuf>,
}

impl Options {
Expand Down Expand Up @@ -429,6 +431,14 @@ impl Options {
}
});

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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huuuum... We have to differentiate two kind of paths: URL and locals. What do you want to do in both cases?

return Err(1);
}
}

let show_coverage = matches.opt_present("show-coverage");
let document_private = matches.opt_present("document-private-items");

Expand Down Expand Up @@ -508,6 +518,7 @@ impl Options {
markdown_playground_url,
generate_search_filter,
generate_redirect_pages,
favicon,
}
})
}
Expand Down
7 changes: 6 additions & 1 deletion src/librustdoc/html/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use crate::html::render::SlashChecker;
pub struct Layout {
pub logo: String,
pub favicon: String,
pub favicon_path: Option<PathBuf>,
pub external_html: ExternalHtml,
pub krate: String,
}
Expand Down Expand Up @@ -197,7 +198,11 @@ pub fn render<T: fmt::Display, S: fmt::Display>(
title = page.title,
description = page.description,
keywords = page.keywords,
favicon = if layout.favicon.is_empty() {
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because one generates a full path whereas the other generates a relative one.

} else if layout.favicon.is_empty() {
format!(r#"<link rel="shortcut icon" href="{static_root_path}favicon{suffix}.ico">"#,
static_root_path=static_root_path,
suffix=page.resource_suffix)
Expand Down
22 changes: 19 additions & 3 deletions src/librustdoc/html/render.rs
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,7 @@ pub fn run(mut krate: clean::Crate,
static_root_path,
generate_search_filter,
generate_redirect_pages,
favicon,
..
} = options;

Expand All @@ -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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.unwrap_or_else(String::new),
favicon_path: favicon,
external_html,
krate: krate.name.clone(),
},
Expand All @@ -572,7 +576,16 @@ pub fn run(mut krate: clean::Crate,
for attr in attrs.lists("doc") {
match (attr.name_or_empty().get(), attr.value_str()) {
("html_favicon_url", Some(s)) => {
scx.layout.favicon = s.to_string();
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 \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're already iterating through libsyntax's attributes here, why not use its span in this warning?

`html_favicon_url` attribute")
.emit();
}
} else {
scx.layout.favicon = s;
}
}
("html_logo_url", Some(s)) => {
scx.layout.logo = s.to_string();
Expand Down Expand Up @@ -816,7 +829,10 @@ fn write_shared(
write(cx.dst.join(&format!("rust-logo{}.png", cx.shared.resource_suffix)),
static_files::RUST_LOGO)?;
}
if (*cx.shared).layout.favicon.is_empty() {
if let Some(ref favicon) = (*cx.shared).layout.favicon_path {
let content = try_err!(fs::read(&favicon), &favicon);
write(cx.dst.join(&(*cx.shared).layout.favicon), &content)?;
} else if (*cx.shared).layout.favicon.is_empty() {
write(cx.dst.join(&format!("favicon{}.ico", cx.shared.resource_suffix)),
static_files::RUST_FAVICON)?;
}
Expand Down
6 changes: 6 additions & 0 deletions src/librustdoc/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,12 @@ fn opts() -> Vec<RustcOptGroup> {
"show-coverage",
"calculate percentage of public items with documentation")
}),
unstable("favicon-path", |o| {
o.optopt("",
"favicon-path",
"Path string to the favicon file to be used for this crate",
"PATH")
}),
]
}

Expand Down
6 changes: 6 additions & 0 deletions src/test/rustdoc/favicon-path.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// compile-flags:-Z unstable-options --favicon-path ./src/librustdoc/html/static/favicon.ico
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I'll just create or use another file. :)


#![crate_name = "foo"]

// @has foo/fn.foo.html '//link/@href' '../favicon-foo.ico'
pub fn foo() {}