Attempt to retain current location when loading a different platform

[Nemo157]

For each target platform, if the current location exists within that platform then generate a link directly to it. Otherwise link to the root of the crate in that platform like normal.

fixes #24

[jyn514]

Perhaps it would be helpful to write a test abstraction that does some of this boilerplate for you? Something like this:

fn test_platforms(default_target: Option<&str>, platforms: &[&str], pages: &[&str]) {
    let mut builder = db.fake_release().name("dummy").version("0.1.0");
    for page in pages {
        builder = builder.rustdoc_file(page, b"dummy content");
    }
    for platform in platforms {
        for page in pages {
            builder = builder.rustdoc_file(format!("{}/{}", platform, page), b"dummy content");
        }
        builder = builder.add_target(platform);
    }
    if let Some(default) = default_target {
        builder = builder.default_target(default);
    }
    builder.create()
}

That would shrink down the boilerplate, making it easier to read and write tests.

[Nemo157]

The builder boilerplate doesn't seem too bad to me, it's the assertions that are really annoying. I can probably simplify them a little with a custom assertion macro (one day track(caller) will be stable and we can start writing nice assertion functions).

[pietroalbini]

Hmm, querying S3 to check if the file exists on all built targets at every request sounds a bit expensive. Another approach we could take is to replace all target links with a URL like:

/crate/{name}/{version}/target-redirect/{target}/{path}

The new route will always issue 301 redirects to either:

• That path on the requested target, if it exists
• The root of the crate if the path doesn't exist on that target

[Nemo157]

👍 that sounds like a good way to go.

[jyn514]

I like this new method, it looks both simpler and faster than my original idea :)

I noticed that if you're viewing a generated source file like target-lexicon/0.10.0/x86_64-apple-darwin/src/target_lexicon/opt/rustwide/target/x86_64-apple-darwin/debug/build/target-lexicon-245b12d75558b260/out/host.rs.html, then the platform switch will send you to the index page instead of the source code for the other platform, because the hash for the build directory has changed (in this case, the proper URL was target-lexicon/0.10.0/x86_64-pc-windows-msvc/src/target_lexicon/opt/rustwide/target/x86_64-pc-windows-msvc/debug/build/target-lexicon-952b0a0f05a593ee/out/host.rs.html). This is certainly an edge case and we don't have to fix it now (I'm not sure how we would!) but it might be nice to add later.

[Nemo157]

This is actually a bad assumption, settings.html is distributed for each target.

[GuillaumeGomez]

There is no need to have one settings.html per target. Per rustdoc version eventually but that's it.

[jyn514]

Hmm, maybe we should add it to ESSENTIAL_FILES with everything else that's shared between rustdoc versions?

[jyn514]

But either way it doesn't need to be addressed in this PR.

[Nemo157]

Ok, latest change fixes redirecting to a search page under a non-default target, along with fixing the "Latest version" link on pages where you are on a non-default target and the latest version no longer has the current item, e.g. https://docs.rs/libc/0.1.5/x86_64-pc-windows-gnu/libc/funcs/extra/winsock/index.html

[jyn514]

Ok, so to summarize the changes I'm seeing:

• Refactor crate_details.doc_targets to be a Vec<String> instead of a JSON Value
• Add a /crate/:name/:version/target-redirect/ page which redirects to the page on the new target if it exists, or searches for the page otherwise
• In the main redirect handler (/:name/:version/*), keep the target if it exists (I left a comment about this)
• Set an inner_path variable in the template so that it knows to redirect to /crate/:name/:version/target-redirect/:inner_path
• Change path_for_version to check against a whitelist of platforms instead of assuming anything that's not target_name is a platform.
• Clean up some test code/formatting

[jyn514]

At some point we should probably change this target_name to crate_name or similar ... but that can wait for another PR.

[Nemo157]

Maybe library_name since non-library-target docs aren't built. I was thinking about it a little and target is actually the correct nomenclature for this: Cargo Targets, it's just unfortunate that there is overlap between platform-target and library-target.

[jyn514]

LGTM, will merge when I have some time to monitor the deploy.

Thanks so much for working on this! I know the scope creeped a bit, I appreciate you sticking with it.

[Nemo157]

Oops, turns out sometimes the list of platforms changes per version, so my fix to the non-default platforms wasn't 100%, "latest version" on https://docs.rs/libc/0.1.5/x86_64-pc-windows-gnu/libc/funcs/extra/winsock/index.html will redirect eventually to a 404 page, but at least it's working on https://docs.rs/libc/0.1.5/x86_64-pc-windows-msvc/libc/funcs/extra/winsock/index.html now.

[jyn514]

I think it's enough of an edge case we can deal with it later, I'll open an issue.