Skip to content

Trivial consistency check #898

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Aug 14, 2020
Merged

Trivial consistency check #898

merged 7 commits into from
Aug 14, 2020

Conversation

Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Jul 16, 2020 •
edited
Loading

part of #766

jyn514
jyn514 reviewed Jul 17, 2020
View reviewed changes
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks great :D Since we're using crates_index anyway it would also be nice to redo

docs.rs/src/docbuilder/crates.rs

Line 64 in d40fb2c

pub fn crates_from_path<F>(path: &PathBuf, func: &mut F) -> Result<()>
but I won't block on that.

src/utils/consistency/data.rs Outdated
Comment on lines 22 to 24
impl std::fmt::Display for CrateId {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
std::fmt::Display::fmt(&self.0, f)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use std::fmt::{self, Display};?

src/utils/consistency/mod.rs
Comment on lines +15 to +17
if !dry_run {
failure::bail!("TODO: only a --dry-run synchronization is supported currently");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we'd want to ever automate fixing this. Deleting documentation is hard to undo (we can rerun the build, but we'd have to notice and it would be broken in the meantime).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A quick check against the prod-dump from april (with my index force reset back to then) shows 4159 releases of 1477 crates in the index that are not in the db, and 1482 releases of 1438 crates in the db that are not in the index. There's definitely more work to do before this could be run to fix stuff (mostly around improving queueing behaviour), but I think we can get to a point where it's safe enough to have a non dry-run version of this, and manually fixing the issues seems like it's too large.

src/utils/consistency/mod.rs
Comment on lines +19 to +23
log::info!("Loading data from database...");
let timer = std::time::Instant::now();
let db_data =
self::db::load(conn).context("Loading crate data from database for consistency check")?;
log::info!("...loaded in {:?}", timer.elapsed());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, maybe we should add a context.time() like rustc has ... https://doc.rust-lang.org/nightly/nightly-rustc/rustc_session/struct.Session.html#method.time

@Nemo157 Nemo157 mentioned this pull request Jul 18, 2020
Closed
@jyn514
Copy link
Member

jyn514 commented Jul 18, 2020

You had a couple cargo-timing.html files that snuck in, please remove those.

@jyn514 jyn514 added A-admin Area: Administration of the production docs.rs server P-medium Medium priority S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Jul 26, 2020
@jyn514 jyn514 mentioned this pull request Jul 28, 2020
Closed
@Nemo157 Nemo157 mentioned this pull request Jul 28, 2020
Merged
@Nemo157 Nemo157 force-pushed the trivial-consistency-check branch from 90131d3 to 5c60883 Compare August 8, 2020 09:45
@Nemo157
Copy link
Member Author

Nemo157 commented Aug 8, 2020 •
edited
Loading

Removed the cargo-timing.html, updated crates-index and rebased over master (conflicted with #917 since that had cherry-picked changes from here, EDIT: and postgres::Client).

@Nemo157 Nemo157 force-pushed the trivial-consistency-check branch from 5c60883 to 29f7e91 Compare August 8, 2020 10:00
@Nemo157 Nemo157 force-pushed the trivial-consistency-check branch from 29f7e91 to 306d1ed Compare August 8, 2020 10:01
@Nemo157 Nemo157 changed the title WIP: Trivial consistency check Trivial consistency check Aug 8, 2020
@jyn514 jyn514 added S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Aug 13, 2020
jyn514
jyn514 reviewed Aug 14, 2020
View reviewed changes
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, since it only reports the changes we can fix any bugs in a follow-up PR.

@jyn514 jyn514 merged commit 7852000 into rust-lang:master Aug 14, 2020
@jyn514 jyn514 mentioned this pull request Jun 5, 2021
Closed
@Nemo157 Nemo157 deleted the trivial-consistency-check branch May 31, 2023 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 jyn514 left review comments

Assignees
No one assigned
Labels
A-admin Area: Administration of the production docs.rs server P-medium Medium priority S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Nemo157 @jyn514