Auto merge of #513 - Zeegomo:refactor-crates, r=pietroalbini

Refactor crates

Some changes especially useful for future updates:
* Save sha inside `Crate` enum instead of saving it into db.
* Simplify `Crate` representation inside db

Author: bors

src/actions/experiments/create.rs

@@ -81,7 +81,7 @@ impl Action for CreateExperiment {
                 let skipped = !self.ignore_blacklist && ctx.config.should_skip(krate);
                 transaction.execute(
                     "INSERT INTO experiment_crates (experiment, crate, skipped, status) VALUES (?1, ?2, ?3, ?4);",
-                    &[&self.name, &::serde_json::to_string(&krate)?, &skipped, &Status::Queued.to_string()],
+                    &[&self.name, &krate.id(), &skipped, &Status::Queued.to_string()],
                 )?;
             }
 
@@ -188,7 +188,7 @@ mod tests {
                     &[&ex],
                     |row| {
                         let krate: String = row.get("crate");
-                        serde_json::from_str(&krate).unwrap()
+                        krate.parse().unwrap()
                     },
                 )
                 .unwrap();

src/actions/experiments/edit.rs

@@ -101,7 +101,7 @@ impl Action for EditExperiment {
                          VALUES (?1, ?2, ?3, ?4);",
                         &[
                             &self.name,
-                            &::serde_json::to_string(&krate)?,
+                            &krate.id(),
                             &(!ex.ignore_blacklist && ctx.config.should_skip(krate)),
                             &Status::Queued.to_string(),
                         ],
@@ -256,7 +256,7 @@ mod tests {
                     &[&ex],
                     |row| {
                         let krate: String = row.get("crate");
-                        serde_json::from_str(&krate).unwrap()
+                        krate.parse().unwrap()
                     },
                 )
                 .unwrap();

src/agent/api.rs

@@ -1,5 +1,5 @@
 use crate::agent::Capabilities;
-use crate::crates::{Crate, GitHubRepo};
+use crate::crates::Crate;
 use crate::experiments::Experiment;
 use crate::prelude::*;
 use crate::results::TestResult;
@@ -151,7 +151,7 @@ impl AgentApi {
         toolchain: &Toolchain,
         log: &[u8],
         result: TestResult,
-        shas: &[(GitHubRepo, String)],
+        version: Option<(&Crate, &Crate)>,
     ) -> Fallible<()> {
         self.retry(|this| {
             let _: bool = this
@@ -166,7 +166,7 @@ impl AgentApi {
                             "log": base64::encode(log),
                         },
                     ],
-                    "shas": shas,
+                    "version": version
                 }))
                 .send()?
                 .to_api_response()?;

src/agent/results.rs

@@ -1,25 +1,25 @@
 use crate::agent::api::AgentApi;
 use crate::config::Config;
-use crate::crates::{Crate, GitHubRepo};
+use crate::crates::Crate;
 use crate::experiments::Experiment;
 use crate::prelude::*;
 use crate::results::{EncodingType, TestResult, WriteResults};
 use crate::toolchain::Toolchain;
 use rustwide::logging::{self, LogStorage};
-use std::ops::DerefMut;
+use std::collections::{hash_map::Entry::Occupied, HashMap};
 use std::sync::{Arc, Mutex};
 
 #[derive(Clone)]
 pub struct ResultsUploader<'a> {
     api: &'a AgentApi,
-    shas: Arc<Mutex<Vec<(GitHubRepo, String)>>>,
+    versions: Arc<Mutex<HashMap<Crate, (Crate, bool)>>>,
 }
 
 impl<'a> ResultsUploader<'a> {
     pub fn new(api: &'a AgentApi) -> Self {
         ResultsUploader {
             api,
-            shas: Arc::new(Mutex::new(Vec::new())),
+            versions: Arc::new(Mutex::new(HashMap::new())),
         }
     }
 }
@@ -35,11 +35,11 @@ impl<'a> WriteResults for ResultsUploader<'a> {
         Ok(None)
     }
 
-    fn record_sha(&self, _ex: &Experiment, repo: &GitHubRepo, sha: &str) -> Fallible<()> {
-        self.shas
+    fn update_crate_version(&self, _ex: &Experiment, old: &Crate, new: &Crate) -> Fallible<()> {
+        self.versions
             .lock()
             .unwrap()
-            .push((repo.clone(), sha.to_string()));
+            .insert(old.clone(), (new.clone(), false));
         Ok(())
     }
 
@@ -60,11 +60,35 @@ impl<'a> WriteResults for ResultsUploader<'a> {
         let result = logging::capture(&storage, f)?;
         let output = storage.to_string();
 
-        let shas = ::std::mem::replace(self.shas.lock().unwrap().deref_mut(), Vec::new());
+        let mut updated = None;
+        let mut new_version = None;
+        // This is done to avoid locking versions if record_progress retries in loop
+        {
+            let mut versions = self.versions.lock().unwrap();
+            if let Occupied(mut entry) = versions.entry(krate.clone()) {
+                let value = entry.get_mut();
+
+                if value.1 {
+                    // delete entry if we already processed both toolchains
+                    updated = Some(entry.remove().0);
+                } else {
+                    updated = Some(value.0.clone());
+                    new_version = updated.as_ref();
+                    // mark we already sent the updated version to the server
+                    value.1 = true;
+                }
+            };
+        }
 
         info!("sending results to the crater server...");
-        self.api
-            .record_progress(ex, krate, toolchain, output.as_bytes(), result, &shas)?;
+        self.api.record_progress(
+            ex,
+            updated.as_ref().unwrap_or(krate),
+            toolchain,
+            output.as_bytes(),
+            result,
+            new_version.map(|new| (krate, new)),
+        )?;
 
         Ok(result)
     }

src/config.rs

@@ -329,10 +329,12 @@ mod tests {
         assert!(list.is_quiet(&Crate::GitHub(GitHubRepo {
             org: "rust-lang".into(),
             name: "rust".into(),
+            sha: None,
         })));
         assert!(!list.is_quiet(&Crate::GitHub(GitHubRepo {
             org: "rust-lang".into(),
             name: "cargo".into(),
+            sha: None,
         })));
 
         assert_eq!(list.chunk_size(), 32);

src/crates/lists.rs

@@ -27,7 +27,7 @@ pub(crate) trait List {
             for krate in &crates {
                 t.execute(
                     "INSERT INTO crates (crate, list, loaded_at) VALUES (?1, ?2, ?3);",
-                    &[&::serde_json::to_string(krate)?, &Self::NAME, &now],
+                    &[&krate.id(), &Self::NAME, &now],
                 )
                 .with_context(|_| {
                     format!(
@@ -51,7 +51,7 @@ pub(crate) trait List {
             &[&Self::NAME],
             |r| {
                 let raw: String = r.get("crate");
-                Ok(::serde_json::from_str(&raw)?)
+                Ok(raw.parse()?)
             },
         )?;
 

src/crates/mod.rs

@@ -21,7 +21,13 @@ impl Crate {
     pub(crate) fn id(&self) -> String {
         match *self {
             Crate::Registry(ref details) => format!("reg/{}/{}", details.name, details.version),
-            Crate::GitHub(ref repo) => format!("gh/{}/{}", repo.org, repo.name),
+            Crate::GitHub(ref repo) => {
+                if let Some(ref sha) = repo.sha {
+                    format!("gh/{}/{}/{}", repo.org, repo.name, sha)
+                } else {
+                    format!("gh/{}/{}", repo.org, repo.name)
+                }
+            }
             Crate::Local(ref name) => format!("local/{}", name),
         }
     }
@@ -54,18 +60,56 @@ impl fmt::Display for Crate {
 impl FromStr for Crate {
     type Err = ::failure::Error;
 
+    // matches with `Crate::id'
     fn from_str(s: &str) -> Fallible<Self> {
-        if s.starts_with("https://github.com/") {
-            Ok(Crate::GitHub(s.parse()?))
-        } else if let Some(dash_idx) = s.rfind('-') {
-            let name = &s[..dash_idx];
-            let version = &s[dash_idx + 1..];
-            Ok(Crate::Registry(RegistryCrate {
+        match s.split('/').collect::<Vec<_>>()[..] {
+            ["reg", name, version] => Ok(Crate::Registry(RegistryCrate {
                 name: name.to_string(),
                 version: version.to_string(),
-            }))
-        } else {
-            bail!("crate not found");
+            })),
+            ["gh", org, name, sha] => Ok(Crate::GitHub(GitHubRepo {
+                org: org.to_string(),
+                name: name.to_string(),
+                sha: Some(sha.to_string()),
+            })),
+            ["gh", org, name] => Ok(Crate::GitHub(GitHubRepo {
+                org: org.to_string(),
+                name: name.to_string(),
+                sha: None,
+            })),
+            ["local", name] => Ok(Crate::Local(name.to_string())),
+            _ => bail!("unexpected crate value"),
+        }
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::{Crate, GitHubRepo, RegistryCrate};
+    use std::str::FromStr;
+
+    #[test]
+    fn test_parse() {
+        macro_rules! test_from_str {
+            ($($str:expr => $rust:expr,)*) => {
+                $(
+                    // Test parsing from string to rust
+                    assert_eq!(Crate::from_str($str).unwrap(), $rust);
+
+                    // Test dumping from rust to string
+                    assert_eq!(&$rust.id(), $str);
+
+                    // Test dumping from rust to string to rust
+                    assert_eq!(Crate::from_str($rust.id().as_ref()).unwrap(), $rust);
+                )*
+            };
+        }
+
+        test_from_str! {
+            "local/build-fail" => Crate::Local("build-fail".to_string()),
+            "gh/org/user" => Crate::GitHub(GitHubRepo{org: "org".to_string(), name: "user".to_string(), sha: None}),
+            "gh/org/user/sha" => Crate::GitHub(GitHubRepo{org: "org".to_string(), name: "user".to_string(), sha: Some("sha".to_string())}),
+            "reg/name/version" => Crate::Registry(RegistryCrate{name: "name".to_string(), version: "version".to_string()}),
         }
     }
 }

src/crates/sources/github.rs

@@ -55,6 +55,7 @@ impl List for GitHubList {
                 list.push(Crate::GitHub(GitHubRepo {
                     org: org.to_string(),
                     name: name.to_string(),
+                    sha: None,
                 }));
             } else {
                 warn!("skipping malformed repo name: {}", line.name);
@@ -69,6 +70,7 @@ impl List for GitHubList {
 pub struct GitHubRepo {
     pub org: String,
     pub name: String,
+    pub sha: Option<String>,
 }
 
 impl GitHubRepo {
@@ -80,6 +82,7 @@ impl GitHubRepo {
         GitHubRepo {
             org: DUMMY_ORG.to_string(),
             name: DUMMY_NAME.to_string(),
+            sha: None,
         }
     }
 }
@@ -88,17 +91,49 @@ impl FromStr for GitHubRepo {
     type Err = ::failure::Error;
 
     fn from_str(input: &str) -> Fallible<Self> {
-        let mut components = input.split('/').collect::<Vec<_>>();
-        let name = components.pop();
+        let mut components = input
+            .trim_start_matches("https://github.com/")
+            .split('/')
+            .rev()
+            .collect::<Vec<_>>();
         let org = components.pop();
+        let name = components.pop();
+        let sha = components.pop();
 
         if let (Some(org), Some(name)) = (org, name) {
             Ok(GitHubRepo {
                 org: org.to_string(),
                 name: name.to_string(),
+                sha: sha.map(|s| s.to_string()),
             })
         } else {
             bail!("malformed repo url: {}", input);
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::GitHubRepo;
+    use std::str::FromStr;
+
+    #[test]
+    fn test_from_str() {
+        assert_eq!(
+            GitHubRepo::from_str("https://github.com/dummy_org/dummy/dummy_sha").unwrap(),
+            GitHubRepo {
+                org: "dummy_org".to_string(),
+                name: "dummy".to_string(),
+                sha: Some("dummy_sha".to_string())
+            }
+        );
+        assert_eq!(
+            GitHubRepo::from_str("https://github.com/dummy_org/dummy").unwrap(),
+            GitHubRepo {
+                org: "dummy_org".to_string(),
+                name: "dummy".to_string(),
+                sha: None
+            }
+        );
+    }
+}

src/db/migrations.rs

@@ -313,6 +313,56 @@ fn migrations() -> Vec<(&'static str, MigrationKind)> {
         ),
     ));
 
+    migrations.push((
+        "delete_sha_table",
+        MigrationKind::SQL(
+            "
+            DROP TABLE shas;
+            ",
+        ),
+    ));
+
+    migrations.push((
+        "stringify_crate_names",
+        MigrationKind::Code(Box::new(|t| {
+            use crate::crates::Crate;
+
+            let fn_name = format!(
+                "crater_migration__{}",
+                rand::thread_rng()
+                    .sample_iter(&Alphanumeric)
+                    .take(10)
+                    .collect::<String>()
+            );
+            t.create_scalar_function(&fn_name, 1, true, |ctx| {
+                let legacy = ctx.get::<String>(0)?;
+
+                if let Ok(parsed) = serde_json::from_str::<Crate>(&legacy) {
+                    Ok(parsed.id())
+                } else {
+                    Ok(legacy)
+                }
+            })?;
+
+            t.execute("PRAGMA foreign_keys = OFF;", no_args())?;
+            t.execute(
+                &format!("UPDATE experiment_crates SET crate = {}(crate);", fn_name),
+                no_args(),
+            )?;
+            t.execute(
+                &format!("UPDATE results SET crate = {}(crate);", fn_name),
+                no_args(),
+            )?;
+            t.execute(
+                &format!("UPDATE crates SET crate = {}(crate);", fn_name),
+                no_args(),
+            )?;
+            t.execute("PRAGMA foreign_keys = ON;", no_args())?;
+
+            Ok(())
+        })),
+    ));
+
     migrations
 }