-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
RFC: Custom logo/favicon command-line flags #3226
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
ad00a88
to
d4557d7
Compare
Wouldn't it make more sense to do it via a configuration file (or an attribute along the lines of Doing it via command-line flags would needlessly add incentive to wrap otherwise perfectly good built-in Cargo commands in something like (Doing it via command-line flags also reminds me of how fragile and out-of-band Sphinx's API documentation generation for Python is, since it was developed to replace an existing "maintain Python 2.x's manual in LaTeX" workflow rather than a more JavaDoc/Doxygen/rustdoc-esque approach.) |
Thanks. When I wrote my previous comment, the lack of a "Rendered" link left me forgetting to manually go read the RFC. Most of the RFC, I agree with as motivation to have alternatives to I think that the two halves should be discussed separately:
As for the "For projects with many small crates" argument, I'd want to see a proposal for how this is going to be integrated into the existing tooling so it doesn't just wind up as something like "Set (eg. What about if one of these crates is pulled into a project, independent of the rest of the repo, like with Sure, there could be |
Possibly (see "Future possibilities"). Since there are already other similar command-line flags (e.g. the playground URL), this seemed closest to existing practice. Both should be fine for my use case, but I wanted to focus this RFC on the custom logo issue, not a potential config file. A config file might be problematic in some edge cases (e.g. a user needs to change the logo for different crates in the same folder) and there is always the question of defining how paths inside the file are resolved. In any case, one does not necessarily preclude the other (indeed, someone may argue to accept a
This would not solve the second set of issues (see "Motivation" and "Rationale and alternatives"), i.e. that one needs to put the attribute in every single crate and that one may want to do it for
Cargo-built projects should definitely have a better way to set a custom logo too -- in particular for the workspace case to tweak it for all crates at once. One could argue to have this in
Command-line flags and config files have different strengths. Config files are more elegant in the cases where they fit the model one has. Command-line flags, however, are more flexible in general and can also double as a config file. |
However, you are mashing together two issues:
For anyone who fits the common case otherwise, you're effectively forcing people not in your situation to choose which of two options is the lesser of two evils. (Do I use
True, but we don't need to get ahead of ourselves, since implying too much change now can incentivize We already have flawed attributes for this in As I see it, a simple way to implement something like this incrementally could be:
I firmly believe that command-line flags are for two things:
For the common-case use of Rust, only option 1 is typically exposed to the user, with option 2 being hidden behind A big part of the common-case Rust experience comes from recognizing that raw command-line arguments are the raw pointers ( (Plus, it just feels clunkier... like it's moving the core Rust experience closer to what you get when you resort to adding a |
This is fine and useful for many users, but note that it does not solve the second set of issues and suffers from the complexity of how to resolve paths.
This is incorrect. As mentioned above, the playground URL has both an attribute and a flag (or two -- but one does not seem documented).
Can you elaborate? Cargo can take advantage of the new flags or not, depending on how it wants to handle it, but I do not see the relation to the "projects with many small crates" bit.
Yes, this is one of the possibilities I was suggesting above.
Cargo-based builds can use them just as they use any of the other flags. It is interesting to discuss what would be the best solution for Cargo-built projects (e.g. Now, if the |
But, as I said, they're two separate sub-issues that should be solved independently, so that you don't force people who want the fix to accept what they see as a feature regression.
How is it more difficult than reusing whatever machinery That seems like the most intuitive thing to do.
Point taken.
"Cargo can take advantage of the new flags or not, depending on how it wants to handle it" is the problem. This isn't C or C++, where people are used to just throwing in whatever new feature they need without considering the effect on the experience as a whole. I don't want this to become "My problem has been fixed. I wash my hands of it. I'll leave you to figure out how to keep this from encouraging the most common-case situation to get uglier and messier."
That's the problem. What you're asking for is a less overtly problematic analogue to "Here's a patch to implement syscalls for streaming TCP-to-disk file read/write. Because I'm using this in a single-user, airgapped-LAN configuration and I run my software as root, hooking it up to filesystem permission checks is out of scope." You're saying "give me what my niche use-case needs, and I leave it up to others to figure out the rest if they want to avoid regressing the common-case experience". (Which is what would happen if the choice is between "broken path/url semantics that are consistent with the rest of the design" and "fixed path/url semantics that encourage people to use hacky ways to invoke them".) |
No, I am not. I have a need for this use case, which is currently not supported, and I wrote an RFC that would solve the issue with the minimum changes required on the Rust side. There are alternative solutions, which the RFC mentions. That is all there is to it. It is on the
I would not say it is a "major flaw". The use case was probably not considered, specially given how it works (copy-paste of the content) and the name of the attributes (
No, the feature is simply not present. See above.
No one is "forced" to do anything. Existing users remain the same, and those that could have taken advantage of this feature are already using a workaround. If this gets introduced, some users will be able to use the feature now instead of using a workaround. Now, you are concerned about Cargo users using a
I am sorry, but I skip the rest of your comment because I believe how you feel about command-line flags, other builds systems, |
I believe insisting on a placing a high emphasis on building and preserving a coherent experience for people using Cargo as their primary interface to the build tooling is a very significant part of what led Rust to the place it is today, so I'll leave it up to others to decide which one of us is off-topic before saying anything more. If I'm wrong, I'll correct myself. If I'm not, then it's not productive to continue talking at someone who's skipping over my points and I'll leave it to someone with more clout to re-state them. |
Please see my other reply about this.
That is fine, but the point was that you have to specify it. Moreover, in the config file case, it is not even Rust code.
I am unsure what you are thinking about, but C/C++ do not do that. In fact, at times the discussions feel endless. But it does not matter, it is off-topic.
Nobody is "washing their hands", and I request that you stop suggesting otherwise. The RFC got written precisely because I am trying to get this done properly upstream. I could also have ignored this completely and simply use a workaround. In fact, I am starting to think that would have been better. And it is "Request For Comments", not "I want this and only this and I will not entertain anything else so please just do what I say". In fact, I already said that both solutions (config file or flags) would be fine for my use case. Disagreeing with your opinion of tackling here the general problem of config files etc. does not mean others are lazy or washing their hands. |
OK, Fair. I admit I was getting a bit un-civil in my frustration and, in hindsight, I should have, at minimum, addressed that differently. I still think that one of the things that has helped Rust to attract the interest it's receiving is an emphasis on ensuring that the Cargo workflow moves toward a more polished, coherent experience... and I think exposing less footgun-y path handling for favicons and logos in rustdoc without exposing it in a way that can be accessed by invoking a bare I could see your proposal being OK on the condition that it's not API-stabilized and allowed to be used outside of |
Given that relative paths are presumably nearly-always broken in the existing URL attributes, does it seem viable to change the meaning of a relative path in those attributes to mean "copy this file in", similar to the CLI flags proposed here? In general, I at least would prefer to avoid rustdoc (and rustc) growing many flags that are intended to be close mappings onto crate-level attributes, and would love to see -Zcrate-attr stabilized instead of that -- I imagine a compiler MCP proposing its stabilization (perhaps initially limited to attributes that don't need more than key/value pairs) may be a good idea there. I don't think it makes sense to block adding this feature on support for Cargo to pass these flags down or rustdoc configuration files -- those seem like potentially reasonable enhancements, but are also addable after the fact, right? |
My two main concerns there are:
|
Definitely, if stabilization of In fact, that is what I initially tried before I noticed that in sub-folders the path was not being adjusted. So I think it is reasonable users could expect that.
This is what I would have thought, yeah. Though it is also true that, if we get config files eventually, the flags could be redundant. |
No worries, all good! |
The static files placement by `rustdoc` changed in Rust 1.67.0 [1], but the custom code we have to replace the logo in the generated HTML files did not get updated. Thus update it to have the Linux logo again in the output. Hopefully `rustdoc` will eventually support a custom logo from a local file [2], so that we do not need to maintain this hack on our side. Link: rust-lang/rust#101702 [1] Link: rust-lang/rfcs#3226 [2] Fixes: 3ed03f4 ("rust: upgrade to Rust 1.68.2") Cc: stable@vger.kernel.org Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
The static files placement by `rustdoc` changed in Rust 1.67.0 [1], but the custom code we have to replace the logo in the generated HTML files did not get updated. Thus update it to have the Linux logo again in the output. Hopefully `rustdoc` will eventually support a custom logo from a local file [2], so that we do not need to maintain this hack on our side. Link: rust-lang/rust#101702 [1] Link: rust-lang/rfcs#3226 [2] Fixes: 3ed03f4 ("rust: upgrade to Rust 1.68.2") Cc: stable@vger.kernel.org Tested-by: Benno Lossin <benno.lossin@proton.me> Link: https://lore.kernel.org/r/20231018155527.1015059-1-ojeda@kernel.org Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
The static files placement by `rustdoc` changed in Rust 1.67.0 [1], but the custom code we have to replace the logo in the generated HTML files did not get updated. Thus update it to have the Linux logo again in the output. Hopefully `rustdoc` will eventually support a custom logo from a local file [2], so that we do not need to maintain this hack on our side. Link: rust-lang/rust#101702 [1] Link: rust-lang/rfcs#3226 [2] Fixes: 3ed03f4 ("rust: upgrade to Rust 1.68.2") Cc: stable@vger.kernel.org Tested-by: Benno Lossin <benno.lossin@proton.me> Reviewed-by: Andreas Hindborg <a.hindborg@samsung.com> Link: https://lore.kernel.org/r/20231018155527.1015059-1-ojeda@kernel.org Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
[ Upstream commit cfd9672 ] The static files placement by `rustdoc` changed in Rust 1.67.0 [1], but the custom code we have to replace the logo in the generated HTML files did not get updated. Thus update it to have the Linux logo again in the output. Hopefully `rustdoc` will eventually support a custom logo from a local file [2], so that we do not need to maintain this hack on our side. Link: rust-lang/rust#101702 [1] Link: rust-lang/rfcs#3226 [2] Fixes: 3ed03f4 ("rust: upgrade to Rust 1.68.2") Cc: stable@vger.kernel.org Tested-by: Benno Lossin <benno.lossin@proton.me> Reviewed-by: Andreas Hindborg <a.hindborg@samsung.com> Link: https://lore.kernel.org/r/20231018155527.1015059-1-ojeda@kernel.org Signed-off-by: Miguel Ojeda <ojeda@kernel.org> Signed-off-by: Sasha Levin <sashal@kernel.org>
[ Upstream commit cfd9672 ] The static files placement by `rustdoc` changed in Rust 1.67.0 [1], but the custom code we have to replace the logo in the generated HTML files did not get updated. Thus update it to have the Linux logo again in the output. Hopefully `rustdoc` will eventually support a custom logo from a local file [2], so that we do not need to maintain this hack on our side. Link: rust-lang/rust#101702 [1] Link: rust-lang/rfcs#3226 [2] Fixes: 3ed03f4 ("rust: upgrade to Rust 1.68.2") Cc: stable@vger.kernel.org Tested-by: Benno Lossin <benno.lossin@proton.me> Reviewed-by: Andreas Hindborg <a.hindborg@samsung.com> Link: https://lore.kernel.org/r/20231018155527.1015059-1-ojeda@kernel.org Signed-off-by: Miguel Ojeda <ojeda@kernel.org> Signed-off-by: Sasha Levin <sashal@kernel.org>
Recent changes to the HTML output (rust-lang/rust#115948) broke our workaround for this. It has made it harder to workaround too, since now the Rust logo is not there for non-Rust crates, which in turn means we lose the relative paths computed for it. It would be nice to have a supported way to bundle a logo file and (most importantly) generate the relative paths to it (even if it is not flags like in this RFC). It would let us remove our workaround and thus would avoid future changes on our side. It will be needed to establish a minimum Rust version in the kernel, eventually. |
Ah, I see for some reason there was no link to the other RFC that overlaps this one: #3397, by changing |
Thanks @Nemo157! Yeah, in that RFC, this:
would seem to solve our use case -- if I understand correctly, it is essentially what @Mark-Simulacrum suggested above. It would still require us to use Perhaps that part of the RFC could be split so that it is easier to stabilize? Then #3397 would be easier to get in too. Otherwise, @ssokolow's suggestion of adding |
Add new command-line flags to
rustdoc
to specify a file to be used as the logo and/or favicon. The file(s) will be copied to the generated documentation directory and relative paths will be computed to point to the resource from within the generated HTML.