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

Use relative path for local links to primitives #74077

Merged
merged 9 commits into from
Jul 10, 2020

Conversation

sethp
Copy link
Contributor

@sethp sethp commented Jul 6, 2020

Else, links to char::foo would point into /path/to/src/libcore/std/primitive.char.html#method.foo.

Split out from #73804.

Else, links to `char::foo` would point into `/path/to/src/libcore/std/primitive.char.html#method.foo`.

Split out from rust-lang#73804.
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ollie27 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 6, 2020
@sethp
Copy link
Contributor Author

sethp commented Jul 6, 2020

cc @jyn514

@jyn514
Copy link
Member

jyn514 commented Jul 6, 2020

Note that this is not special to libcore, this is because x.py passes --index-page which hits the ExternalLocation::Local code path.

Please add some test cases in src/test/rustdoc. In this case you will also need to pass --index-page: // compile-flags: --index-page .. Not sure exactly which path you need to pass, you might have to experiment.

@sethp sethp changed the title Use relative path for local links to primitives in libcore Use relative path for local links to primitives Jul 6, 2020
@sethp
Copy link
Contributor Author

sethp commented Jul 6, 2020

Ah, gotcha, thanks for the pointers. I'm still in the "using words that I've seen once or twice in totally wrong ways" phase of learning! I'll take a look into adding a test.

A question I had about this is whether it'll break in a non-rustc context – for example, would someone try to link to char from their crate's rustdoc with an --index-page set?

@jyn514
Copy link
Member

jyn514 commented Jul 6, 2020

Ah hmm good point. With that in mind I don't think this is the right solution - note it appends std/primitive.char.html below: https://github.com/rust-lang/rust/pull/74077/files#diff-6a301d597807ee441a41e7237800563dR666. Instead it looks like the src location is wrong somehow - it should be /path/to/build/x86_64-unknown-linux-gnu/doc but instead it's /path/to/doc/src/libcore.

I think this is actually a bug in x.py itself:

$ x.py doc --stage 1 src/libstd -v
running: "/home/joshua/src/rust/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "rustdoc" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "8" "--release" "--features" "panic-unwind backtrace compiler-builtins-c" "--manifest-path" "/home/joshua/src/rust/src/libtest/Cargo.toml" "-p" "test" "--" "--markdown-css" "rust.css" "--markdown-no-toc" "--generate-redirect-pages" "-Z" "unstable-options" "--resource-suffix" "1.46.0" "--index-page" "/home/joshua/src/rust/src/doc/index.md"

Note that --index-page points to src/doc instead of the output directory.

cc @Mark-Simulacrum @GuillaumeGomez , am I missing something?

@Mark-Simulacrum
Copy link
Member

Not sure what the right value is, not knowing what the flag does -- I can't imagine it making a difference whether we point to the build directory or source, though, seeing as neither will be in the same place when the documentation is read in the web browser. But maybe I'm misinterpreting? I don't really know.

It's been like that since fb2813b -- this seems to be the relevant line:

.arg(&builder.src.join("src/doc/index.md"))

@jyn514
Copy link
Member

jyn514 commented Jul 6, 2020

Ah yeah you're right, --index-page isn't related. I'm not sure what's going on then, sorry for the noise.

@jyn514
Copy link
Member

jyn514 commented Jul 6, 2020

@sethp do you mind if I take over this fix? It's not clear to me what/where the bug is so someone is going have to spend a fair bit of time debugging.

@sethp
Copy link
Contributor Author

sethp commented Jul 6, 2020

I think that would be fine: I've spent a fair bit of time adding log statements already and not getting very far 😆

I will say – the more I look at this, the more I suspect it is a valid fix, because I'm not sure what other context would produce a Local destination for primitives:

// See if there's documentation generated into the local directory
let local_location = dst.join(&e.name);
if local_location.is_dir() {
return Local;
}

I think I read that as "we're local just in case there's a directory for it in the output path we're emitting to," which seems like exactly the right time to be giving a relative link like this? I haven't come up with a test case yet, everything I try generating docs for src/test/rustdoc/intra-link-prim-methods.rs ends up emitting links to https://doc.rust-lang.org/nightly/....

@jyn514
Copy link
Member

jyn514 commented Jul 6, 2020

Ah hmm interesting. I'm not super familiar with this part of the codebase, if you think it's the right approach let's

r? @GuillaumeGomez

@GuillaumeGomez
Copy link
Member

Looks good to me as well but please add a test.

@sethp
Copy link
Contributor Author

sethp commented Jul 6, 2020

It's a reasonable request that I'm not quite sure how to honor – in the few hours I've spent trying to activate that code path, the only time I can successfully hit it is when documenting libstd, and I'm not sure how to simulate that condition in a test. For what it's worth, I think ee3a0f8 adds an explicit guard to ensure this can only be hit when documenting a create with a local reference to an external libcore (i.e., libstd and friends).

I'd be glad for some guidance on how to test this case. Ultimately the point of all of this is to remove the exclusion for broken links in the standard library's String pages, which will indirectly check this code path, but not in a very obvious way.

@GuillaumeGomez
Copy link
Member

In this case, the best would be to add a check on the generated HTML so take a look at src/tests/rustdoc. If the link is found then it means it's working as expected. :)

@jyn514
Copy link
Member

jyn514 commented Jul 6, 2020

@GuillaumeGomez their point is that this doesn't happen in stand alone cases, only when documenting core. So they can't test it in src/test/rustdoc.

@sethp - have you tried defining #[lang_item ="char"] in those tests?

@jyn514
Copy link
Member

jyn514 commented Jul 6, 2020

ee3a0f8 adds an explicit guard to ensure this can only be hit when documenting a create with a local reference to an external libcore (i.e., libstd and friends)

I don't think this is the right approach, it breaks docs for any crate that uses #[no_core] then defines char.

@sethp
Copy link
Contributor Author

sethp commented Jul 6, 2020

I don't think this is the right approach, it breaks docs for any crate that uses #[no_core] then defines char.

Oh, interesting – perhaps this is the test case I should try to add? Is that what #[lang_item = "char"] signifies?

@jyn514
Copy link
Member

jyn514 commented Jul 6, 2020

Yes, exactly. I'd be interested if that reproduces the bug.

sethp added 2 commits July 6, 2020 19:35
They both produce less-than-desirable output (links going to docs.rust-lang.org), but I haven't figured out yet how to assert about them properly.
@sethp
Copy link
Contributor Author

sethp commented Jul 7, 2020

Well, not quite. 33a5d00 does show us two new behaviors when I run it: both end up linking out to rust-docs for the primitive char despite the local or nearly local definition.

Some questions I have to look into tomorrow:

  1. I'm not quite sure how to assert things about the output html – would I be looking into adding a new compilertest header directive, or is there something I missed?
  2. Why does the output from neither test seem to include the char lang item documentation, and does that matter?
  3. When I run the test for intra-links-prim-methods-external-core.rs then I get a "Local" reference in the extern_locations cache (hooray), but I'm still seeing links generated to rust-lang.org. This appears to be happening because the CrateNum being passed into the links function isn't lining up the same way as when documenting libstd and/or libcore, so we're still taking the None path rather than the Local path.

@jyn514
Copy link
Member

jyn514 commented Jul 7, 2020

I'm not quite sure how to assert things about the output html – would I be looking into adding a new compilertest header directive, or is there something I missed?

Yes, look for @has in the other tests for examples. There's also some sparse documentation in https://rustc-dev-guide.rust-lang.org/rustdoc-internals.html#dotting-is-and-crossing-ts.

Why does the output from neither test seem to include the char lang item documentation, and does that matter?

Good question, I'm not sure. I'd be interested what happens after the change I suggest below though.

When I run the test for intra-links-prim-methods-external-core.rs then I get a "Local" reference in the extern_locations cache (hooray), but I'm still seeing links generated to rust-lang.org. This appears to be happening because the CrateNum being passed into the links function isn't lining up the same way as when documenting libstd and/or libcore, so we're still taking the None path rather than the Local path.

Isn't this because you special case "core" in your change? Try removing that special casing and see if it works after.

@sethp
Copy link
Contributor Author

sethp commented Jul 7, 2020

Yes, look for @has in the other tests for examples.

Ah, thank you, I will take a look at that!

Isn't this because you special case "core" in your change?

I thought so too, but even after reverting ee3a0f8 I see the same behavior. With a little bit of print-based debugging what I've found is that the extern_locations cache looks like this:

{crate1: ("my_core", "/Users/seth/Code/Public/rust/src/test/rustdoc/auxiliary", Local)}

But the krate argument to links is always crate0 in my test scenarios, so the get returns None and the code path in question here isn't hit. Contra when I run the standard library build, the cache very similar (+/- a lot more stuff):

{..., crate1: ("core", "/Users/seth/Code/Public/rust/src/libcore", Local), ...}

However, in that case we only ever seem to pass in a crate1 argument, so the get returns the ExternalLocation and we go down that arm of the match.

I'm not too sure what any of that means, yet. I am starting to wonder if it wouldn't make sense to consider the lang_item documentation a separate bug, though, since that doesn't seem to be working with or without this change.

@jyn514
Copy link
Member

jyn514 commented Jul 7, 2020

For the auxiliary case, try also adding // build-aux-docs. Otherwise rustdoc won't have anything to link to (by default the tests use --no-deps).

Does the same issue show up when you have a Lang item in the crate being documented? That's surprising to me.

@sethp
Copy link
Contributor Author

sethp commented Jul 7, 2020

Same! Neither of the two tests I added are doing what I expected:

  1. intra-link-prim-methods-local.rs has #[no_core] and local lang_items, but when I build the docs I see 1) no mention of the char lang_item that's part of the crate, and 2) links out to rust-lang.org
  2. intra-link-prim-methods-external-core.rs uses build-aux-docs but the produced documentation is similar to the above (no char docs, Remote links)

However, both complain when I omit the len_utf8 method being linked to, so they do at least seem to be resolving against the expected places.

For the two of these tests that have a local `char` to link to, this behavior isn't what's expected, but is what's happening presently.
@Manishearth
Copy link
Member

@bors r-

#74151 (comment)

@Manishearth Manishearth closed this Jul 8, 2020
@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 8, 2020
@Manishearth Manishearth reopened this Jul 8, 2020
@Manishearth
Copy link
Member

bors r-

There don't seem to be any other compiletests that are 1) building a standalone "no_core" create and then 2) trying to link against it. There seems to be a platform-specific limitation in doing so:

```
2020-07-08T16:07:42.9419409Z   = note:    Creating library D:\a\rust\rust\build\i686-pc-windows-msvc\test\rustdoc\intra-link-prim-methods-external-core\auxiliary\my_core.dll.lib and object D:\a\rust\rust\build\i686-pc-windows-msvc\test\rustdoc\intra-link-prim-methods-external-core\auxiliary\my_core.dll.exp
2020-07-08T16:07:42.9419810Z           LINK : error LNK2001: unresolved external symbol __DllMainCRTStartup@12
2020-07-08T16:07:42.9420032Z           D:\a\rust\rust\build\i686-pc-windows-msvc\test\rustdoc\intra-link-prim-methods-external-core\auxiliary\my_core.dll : fatal error LNK1120: 1 unresolved externals
```

Possibly this could be resolved by adding a `__DllMainCRTStartup` or `__DllMainCRTStartup@12` symbol in an architecture- and platform-specific way.
@sethp
Copy link
Contributor Author

sethp commented Jul 9, 2020

I don't think I have access to a Windows system to test on, but there seems to be a few other no_core rustdoc tests that run fine on Windows as long as they don't try to link against an external no_core crate. It looks like there's a few details to handling that one correctly, so I marked it as ignore-windows for now.

@Manishearth would you be willing to try again?

@Manishearth
Copy link
Member

@bors r=jyn514

@bors
Copy link
Contributor

bors commented Jul 9, 2020

📌 Commit 56b6b44 has been approved by jyn514

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 9, 2020
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 9, 2020
…link, r=jyn514

Use relative path for local links to primitives

Else, links to `char::foo` would point into `/path/to/src/libcore/std/primitive.char.html#method.foo`.

Split out from rust-lang#73804.
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 9, 2020
…link, r=jyn514

Use relative path for local links to primitives

Else, links to `char::foo` would point into `/path/to/src/libcore/std/primitive.char.html#method.foo`.

Split out from rust-lang#73804.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 9, 2020
…arth

Rollup of 14 pull requests

Successful merges:

 - rust-lang#73292 (Fixing broken link for the Eq trait)
 - rust-lang#73791 (Allow for parentheses after macro intra-doc-links)
 - rust-lang#74070 ( Use for<'tcx> fn pointers in Providers, instead of having Providers<'tcx>.)
 - rust-lang#74077 (Use relative path for local links to primitives)
 - rust-lang#74079 (Eliminate confusing "globals" terminology.)
 - rust-lang#74107 (Hide `&mut self` methods from Deref in sidebar if there are no `DerefMut` impl for the type.)
 - rust-lang#74136 (Fix broken link in rustdocdoc)
 - rust-lang#74137 (Update cargo)
 - rust-lang#74142 (Liballoc use vec instead of vector)
 - rust-lang#74143 (Try remove unneeded ToString import in liballoc slice)
 - rust-lang#74146 (update miri)
 - rust-lang#74150 (Avoid "blacklist")
 - rust-lang#74184 (Add docs for intra-doc-links)
 - rust-lang#74188 (Tweak `::` -> `:` typo heuristic and reduce verbosity)

Failed merges:

 - rust-lang#74122 (Start-up clean-up)
 - rust-lang#74127 (Avoid "whitelist")

r? @ghost
@bors bors merged commit 07301e3 into rust-lang:master Jul 10, 2020
@sethp sethp deleted the docs/fix-intra-doc-primitive-link branch July 10, 2020 04:03
@jyn514
Copy link
Member

jyn514 commented Jul 18, 2020

@sethp I finally figured out why you couldn't reproduce the issue: #73423 (comment)

@sethp
Copy link
Contributor Author

sethp commented Jul 18, 2020

Oh, interesting – it looks like that's definitely part of it. With a #[doc(primitive = "char")] added to my_core :

diff --git a/src/test/rustdoc/auxiliary/my-core.rs b/src/test/rustdoc/auxiliary/my-core.rs
index 54e986be9ec..208e9ca26f4 100644
--- a/src/test/rustdoc/auxiliary/my-core.rs
+++ b/src/test/rustdoc/auxiliary/my-core.rs
@@ -17,3 +17,8 @@ pub trait Clone: Sized {}

 #[lang = "copy"]
 pub trait Copy: Clone {}
+
+
+#[doc(primitive = "char")]
+/// Docs for the primitive char type
+mod prim_char {}

I do see that my-core.rs generates rustdoc for my len_utf8 method, so there's at least a possible link target now.

Unfortunately, it still generates links to nightly. I think the other detail here is that the rustdoc invocation for libcore is actually cargo rustdoc -p core against the src/libtest/Cargo.toml manifest – maybe that's putting the docs "together" in a way my separate rustdoc invocations aren't?

@jyn514
Copy link
Member

jyn514 commented Jul 18, 2020

Try it within a single crate instead of across crates? Or rebase over master now that #73101 is merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.