Auto merge of #5886 - dekellum:record-package-git-head-3, r=alexcrichton

Generate .cargo_vcs_info.json and include in `cargo package` (take 2)

Implements #5629 and supersedes #5786, with the following improvements:

* With an upstream git2-rs fix (tracked #5823, validated and min version update in: #5858), no longer requires windows/unix split tests.

* Per review in #5786, drops file system output and locks for .cargo_vcs_info.json.

* Now uses serde `json!` macro for json production with appropriate escaping.

* Now includes a test of the output json format.

Possible followup:

* Per discussion in #5786, we could improve reliability of both the VCS dirty check and the git head sha1 recording by preferring (and/or warning otherwise) the local repository bytes of each source file, at the same head commit. This makes the process more appropriately like an atomic snapshot, with no sentry file or other locking required.  However given my lack of a window's license and dev setup, as exhibited by troubles of #5823, this feel intuitively like too much to attempt to change in one iteration here.  I accept the "best effort" concept of this feature as suggested in #5786, and think it acceptable progress if merged in this form.

@alexcrichton r?
@joshtriplett cc

Author: bors

src/cargo/ops/cargo_package.rs

@@ -1,12 +1,13 @@
 use std::fs::{self, File};
-use std::io::SeekFrom;
 use std::io::prelude::*;
-use std::path::{self, Path};
+use std::io::SeekFrom;
+use std::path::{self, Path, PathBuf};
 use std::sync::Arc;
 
 use flate2::read::GzDecoder;
 use flate2::{Compression, GzBuilder};
 use git2;
+use serde_json;
 use tar::{Archive, Builder, EntryType, Header};
 
 use core::{Package, Source, SourceId, Workspace};
@@ -28,6 +29,8 @@ pub struct PackageOpts<'cfg> {
     pub registry: Option<String>,
 }
 
+static VCS_INFO_FILE: &'static str = ".cargo_vcs_info.json";
+
 pub fn package(ws: &Workspace, opts: &PackageOpts) -> CargoResult<Option<FileLock>> {
     ops::resolve_ws(ws)?;
     let pkg = ws.current()?;
@@ -42,6 +45,19 @@ pub fn package(ws: &Workspace, opts: &PackageOpts) -> CargoResult<Option<FileLoc
 
     verify_dependencies(pkg)?;
 
+    // `list_files` outputs warnings as a side effect, so only do it once.
+    let src_files = src.list_files(pkg)?;
+
+    // Make sure a VCS info file is not included in source, regardless of if
+    // we produced the file above, and in particular if we did not.
+    check_vcs_file_collision(pkg, &src_files)?;
+
+    // Check (git) repository state, getting the current commit hash if not
+    // dirty. This will `bail!` if dirty, unless allow_dirty. Produce json
+    // info for any sha1 (HEAD revision) returned.
+    let vcs_info = check_repo_state(pkg, &src_files, &config, opts.allow_dirty)?
+        .map(|h| json!({"git":{"sha1": h}}));
+
     if opts.list {
         let root = pkg.root();
         let mut list: Vec<_> = src.list_files(pkg)?
@@ -51,17 +67,16 @@ pub fn package(ws: &Workspace, opts: &PackageOpts) -> CargoResult<Option<FileLoc
         if include_lockfile(pkg) {
             list.push("Cargo.lock".into());
         }
+        if vcs_info.is_some() {
+            list.push(Path::new(VCS_INFO_FILE).to_path_buf());
+        }
         list.sort_unstable();
         for file in list.iter() {
             println!("{}", file.display());
         }
         return Ok(None);
     }
 
-    if !opts.allow_dirty {
-        check_not_dirty(pkg, &src, &config)?;
-    }
-
     let filename = format!("{}-{}.crate", pkg.name(), pkg.version());
     let dir = ws.target_dir().join("package");
     let mut dst = {
@@ -77,7 +92,7 @@ pub fn package(ws: &Workspace, opts: &PackageOpts) -> CargoResult<Option<FileLoc
         .shell()
         .status("Packaging", pkg.package_id().to_string())?;
     dst.file().set_len(0)?;
-    tar(ws, &src, dst.file(), &filename)
+    tar(ws, &src_files, vcs_info.as_ref(), dst.file(), &filename)
         .chain_err(|| format_err!("failed to prepare local package for uploading"))?;
     if opts.verify {
         dst.seek(SeekFrom::Start(0))?;
@@ -152,7 +167,16 @@ fn verify_dependencies(pkg: &Package) -> CargoResult<()> {
     Ok(())
 }
 
-fn check_not_dirty(p: &Package, src: &PathSource, config: &Config) -> CargoResult<()> {
+// Check if the package source is in a *git* DVCS repository. If *git*, and
+// the source is *dirty* (e.g. has uncommited changes) and not `allow_dirty`
+// then `bail!` with an informative message. Otherwise return the sha1 hash of
+// the current *HEAD* commit, or `None` if *dirty*.
+fn check_repo_state(
+    p: &Package,
+    src_files: &[PathBuf],
+    config: &Config,
+    allow_dirty: bool
+) -> CargoResult<Option<String>> {
     if let Ok(repo) = git2::Repository::discover(p.root()) {
         if let Some(workdir) = repo.workdir() {
             debug!("found a git repo at {:?}", workdir);
@@ -164,7 +188,7 @@ fn check_not_dirty(p: &Package, src: &PathSource, config: &Config) -> CargoResul
                         "found (git) Cargo.toml at {:?} in workdir {:?}",
                         path, workdir
                     );
-                    return git(p, src, &repo);
+                    return git(p, src_files, &repo, allow_dirty);
                 }
             }
             config.shell().verbose(|shell| {
@@ -182,11 +206,16 @@ fn check_not_dirty(p: &Package, src: &PathSource, config: &Config) -> CargoResul
 
     // No VCS with a checked in Cargo.toml found. so we don't know if the
     // directory is dirty or not, so we have to assume that it's clean.
-    return Ok(());
-
-    fn git(p: &Package, src: &PathSource, repo: &git2::Repository) -> CargoResult<()> {
+    return Ok(None);
+
+    fn git(
+        p: &Package,
+        src_files: &[PathBuf],
+        repo: &git2::Repository,
+        allow_dirty: bool
+    ) -> CargoResult<Option<String>> {
         let workdir = repo.workdir().unwrap();
-        let dirty = src.list_files(p)?
+        let dirty = src_files
             .iter()
             .filter(|file| {
                 let relative = file.strip_prefix(workdir).unwrap();
@@ -204,20 +233,46 @@ fn check_not_dirty(p: &Package, src: &PathSource, config: &Config) -> CargoResul
             })
             .collect::<Vec<_>>();
         if dirty.is_empty() {
-            Ok(())
+            let rev_obj = repo.revparse_single("HEAD")?;
+            Ok(Some(rev_obj.id().to_string()))
         } else {
-            bail!(
-                "{} files in the working directory contain changes that were \
-                 not yet committed into git:\n\n{}\n\n\
-                 to proceed despite this, pass the `--allow-dirty` flag",
-                dirty.len(),
-                dirty.join("\n")
-            )
+            if !allow_dirty {
+                bail!(
+                    "{} files in the working directory contain changes that were \
+                     not yet committed into git:\n\n{}\n\n\
+                     to proceed despite this, pass the `--allow-dirty` flag",
+                    dirty.len(),
+                    dirty.join("\n")
+                )
+            }
+            Ok(None)
         }
     }
 }
 
-fn tar(ws: &Workspace, src: &PathSource, dst: &File, filename: &str) -> CargoResult<()> {
+// Check for and `bail!` if a source file matches ROOT/VCS_INFO_FILE, since
+// this is now a cargo reserved file name, and we don't want to allow
+// forgery.
+fn check_vcs_file_collision(pkg: &Package, src_files: &[PathBuf]) -> CargoResult<()> {
+    let root = pkg.root();
+    let vcs_info_path = Path::new(VCS_INFO_FILE);
+    let collision = src_files.iter().find(|&p| {
+        util::without_prefix(&p, root).unwrap() == vcs_info_path
+    });
+    if collision.is_some() {
+        bail!("Invalid inclusion of reserved file name \
+               {} in package source", VCS_INFO_FILE);
+    }
+    Ok(())
+}
+
+fn tar(
+    ws: &Workspace,
+    src_files: &[PathBuf],
+    vcs_info: Option<&serde_json::Value>,
+    dst: &File,
+    filename: &str
+) -> CargoResult<()> {
     // Prepare the encoder and its header
     let filename = Path::new(filename);
     let encoder = GzBuilder::new()
@@ -229,7 +284,7 @@ fn tar(ws: &Workspace, src: &PathSource, dst: &File, filename: &str) -> CargoRes
     let pkg = ws.current()?;
     let config = ws.config();
     let root = pkg.root();
-    for file in src.list_files(pkg)?.iter() {
+    for file in src_files.iter() {
         let relative = util::without_prefix(file, root).unwrap();
         check_filename(relative)?;
         let relative = relative.to_str().ok_or_else(|| {
@@ -297,6 +352,36 @@ fn tar(ws: &Workspace, src: &PathSource, dst: &File, filename: &str) -> CargoRes
         }
     }
 
+    if let Some(ref json) = vcs_info {
+        let filename: PathBuf = Path::new(VCS_INFO_FILE).into();
+        debug_assert!(check_filename(&filename).is_ok());
+        let fnd = filename.display();
+        config
+            .shell()
+            .verbose(|shell| shell.status("Archiving", &fnd))?;
+        let path = format!(
+            "{}-{}{}{}",
+            pkg.name(),
+            pkg.version(),
+            path::MAIN_SEPARATOR,
+            fnd
+        );
+        let mut header = Header::new_ustar();
+        header.set_path(&path).chain_err(|| {
+            format!("failed to add to archive: `{}`", fnd)
+        })?;
+        let json = format!("{}\n", serde_json::to_string_pretty(json)?);
+        let mut header = Header::new_ustar();
+        header.set_path(&path)?;
+        header.set_entry_type(EntryType::file());
+        header.set_mode(0o644);
+        header.set_size(json.len() as u64);
+        header.set_cksum();
+        ar.append(&header, json.as_bytes()).chain_err(|| {
+            internal(format!("could not archive source file `{}`", fnd))
+        })?;
+    }
+
     if include_lockfile(pkg) {
         let toml = paths::read(&ws.root().join("Cargo.lock"))?;
         let path = format!(

tests/testsuite/package.rs

@@ -155,38 +155,64 @@ See http://doc.crates.io/manifest.html#package-metadata for more info.
 #[test]
 fn package_verbose() {
     let root = paths::root().join("all");
-    let p = git::repo(&root)
+    let repo = git::repo(&root)
         .file("Cargo.toml", &basic_manifest("foo", "0.0.1"))
         .file("src/main.rs", "fn main() {}")
         .file("a/Cargo.toml", &basic_manifest("a", "0.0.1"))
         .file("a/src/lib.rs", "")
         .build();
-    assert_that(cargo_process("build").cwd(p.root()), execs());
+    assert_that(cargo_process("build").cwd(repo.root()), execs());
 
     println!("package main repo");
     assert_that(
-        cargo_process("package -v --no-verify").cwd(p.root()),
+        cargo_process("package -v --no-verify").cwd(repo.root()),
         execs().with_stderr(
             "\
 [WARNING] manifest has no description[..]
 See http://doc.crates.io/manifest.html#package-metadata for more info.
 [PACKAGING] foo v0.0.1 ([..])
 [ARCHIVING] [..]
 [ARCHIVING] [..]
+[ARCHIVING] .cargo_vcs_info.json
 ",
         ),
     );
 
+    let f = File::open(&repo.root().join("target/package/foo-0.0.1.crate")).unwrap();
+    let mut rdr = GzDecoder::new(f);
+    let mut contents = Vec::new();
+    rdr.read_to_end(&mut contents).unwrap();
+    let mut ar = Archive::new(&contents[..]);
+    let mut entry = ar.entries().unwrap()
+        .map(|f| f.unwrap())
+        .find(|e| e.path().unwrap().ends_with(".cargo_vcs_info.json"))
+        .unwrap();
+    let mut contents = String::new();
+    entry.read_to_string(&mut contents).unwrap();
+    assert_eq!(
+        &contents[..],
+        &*format!(
+            r#"{{
+  "git": {{
+    "sha1": "{}"
+  }}
+}}
+"#,
+            repo.revparse_head()
+        )
+    );
+
     println!("package sub-repo");
     assert_that(
-        cargo_process("package -v --no-verify").cwd(p.root().join("a")),
+        cargo_process("package -v --no-verify").cwd(repo.root().join("a")),
         execs().with_stderr(
             "\
 [WARNING] manifest has no description[..]
 See http://doc.crates.io/manifest.html#package-metadata for more info.
 [PACKAGING] a v0.0.1 ([..])
-[ARCHIVING] [..]
-[ARCHIVING] [..]
+[ARCHIVING] Cargo.toml
+[ARCHIVING] src/lib.rs
+[ARCHIVING] .cargo_vcs_info.json
 ",
         ),
     );
@@ -214,6 +240,42 @@ See http://doc.crates.io/manifest.html#package-metadata for more info.
     );
 }
 
+#[test]
+fn vcs_file_collision() {
+    let p = project().build();
+    let _ = git::repo(&paths::root().join("foo"))
+        .file(
+            "Cargo.toml",
+            r#"
+            [project]
+            name = "foo"
+            description = "foo"
+            version = "0.0.1"
+            authors = []
+            license = "MIT"
+            documentation = "foo"
+            homepage = "foo"
+            repository = "foo"
+            exclude = ["*.no-existe"]
+        "#)
+        .file(
+            "src/main.rs",
+            r#"
+            fn main() {}
+        "#)
+        .file(".cargo_vcs_info.json", "foo")
+        .build();
+    assert_that(
+        p.cargo("package").arg("--no-verify"),
+        execs().with_status(101).with_stderr(&format!(
+            "\
+[ERROR] Invalid inclusion of reserved file name .cargo_vcs_info.json \
+in package source
+",
+        )),
+    );
+}
+
 #[test]
 fn path_dependency_no_version() {
     let p = project()
@@ -320,8 +382,6 @@ fn exclude() {
             "\
 [WARNING] manifest has no description[..]
 See http://doc.crates.io/manifest.html#package-metadata for more info.
-[WARNING] No (git) Cargo.toml found at `[..]` in workdir `[..]`
-[PACKAGING] foo v0.0.1 ([..])
 [WARNING] [..] file `dir_root_1/some_dir/file` WILL be excluded [..]
 See [..]
 [WARNING] [..] file `dir_root_2/some_dir/file` WILL be excluded [..]
@@ -334,6 +394,8 @@ See [..]
 See [..]
 [WARNING] [..] file `some_dir/file_deep_1` WILL be excluded [..]
 See [..]
+[WARNING] No (git) Cargo.toml found at `[..]` in workdir `[..]`
+[PACKAGING] foo v0.0.1 ([..])
 [ARCHIVING] [..]
 [ARCHIVING] [..]
 [ARCHIVING] [..]
@@ -483,7 +545,7 @@ fn no_duplicates_from_modified_tracked_files() {
         .unwrap();
     assert_that(cargo_process("build").cwd(p.root()), execs());
     assert_that(
-        cargo_process("package --list").cwd(p.root()),
+        cargo_process("package --list --allow-dirty").cwd(p.root()),
         execs().with_stdout(
             "\
 Cargo.toml
@@ -1188,6 +1250,7 @@ fn package_lockfile_git_repo() {
         p.cargo("package -l").masquerade_as_nightly_cargo(),
         execs().with_stdout(
             "\
+.cargo_vcs_info.json
 Cargo.lock
 Cargo.toml
 src/main.rs

tests/testsuite/support/git.rs

@@ -74,6 +74,10 @@ impl Repository {
     pub fn url(&self) -> Url {
         path2url(self.0.workdir().unwrap().to_path_buf())
     }
+
+    pub fn revparse_head(&self) -> String {
+        self.0.revparse_single("HEAD").expect("revparse HEAD").id().to_string()
+    }
 }
 
 pub fn new<F>(name: &str, callback: F) -> Result<Project, ProcessError>