[rustdoc] Remove old style files #56577

GuillaumeGomez · 2018-12-06T23:01:49Z

QuietMisdreavus · 2018-12-06T23:06:55Z

Based on the discussion in #35705 i really doubt we should be getting rid of the redirect pages. cc @rust-lang/rustdoc

ollie27 · 2018-12-06T23:59:47Z

I don't think there's much reason to remove the macro redirects as I don't think many crates have that many macros and we don't want to break existing links.

The other redirects aren't old, they're a new style added by #35236. To remove them we would need to update anything generating links of that style like https://github.com/rust-dev-tools/rls-analysis/blob/1e0812e8595b16c086ce9a400b9a50adaf82914b/src/lib.rs#L504-L561. cc. @nrc

GuillaumeGomez · 2018-12-07T09:00:46Z

They're both useless and take space for nothing. :-/

retep998 · 2018-12-07T09:48:02Z

I greatly appreciate any reduction in the number of files produced by rustdoc. Killing off redirects will substantially improve doc generation time for winapi, which is currently a major blocker for anyone trying to have offline documentation of their dependencies on windows.

nrc · 2018-12-07T14:09:07Z

They're both useless and take space for nothing

This is not true, they allow linking to rustdoc from tools outside the compiler. Some ways to remove them:

only generate them with a flag (which would be set by the Rust build system and docs.rs, but not when building locally)
run rustdoc using its own web server so we can do a programmatic redirect rather than html ones
change the existing URLs to the redirect ones
use disambiguation pages and no namespaces by default (this would still require some redirects, but we could remove the tools ones)

ollie27 · 2018-12-07T17:22:39Z

@nrc: what tools are using these redirects and how hard would it be for them to generate links using the current URL scheme instead?

nrc · 2018-12-07T20:10:06Z

what tools are using these redirects

RLS, cargo-src, maybe some other stuff

how hard would it be for them to generate links using the current URL scheme instead?

impossible. Basically nobody knows about the struct or whatever part of the URL so its impossible to synthesize. That's why the redirects exist

GuillaumeGomez · 2018-12-08T15:52:14Z

What about I add a flag that'll be disabled by default in order to have those files generated?

nrc · 2018-12-09T21:54:33Z

What about I add a flag that'll be disabled by default in order to have those files generated?

As long as the files are there on the hosted rustdoc and docs.rs instances, that seems fine

GuillaumeGomez · 2018-12-09T22:07:58Z

Ok, I'll do it tomorrow.

GuillaumeGomez · 2018-12-11T23:35:43Z

I added the option (as stable, if that's an issue, I'll move it as unstable, but considering it allows to generate files that were already here...).

retep998 · 2018-12-12T02:00:15Z

Would it be better to have "redirect" in the name of the flag?

GuillaumeGomez · 2018-12-12T13:39:22Z

I'm open to changing the name of the flag. No preference on this side.

QuietMisdreavus

Left some suggestions.

We'll need to remember to coordinate this with docs.rs when this is merged.

src/librustdoc/lib.rs

QuietMisdreavus · 2018-12-12T16:35:27Z

src/test/rustdoc/issue-19190.rs

@@ -23,9 +23,6 @@ impl Deref for Bar {
    fn deref(&self) -> &Foo { loop {} }
 }

-// @has issue_19190/Bar.t.html


You should either include a test that uses the flag or add it to existing tests that check for these pages.

// compile-flags:--generate-redirect-pages

QuietMisdreavus · 2018-12-12T16:36:23Z

src/test/rustdoc/issue-19190.rs

 // @has issue_19190/struct.Bar.html
-// @has - '//*[@id="foo.v"]' 'fn foo(&self)'


This PR doesn't delete the legacy HTML IDs, does it? Why did you remove it from all these tests?

I went a bit too strongly on the tests haha.

QuietMisdreavus · 2018-12-12T16:38:22Z

src/test/rustdoc/old-style-files.rs

+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+#![crate_name = "foo"]


Remember to rename this test file if you change the name of the flag.

GuillaumeGomez · 2018-12-13T20:50:31Z

Updated.

rust-highfive · 2018-12-13T21:09:14Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_time:end:030e7b9c:start=1544734309553568350,finish=1544734368222419390,duration=58668851040
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---

[00:16:10] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:16:11] tidy error: /checkout/src/test/rustdoc/issue-19190.rs: missing trailing newline
none            7.4G     0  7.4G   0% /run/shm
none            100M     0  100M   0% /run/user
none            768M     0  768M   0% /var/ramfs
\; -exec echo travis_fold":"end:crashlog \; || true
\; -exec echo travis_fold":"end:crashlog \; || true
find: `/home/travis/Library/Logs/DiagnosticReports': No such file or directory
travis_time:end:056ad326:start=1544735349503162431,finish=1544735349507862520,duration=4700089
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0027d080
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:03c8ba88
travis_time:start:03c8ba88
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0076a0e8
$ dmesg | grep -i kill

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 @TimNN. (Feature Requests)

QuietMisdreavus · 2018-12-13T21:42:10Z

src/test/rustdoc/issue-35169-2.rs

 // @has - '//*[@id="method.by_explicit_mut_ref"]' 'fn by_explicit_mut_ref(self: &mut Foo)'
-// @!has - '//*[@id="by_explicit_box.v"]' 'fn by_explicit_box(self: Box<Foo>)'


This test still should be checking for the HTML IDs.

QuietMisdreavus · 2018-12-13T21:42:17Z

src/test/rustdoc/issue-35169.rs

 // @!has - '//*[@id="method.by_mut_ref"]' 'fn by_mut_ref(&mut self)'
-// @!has - '//*[@id="by_explicit_mut_ref.v"]' 'fn by_explicit_mut_ref(self: &mut Foo)'


QuietMisdreavus · 2018-12-13T21:43:23Z

src/test/rustdoc/structfields.rs

@@ -8,8 +8,6 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.

-// @has structfields/Foo.t.html
-// @has - struct.Foo.html


Since this one actually checks the content of the redirect pages (and these lines were added in the PR that added the redirect pages in the first place) i feel like this test should also keep them.

QuietMisdreavus · 2018-12-13T21:47:12Z

There's one more place where redirect pages are emitted that you aren't covering in the current PR:

https://github.com/rust-lang/rust/blob/56b0bd5b3c45f8333cc790769a7ce13a8ec3ff82/src/librustdoc/html/render.rs#L2056-L2072

QuietMisdreavus · 2018-12-13T23:12:41Z

I take back my last comment. After looking through the code where that's called, it turns out that those redirects are used by rustdoc itself: When an item is defined privately but re-exported publicly, its "canonical" location is still the private one. So any links that get generated to it point to the "private" location in the hierarchy, which are then redirected to the location in cache().paths. So those redirect pages need to stay, so as to not break rustdoc itself.

My other outstanding review comments still stand though.

QuietMisdreavus

With the last force-push, looks like we're good to go!

QuietMisdreavus · 2018-12-14T22:40:51Z

@bors r+

bors · 2018-12-14T22:40:52Z

📌 Commit 3d339f2 has been approved by QuietMisdreavus

ollie27 · 2018-12-15T00:15:46Z

I don't agree with the macro redirects being controlled by the same flag as the tool redirects. The macro redirects are there for backward compatibility and I'm not aware of any crates with so many macros that their generation is causing problems. If the tool redirects are only to be used on documentation uploaded to doc.rust-lang.org and docs.rs then they should be behind an permanently unstable option.

Also, doesn't stabilising a new command line option require documentation in the rustdoc book and a FCP?

QuietMisdreavus · 2018-12-15T01:17:03Z

@bors r-

I keep forgetting about the process. I'd forgotten that the flag was insta-stable and forgot about needing documentation for it. The suggestion about being perma-unstable is interesting - @nrc do you know if the tool redirects will be needed for local documentation? If they're not, then it's worth never stabilizing it.

bors · 2018-12-23T01:56:58Z

☔ The latest upstream changes (presumably #57063) made this pull request unmergeable. Please resolve the merge conflicts.

Dylan-DPC-zz · 2019-01-07T18:48:38Z

ping from triage @QuietMisdreavus @GuillaumeGomez any updates on this

GuillaumeGomez · 2019-01-08T09:22:49Z

I need to add a few things.

TimNN · 2019-01-22T18:13:53Z

Ping from triage! It looks like this PR hasn't made any progress in a while, so I'm closing it as inactive, per our guidelines. Thanks for your contributions, @GuillaumeGomez, and please feel free to re-open in the future.

…tyle-files, r=ollie27 Rustdoc remove old style files Reopening of rust-lang#56577 (which I can't seem to reopen...). I made the flag unstable so with this change, what was blocking the PR is now gone I assume.

rust-highfive assigned QuietMisdreavus Dec 6, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 6, 2018

Remove rustdoc old style files generation

b3a1039

GuillaumeGomez force-pushed the rustodc-remove-old-style-files branch from a98637c to 270515d Compare December 11, 2018 23:33

QuietMisdreavus reviewed Dec 12, 2018

View reviewed changes

GuillaumeGomez force-pushed the rustodc-remove-old-style-files branch from 270515d to 93ac41f Compare December 13, 2018 20:49

GuillaumeGomez force-pushed the rustodc-remove-old-style-files branch from 93ac41f to 56b0bd5 Compare December 13, 2018 21:12

QuietMisdreavus reviewed Dec 13, 2018

View reviewed changes

GuillaumeGomez force-pushed the rustodc-remove-old-style-files branch from d44f38f to 56b0bd5 Compare December 13, 2018 23:07

Add generate-old-style-files option to rustdoc

3d339f2

GuillaumeGomez force-pushed the rustodc-remove-old-style-files branch from 56b0bd5 to 3d339f2 Compare December 14, 2018 19:51

QuietMisdreavus approved these changes Dec 14, 2018

View reviewed changes

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-review Status: Awaiting review from the assignee but also interested parties. labels Dec 14, 2018

QuietMisdreavus mentioned this pull request Dec 14, 2018

changes required in future nightlies rust-lang/docs.rs#270

Closed

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 Dec 15, 2018

TimNN closed this Jan 22, 2019

TimNN added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 22, 2019

GuillaumeGomez mentioned this pull request Jan 26, 2019

Rustdoc remove old style files #57929

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[rustdoc] Remove old style files #56577

[rustdoc] Remove old style files #56577

GuillaumeGomez commented Dec 6, 2018

QuietMisdreavus commented Dec 6, 2018

ollie27 commented Dec 6, 2018

GuillaumeGomez commented Dec 7, 2018

retep998 commented Dec 7, 2018

nrc commented Dec 7, 2018

ollie27 commented Dec 7, 2018

nrc commented Dec 7, 2018

GuillaumeGomez commented Dec 8, 2018

nrc commented Dec 9, 2018

GuillaumeGomez commented Dec 9, 2018

GuillaumeGomez commented Dec 11, 2018

retep998 commented Dec 12, 2018

GuillaumeGomez commented Dec 12, 2018

QuietMisdreavus left a comment

QuietMisdreavus Dec 12, 2018

QuietMisdreavus Dec 12, 2018

GuillaumeGomez Dec 12, 2018

QuietMisdreavus Dec 12, 2018

GuillaumeGomez commented Dec 13, 2018

rust-highfive commented Dec 13, 2018

QuietMisdreavus Dec 13, 2018

QuietMisdreavus Dec 13, 2018

QuietMisdreavus Dec 13, 2018

QuietMisdreavus commented Dec 13, 2018

QuietMisdreavus commented Dec 13, 2018

QuietMisdreavus left a comment

QuietMisdreavus commented Dec 14, 2018

bors commented Dec 14, 2018

ollie27 commented Dec 15, 2018

QuietMisdreavus commented Dec 15, 2018

bors commented Dec 23, 2018

Dylan-DPC-zz commented Jan 7, 2019

GuillaumeGomez commented Jan 8, 2019

TimNN commented Jan 22, 2019

		// @has issue_19190/struct.Bar.html
		// @has - '//*[@id="foo.v"]' 'fn foo(&self)'

		// @has - '//*[@id="method.by_explicit_mut_ref"]' 'fn by_explicit_mut_ref(self: &mut Foo)'
		// @!has - '//*[@id="by_explicit_box.v"]' 'fn by_explicit_box(self: Box<Foo>)'

		// @!has - '//*[@id="method.by_mut_ref"]' 'fn by_mut_ref(&mut self)'
		// @!has - '//*[@id="by_explicit_mut_ref.v"]' 'fn by_explicit_mut_ref(self: &mut Foo)'

[rustdoc] Remove old style files #56577

[rustdoc] Remove old style files #56577

Conversation

GuillaumeGomez commented Dec 6, 2018

QuietMisdreavus commented Dec 6, 2018

ollie27 commented Dec 6, 2018

GuillaumeGomez commented Dec 7, 2018

retep998 commented Dec 7, 2018

nrc commented Dec 7, 2018

ollie27 commented Dec 7, 2018

nrc commented Dec 7, 2018

GuillaumeGomez commented Dec 8, 2018

nrc commented Dec 9, 2018

GuillaumeGomez commented Dec 9, 2018

GuillaumeGomez commented Dec 11, 2018

retep998 commented Dec 12, 2018

GuillaumeGomez commented Dec 12, 2018

QuietMisdreavus left a comment

Choose a reason for hiding this comment

QuietMisdreavus Dec 12, 2018

Choose a reason for hiding this comment

QuietMisdreavus Dec 12, 2018

Choose a reason for hiding this comment

GuillaumeGomez Dec 12, 2018

Choose a reason for hiding this comment

QuietMisdreavus Dec 12, 2018

Choose a reason for hiding this comment

GuillaumeGomez commented Dec 13, 2018

rust-highfive commented Dec 13, 2018

QuietMisdreavus Dec 13, 2018

Choose a reason for hiding this comment

QuietMisdreavus Dec 13, 2018

Choose a reason for hiding this comment

QuietMisdreavus Dec 13, 2018

Choose a reason for hiding this comment

QuietMisdreavus commented Dec 13, 2018

QuietMisdreavus commented Dec 13, 2018

QuietMisdreavus left a comment

Choose a reason for hiding this comment

QuietMisdreavus commented Dec 14, 2018

bors commented Dec 14, 2018

ollie27 commented Dec 15, 2018

QuietMisdreavus commented Dec 15, 2018

bors commented Dec 23, 2018

Dylan-DPC-zz commented Jan 7, 2019

GuillaumeGomez commented Jan 8, 2019

TimNN commented Jan 22, 2019