Display reported build information in a badge and on crate version pages #764

shepmaster · 2017-06-08T17:12:29Z

(This is a replacement of #540)

This goes with rust-lang/cargo#5099. @carols10cents worked on these too :)

This PR adds a table that stores recorded crate version, rust version, target, and pass/fail as reported by crate owners using the cargo publish-build-info command.

The purpose is to enable crate authors to automatically report to potential crate users on the Rust version and platform compatibility of each version of the crate.

There's documentation in the cargo PR for doc.crates.io about how this feature is intended to work.

The badges, as described in that documentation, will display the latest version of the most stable channel that reported any pass results for the max version of that crate. Here's what that will look like, including the hover text:

Stable:

Beta:

Nightly:

Each crate version page will display more detail, but not all of the possible information that collection supports. It's in a new section above the download stats graph as a table showing the most recent stable, beta, and nightly with any reported results for the Tier 1 platforms using 64 bit architectures:

There's also a new page with full information for the 64-bit tier 1 platforms, showing more detailed information:

Some enhancements to the display that we've thought about but haven't implemented yet:

Have dropdowns for all the available versions for each channel
Have a dropdown to select from additional targets
Show icons or badges or colors instead of or in addition to the table

We also thought about, but didn't implement:

Ember tests for the UI
Adding a different API token that would only be valid for publishing build info
Showing the max build version badge on crate page in the sidebar

We're excited to hear what anyone thinks, and of course happy to answer questions or make modifications!

sgrif · 2017-06-08T17:28:33Z

src/version.rs

+            build_info::table.primary_key(),
+            do_update()
+                .set((build_info::passed.eq(excluded(build_info::passed)),
+                      build_info::updated_at.eq(now_utc().to_timespec())))


updated_at is handled by database triggers everywhere else. Should we do the same here? (adding SELECT diesel_manage_updated_at('build_info'); to your migration will have Diesel set up the trigger automatically.

Ah yes, good point. We should be using the db triggers here.

Will the triggers also handle created_at?

sgrif · 2017-06-08T17:45:27Z

src/version.rs

+                ChannelVersion::Beta(date) => beta.push(date),
+                ChannelVersion::Nightly(date) => nightly.push(date),
+            }
+        }


Do you think this amount of code is worth it just to avoid the one allocation of an intermediate vec? What do you think about this instead?

let rust_versions = build_infos.into_iter().map(|row| row.rust_version.parse::<ChannelVersion>()).collect::<Result<Vec<_>, _>>()?; let nightly = rust_versions.iter().filter_map(|version| match *version { ChannelVersion::Nightly(date) => Some(date), _ => None, }).max(); let beta = rust_versions.iter().filter_map(|version| match *version { ChannelVersion::Beta(date) => Some(date), _ => None, }).max(); let stable = rust_versions.into_iter().filter_map(|version| match version { ChannelVersion::Stable(semver) => Some(semver), _ => None, }).max();

I don't see which part of the proposed code only keeps the most recent value?

What do you mean by "most recent"? The code simply grabs the maximum of each one, does it not?

Ah, there's a max call at the end of each of those. I'm not the biggest fan of that syntax for that reason. I see what you mean now though.

just to avoid the one allocation of an intermediate vec

To be honest, I've never really worked on mission-critical infrastructure supporting the entire ecosystem behind a language, so I don't really know when it's appropriate. From the comment, I assume you think it's not, so I'll defer to your greater experience.

sgrif · 2017-06-08T17:46:33Z

src/version.rs

+    passed: bool,
+}
+
+pub const BUILD_INFO_FIELDS: (build_info::version_id, build_info::rust_version, build_info::target, build_info::passed) = (build_info::version_id, build_info::rust_version, build_info::target, build_info::passed);


Do we even need the created_at and updated_at fields if we aren't selecting them or using them for filtering anywhere?

I'd like to have them, I could see wanting to display them somewhere, but they're useful to me for auditing purposes.

sgrif · 2017-06-08T17:48:24Z

src/version.rs

+        }
+    }
+
+    encodable_build_info.ordering.insert(


Do you think using the ordermap crate instead of a hashmap could simplify this code?

I think we'd still want the "stable", "beta", "nightly" keys for the JSON... or am I misunderstanding? How would ordermap instead of a hashmap simplify this code?

We wouldn't need to maintain this ordering field, since OrderMap preserves insertion order. Even if we still need ordering for the front-end, it'd just be .keys().collect() to get the corresponding vec.

Though on second reading, that's not even what this code is doing. Since it's in a BTreeSet, it's just going to be sorting the strings in ascending order. Is that the desired behavior? It seems like we could just sort them when we're ready to display them in that case.

I've updated the function with an example of the output as well as the rationale behind it. In short: no, I don't think the ordermap crate would help here.

As far as I understand it, JSON and JavaScript do not guarantee the order of objects. The ordering field exists to avoid requiring the frontend to understand how to sort by date and/or semver.

While OrderMap preserves insertion order, our iteration order over the returned rows is not guaranteed to be anything useful, therefore the insertion order is not valuable.

We aren't relying on the BTree part, just the Set part; we could just have easily used a HashSet. I only use BTree* stuff as a favor to @gankro, who told me that B-Trees are just all-around-cooler.

Additionally, the sets don't contain Strings — the stable one contains version numbers and the others two contain dates. We only convert to a string to serialize.

sgrif · 2017-06-08T17:48:48Z

src/version.rs

+/// Handles the `POST /crates/:crate_id/:version/build_info` route.
+pub fn publish_build_info(req: &mut Request) -> CargoResult<Response> {
+    let mut body = String::new();
+    try!(req.body().read_to_string(&mut body));


sgrif · 2017-06-08T17:51:22Z

src/krate.rs

    }

+    #[cfg_attr(feature = "lint", allow(too_many_arguments))]


Do you think we should listen to this lint here, and start to refactor this out a bit? encodable feels like it's becoming a bit of a dumping ground.

I do think we should refactor this, I am agnostic about whether it should be done in this PR or in a separate PR

sgrif · 2017-06-08T17:52:37Z

src/version.rs

+#[derive(Insertable, AsChangeset)]
+#[table_name="build_info"]
+#[primary_key(version_id, rust_version, target)]
+struct NewBuildInfo {


If we expect this table to be relatively stable, it might be worth just dumping this struct and adding #[derive(Insertable, AsChangeset)] onto BuildInfo.

That sounds good.

shepmaster · 2017-06-15T18:31:56Z

I've rebased to address some conflicts and applied rustfmt to each commit. @sgrif I've added a commit that does your proposed max finding. What are your thoughts on it?

I'm still going to address these comments:

Display reported build information in a badge and on crate version pages #764 (comment)
Display reported build information in a badge and on crate version pages #764 (comment)
Display reported build information in a badge and on crate version pages #764 (comment)
Display reported build information in a badge and on crate version pages #764 (comment)

I just wanted to save them from being hidden

shepmaster · 2017-07-27T16:27:52Z

@sgrif

function diesel_manage_updated_at(unknown) does not exist

hints?

shepmaster · 2017-07-28T13:54:30Z

Ah, diesel-rs/diesel#991

shepmaster · 2017-07-28T13:56:13Z

All talking points addressed. Ready for next round of review.

vignesh-sankaran

Could we have doc comments added to all new public and private types?

vignesh-sankaran · 2017-08-01T13:29:58Z

src/upload.rs

@@ -55,6 +57,19 @@ pub struct CrateDependency {
    pub kind: Option<DependencyKind>,
 }

+#[derive(Debug, Serialize, Deserialize)]
+pub struct VersionBuildInfo {


Could we add a doc comment here?

vignesh-sankaran · 2017-08-01T13:31:58Z

src/version.rs

+#[belongs_to(Version)]
+#[table_name = "build_info"]
+#[primary_key(version_id, rust_version, target)]
+pub struct BuildInfo {


Could we have a doc comment here as well as for all structs, enums, and functions in this module?

Icons from https://github.com/vorillaz/devicons

If the latest version builds on any stable version, show that it builds on stable. If not and it builds on any beta version, show that. If not and it builds on any nightly version, show that. Otherwise, don't show any badge.

carols10cents · 2018-02-06T17:39:39Z

Ok, I've rebased and revived this old thing, in the start of an effort to actually get through all the open PRs.

I'd love reviews from:

@sgrif to make sure I updated the diesel parts properly
@jtgeibel to see if I put things in the right spots in the new MVC layout, there are a few things i'm not sure about
@shepmaster to make sure the original functionality we intended is still there

whenever yinz have time <3

…ed-page

jtgeibel · 2018-03-01T00:37:49Z

@carols10cents I've finally gotten around to looking at this. In terms of MVC style, I have no issues.

I'm testing this PR in a staging area with the cargo PR as well. Here are a few things I've run into:

The crate.png image has been replaced with SVG, but the build-info page still references the PNG.
I've pushed build info for stable, beta, and nightly. I'm only seeing the information for stable.
- At first I pushed nightly build info. This showed up as a badge on the browse page, but didn't show up on the crate's page.
- Once I pushed stable build info, the summary showed up on the crate's page, but only for stable. The summary page and build-info details page are both missing information on beta and nightly. All the data is present in the API response, so I think this is a frontend issue.

For reference, the staging area is here.

The NaiveDate type doesn't include any time or timezone information, so I'm explicitly formatting as year-month-day ("%Y-%m-%d").

jtgeibel · 2018-03-01T04:36:07Z

@carols10cents I've pushed a commit that should pass your new test. I'm explicitly formatting as "%Y-%m-%d" and tweaked the test a bit to match. I'm still reviewing but will probably r+ this weekend.

sgrif

Rust side generally looks good to me. I'd like to take a second look at the parsing/encoding stuff when I'm less tired. I don't know enough about Ember to comment on that end.

sgrif · 2018-03-04T22:45:01Z

src/views/mod.rs

+
+/// `MaxBuildInfo` in JSON form.
+#[derive(Debug, Default, Serialize, Deserialize)]
+pub struct EncodableMaxVersionBuildInfo {


Every field on MaxBuildVersionInfo implements Serialize, doesn't it? Is there a reason we need this as a separate struct?

sgrif · 2018-03-04T22:46:40Z

src/version/build_info.rs

+/// The columns to select from the `build_info` table. The table also stores `created_at` and
+/// `updated_at` metadata for each row, but we're not displaying those anywhere so we're not
+/// bothering to select them.
+pub const BUILD_INFO_FIELDS: (


Are the created_at and updated_at fields ever used at all? Is it worth even having them in the first place?

sgrif · 2018-03-04T22:49:14Z

src/version/build_info.rs

+/// Stores information about whether this version built on the specified Rust version and target.
+pub struct BuildInfo {
+    version_id: i32,
+    pub rust_version: String,


Is this always a semver version? Is it worth pointing at dtolnay/semver#169 to make things easier here?

@sgrif currently this is storing the full version string, such as rustc 1.15.0-beta.5 (10893a9a3 2017-01-19). It may be better to parse this string when the build info is submitted, but I'm not sure how to best store the ParsedRustChannelVersion enum in the database. Maybe 3 columns, rust_channel (enum), rust_version (semver), and rust_date (date) where we have a semver or a date, but not both on any given row. Seems complicated, but I'd also prefer not to store a bunch of full rustc version strings.

pub enum ParsedRustChannelVersion { Stable(semver::Version), Beta(NaiveDate), Nightly(NaiveDate), }

This should fix a regression introduced in this commit series. We should add more tests for the search route.

vignesh-sankaran · 2018-12-17T08:35:58Z

@jtgeibel @shepmaster Did we still want to merge this in? This probably needs a rework given that the codebase has been refactored since the PR was opened.

carols10cents · 2018-12-23T22:21:33Z

Ugh, yeah, I'm going to close.

sgrif reviewed Jun 8, 2017

View reviewed changes

carols10cents added the S-waiting-on-review label Jun 9, 2017

shepmaster force-pushed the build-info-detailed-page branch from 3264a05 to c01c6bf Compare June 15, 2017 18:22

carols10cents mentioned this pull request Jun 23, 2017

RFC: Improve the visibility of the nightly compiler requirement for crates that use unstable features. rust-lang/rfcs#2038

Closed

carols10cents added S-waiting-on-author and removed S-waiting-on-review labels Jul 8, 2017

shepmaster force-pushed the build-info-detailed-page branch 3 times, most recently from 5f8c398 to f86a363 Compare July 27, 2017 20:45

shepmaster force-pushed the build-info-detailed-page branch from f86a363 to 956b1a9 Compare July 28, 2017 13:55

vignesh-sankaran reviewed Aug 1, 2017

View reviewed changes

carols10cents added S-waiting-on-review and removed S-waiting-on-author labels Aug 2, 2017

carols10cents mentioned this pull request Sep 12, 2017

Alert the user up-front when their dependency graph contains unstable code and they're building on stable or beta Rust rust-lang/cargo#2314

Closed

carols10cents force-pushed the build-info-detailed-page branch 3 times, most recently from 6d29a62 to aacad02 Compare February 6, 2018 16:51

carols10cents added 3 commits February 6, 2018 12:14

Store version build info uploaded by cargo in the database, also return

f46203f

Display the latest version and targets this crate version builds on

6f7b34e

Icons from https://github.com/vorillaz/devicons

Show detailed build information on a separate page

c492420

carols10cents force-pushed the build-info-detailed-page branch from aacad02 to c492420 Compare February 6, 2018 17:14

carols10cents mentioned this pull request Mar 1, 2018

Add a publish-build-info command for reporting build results to crates.io rust-lang/cargo#5099

Closed

Merge remote-tracking branch 'upstream/master' into build-info-detail…

231d7e2

…ed-page

carols10cents and others added 2 commits February 28, 2018 20:31

Failing test for build info JSON key mismatches

6cdb0e7

Change date format in build-info ordering and use SVG instead of PNG

881ff2c

The NaiveDate type doesn't include any time or timezone information, so I'm explicitly formatting as year-month-day ("%Y-%m-%d").

sgrif reviewed Mar 4, 2018

View reviewed changes

Swap order of semver comparison when returning search results

6c9794d

This should fix a regression introduced in this commit series. We should add more tests for the search route.

jtgeibel force-pushed the build-info-detailed-page branch from baadd1d to 6c9794d Compare March 8, 2018 05:01

carols10cents closed this Dec 23, 2018

Display reported build information in a badge and on crate version pages #764

Display reported build information in a badge and on crate version pages #764

Conversation

shepmaster commented Jun 8, 2017 • edited by carols10cents Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgrif Jun 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shepmaster Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shepmaster commented Jun 15, 2017 • edited Loading

shepmaster commented Jul 27, 2017

shepmaster commented Jul 28, 2017

shepmaster commented Jul 28, 2017

vignesh-sankaran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vignesh-sankaran Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

carols10cents commented Feb 6, 2018

jtgeibel commented Mar 1, 2018

jtgeibel commented Mar 1, 2018

sgrif left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vignesh-sankaran commented Dec 17, 2018

carols10cents commented Dec 23, 2018

shepmaster commented Jun 8, 2017 •

edited by carols10cents

Loading

sgrif Jun 8, 2017 •

edited

Loading

shepmaster Jul 27, 2017 •

edited

Loading

shepmaster commented Jun 15, 2017 •

edited

Loading

vignesh-sankaran Aug 1, 2017 •

edited

Loading