-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Fix links pointing to std_detect #96519
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
cc @rust-lang/rustdoc |
This comment has been minimized.
This comment has been minimized.
f3ce412
to
e1a60e6
Compare
.arg(&builder.src.join("src/doc/index.md")); | ||
.arg(&builder.src.join("src/doc/index.md")) | ||
.arg("--extern-html-root-url") | ||
.arg("std_detect=https://docs.rs/std_detect/0.1.5/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Version will eventually be changed, and this can lead to some hard to detect errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is exactly why I specify a version: otherwise older std docs version will always point to the latest std_detect
version and that will very likely be completely wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or we can check version of crate via metadata and fail if it not equal hardcoded one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can extract the version from the libstd Cargo.toml
file I guess...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cargo_metadata
crate should work, i guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is std_detect not added to the list of crates we document?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea. Should it be part of the crates documented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe - it seems like a likely easy and correct way to expose it's documentation/source. There's a policy question of whether we should do that - it would probably be a reasonable stance that it's not part of the stable surface area so showing search results (for example) from it isn't appropriate. Perhaps rustdoc shouldn't render the src link in this case.
But I think this PR is almost certainly the wrong approach - if we want to not include it in search results but include links to it, there's probably a way to order the builds and later combine them or something like that to accomplish that.
I'm going to close this PR as I don't think it's the right direction (as noted in my last comment), and if we want to pursue this further my recommendation is to first define what behavior we want, answering these questions:
One alternative to answering these questions might be to import the std_detect crate into libstd proper, which doesn't seem like it should be that hard to me (it's already part of the dependency closure and doesn't receive that much traffic in terms of contributions). I'm not sure why it lives separately right now. |
This PR is one way to fix #96506. Another one would be to extend
html_root_url
attribute to allow it to work like the unstable--html-root-url
option.Any opinion maybe?