Link to last successful build when build fails

[jyn514]

I thought about this while investigating why https://docs.rs/crate/hyper/0.13.0 failed to build - the reason this usually matters to library authors is because new users/users upgrading see an error instead of the docs. If there a version of the docs, even an out of date version, I imagine that would make the failures much less critical for library authors.

[jyn514]

I want to make it a link, not a redirect, so people know that it's an old version.

[jyn514]

Actually maybe this would be better served by implementing #341, #396 at the same time and just putting it in the 'Redirect to Latest Version' button at the top? 'Latest Version' is no longer an accurate description then ... 'latest successful stable version' is closer but way too verbose to put in a link.

[jyn514]

Hmm ... but in this case I think people would want to know the latest version is released even if there's no docs. I think separate links is fine.

Relevant code: https://github.com/rust-lang/docs.rs/blob/master/templates/crate_details.hbs#L65

[jyn514]

The way to get started with this would be to

• find the last successful build from the database (something like SELECT version FROM releases INNER JOIN crates ON releases.crate_id = crates.id WHERE build_status = true AND yanked = false AND crates.name = $1;)
• add the last successful build to the variables for the page
• add a message to the error template which links to the last build

[yvrhdn]

Hi, I would like to work on this 👋 Can you assign me? 🙂

[yvrhdn]

I did some analysis, just to make sure I'm changing the right things 😉

So the main issue: if the build of a release of a crate fails, a user should be able to find the most recent successful build fairly simple.
Currently we often link to the latest release which isn't necessary a successful build.

There a two places this causes a problem:

Crate details page

If a build failed (for example fie-0.16.2), currently an error message is showed:

We can extend the error message with a quick link: "The most recent successful build is fie-0.15.0"
This will allow users to quickly find working documentation.

I also propose extending the "Versions" segment in the left column. I think it would be nice to show a little icon (⚠️) next to versions that failed to build.
That would look a bit like this then 👇

Navigation header

Another place with a possibly problematic link is the navigation bar.

Let's take as example this crate fie. Version 0.16.x doesn't build, but 0.15.0 did.
So when you visit fie-0.15.0, there is a link ":warning: Go to latest version"
This links to a fie-0.16.2 which is not available!

Maybe we can split this up and show two messages instead:

• if the latest release failed: ":warning: Build of latest release failed" --> this links to crate details (useful for library authors)
• if there is a more recent successful build: ":warning: This is not the latest available documentation" --> links to most recent successful build (useful for users)

Changes

So changes I would like to implement:

1. crate details: extend error message to link to the last successful build
2. crate details: add a little icon to versions that failed to build
3. navigation header: split up warning messages into "latest release failed to build" and "go to latest successful build"

Feedback and comments are welcome!

[jyn514]

Currently we often link to the latest release which isn't necessary a successful build.

Yes, and moreover usually it's the latest release which failed a build (at least if the user sees the message at all).

We can extend the error message with a quick link: "The most recent successful build is fie-0.15.0"

Yes, that's about what I was imagining.

I think it would be nice to show a little icon (warning) next to versions that failed to build.

That would be really great! I hadn't though of that :)

So when you visit fie-0.15.0, there is a link "warning Go to latest version" This links to a fie-0.16.2 which is not available!

I am not opposed to fixing this, but there are many issues with the current 'latest version' calculation and I think that would be better as a separate PR. See #396, #502, #223, #341, #513 for related issues.

[jyn514]

So in other words, 1. and 2. are very welcome but it may be better to hold off on 3. for a while.

[yvrhdn]

Agree, I'll focus on crate details for now.

I have been able to add the link to the last successful build. Just not sure how to add it to the UI...

The current error message looks like this:

I can add the link "inline":

Or I can put it in a separate box:

Inline:

➖ feels a bit cluttered
➖ the link is not super intuitive, you have to read the text of the error message

Separate box:

➕ I like that the message comes across very clear
➖ having two info/warning boxes seems a bit much?

[jyn514]

I don't have a strong opinion either way. @GuillaumeGomez you do more of the CSS/styling, do you have a preference?

[pietroalbini]

I prefer the separate box myself.

[yvrhdn]

I've submitted two pull requests related to this issue: #543 and #546.

Take your time checking them. After they have been vetted and merged, a refactor of crate_details.rs might be in order.

[yvrhdn]

With #543 and #546 merged, I think this issue can be closed?

I think the following refactorings in crate_details.rs would be good as well, but I'm not sure if we should pursue them now or wait until we feel more comfortable with the codebase.

I wouldn't mind adding some tests and making a PR for this.

Possible refactorings:

• In CrateDetails::new: the population of the CrateDetails::releases field is not optimal. First we fetch all versions from crates.versions (which would be removed by Remove duplicate crates.version column #544) and then we query releases for each version (map_to_release contains a DB query).
  This could be simplified by using two queries: a query to fetch all crate information and a query to fetch all versions and their build status.

• It kind of bothers me how the struct CrateDetails is used by other modules, like rustdoc.rs. The filename crate_details.rs and crate_details.hbs give the impression that CrateDetails is only used by the crate details page, but that is not the case... I missed this which caused a regression (Fix regression when showing versions in dropdown #557).
  I would propose putting struct CrateDetails as only thing in crate_details.rs and move the page handler crate_details_handler to a separate file crate_details_page.rs.
  latest_version could be made a member function of CrateDetails, since it is always called with a CrateDetails nearby. This would also avoid conversion from String to semver::Version and back.
  The intention of this move would be to bundle all business logic related to crates and their releases in a single file.

[jyn514]

I'm fine with making latest_version a member function. However, I don't see a way to avoid converting to semver::Version and back: we need that conversion to order the versions by semver and not just release date.

I'm also not sure I understand the rationale for a separate crate_details_page. I know having fields for the templates unchecked by the compiler is not ideal, but I don't see how making that a separate .rs file would help with that.

This could be simplified by using two queries: a query to fetch all crate information and a query to fetch all versions and their build status.

I would be interested to see this query, I tried to write it and my SQL wasn't good enough 😆 the problem I ran into was that we have an array of different versions. I guess we could just concatenate them (crate_id = $1 or crate_id = $2 or ...) but I'm not sure how to do that with a parameterized query. Also I don't think this is a super high priority, it has negligible performance impact.

Finally, I would like to see a lot more tests before we make refactorings like this.

[yvrhdn]

I'm fine with making latest_version a member function.

I'll put a PR together in the next few days.

However, I don't see a way to avoid converting to semver::Version and back: we need that conversion to order the versions by semver and not just release date.

I think this could be avoided by either:

• ensuring that releases is already sorted during creation (this is already the case I think), so latest_version doesn't have to sort the Vec again.
• we could store the version as semver::Version instead of String, this would avoid doing the conversion until it is really necessary

I'm also not sure I understand the rationale for a separate crate_details_page. I know having fields for the templates unchecked by the compiler is not ideal, but I don't see how making that a separate .rs file would help with that.

This wouldn't do anything for the templates. The biggest advantage imo is that it communicates intent a lot better.
If crate_details only contains CrateDetails it is clear that this is some kind of data structure that is used by other modules. Currently, because CrateDetails is right next to crate_details_handler it creates the impression that it only exists for crate_details_handler.
Splitting it off would make it more evident that other modules (crate_details_page, rust_doc,...) use CrateDetails to generate their content.

I would be interested to see this query, I tried to write it and my SQL wasn't good enough 😆

This query could populate most fields of CrateDetails (this is basically the existing query minus crates.versions):

SELECT crates.id,
       releases.id,
       crates.name,
       releases.version,
       releases.description,
       releases.authors,
       releases.dependencies,
       releases.readme,
       releases.description_long,
       releases.release_time,
       releases.build_status,
       releases.rustdoc_status,
       releases.repository_url,
       releases.homepage_url,
       releases.keywords,
       releases.have_examples,
       releases.target_name,
       crates.github_stars,
       crates.github_forks,
       crates.github_issues,
       releases.is_library,
       releases.doc_targets,
       releases.license,
       releases.documentation_url
FROM releases
INNER JOIN crates ON releases.crate_id = crates.id
WHERE crates.name = $1 AND releases.version = $2;

And then this query could be used to populate CrateDetails::releases:

SELECT version, build_status
FROM releases
WHERE crate_id = $1;

The output should be something like:

version	build_status
0.0.1	true
0.0.2	true
0.0.3	false

Also I don't think this is a super high priority, it has negligible performance impact.

And agreed, it works fine now.

[yvrhdn]

I think this issue can be closed @jyn514. I will probably do one more small refactor PR tomorrow (similar to #559). The other stuff isn't really worth it cause there are bigger fish to fry.