Auto merge of #7237 - ehuss:git-with-version, r=alexcrichton

Allow git+version dependency to be published.

This allows you to publish a dependency that specifies both `git` and `version`. The `git` value will be stripped out, just like a `path` dependency.

My original intent was to improve the error message, which was very confusing. I figured I might as well make `git` behave the same as `path`. I can change this PR to just reword the error instead of changing behavior if the new behavior isn't desired.

Closes #6738

src/cargo/ops/registry.rs

@@ -114,41 +114,47 @@ fn verify_dependencies(
     registry_src: SourceId,
 ) -> CargoResult<()> {
     for dep in pkg.dependencies().iter() {
-        if dep.source_id().is_path() {
+        if dep.source_id().is_path() || dep.source_id().is_git() {
             if !dep.specified_req() {
+                let which = if dep.source_id().is_path() {
+                    "path"
+                } else {
+                    "git"
+                };
+                let dep_version_source = dep.registry_id().map_or_else(
+                    || "crates.io".to_string(),
+                    |registry_id| registry_id.display_registry_name(),
+                );
                 bail!(
-                    "all path dependencies must have a version specified \
-                     when publishing.\ndependency `{}` does not specify \
-                     a version",
-                    dep.package_name()
+                    "all dependencies must have a version specified when publishing.\n\
+                     dependency `{}` does not specify a version\n\
+                     Note: The published dependency will use the version from {},\n\
+                     the `{}` specification will be removed from the dependency declaration.",
+                    dep.package_name(),
+                    dep_version_source,
+                    which,
                 )
             }
+        // TomlManifest::prepare_for_publish will rewrite the dependency
+        // to be just the `version` field.
         } else if dep.source_id() != registry_src {
-            if dep.source_id().is_registry() {
-                // Block requests to send to crates.io with alt-registry deps.
-                // This extra hostname check is mostly to assist with testing,
-                // but also prevents someone using `--index` to specify
-                // something that points to crates.io.
-                if registry_src.is_default_registry() || registry.host_is_crates_io() {
-                    bail!("crates cannot be published to crates.io with dependencies sourced from other\n\
-                           registries either publish `{}` on crates.io or pull it into this repository\n\
-                           and specify it with a path and version\n\
-                           (crate `{}` is pulled from {})",
-                          dep.package_name(),
-                          dep.package_name(),
-                          dep.source_id());
-                }
-            } else {
-                bail!(
-                    "crates cannot be published with dependencies sourced from \
-                     a repository\neither publish `{}` as its own crate and \
-                     specify a version as a dependency or pull it into this \
-                     repository and specify it with a path and version\n(crate `{}` has \
-                     repository path `{}`)",
-                    dep.package_name(),
-                    dep.package_name(),
-                    dep.source_id()
-                );
+            if !dep.source_id().is_registry() {
+                // Consider making SourceId::kind a public type that we can
+                // exhaustively match on. Using match can help ensure that
+                // every kind is properly handled.
+                panic!("unexpected source kind for dependency {:?}", dep);
+            }
+            // Block requests to send to crates.io with alt-registry deps.
+            // This extra hostname check is mostly to assist with testing,
+            // but also prevents someone using `--index` to specify
+            // something that points to crates.io.
+            if registry_src.is_default_registry() || registry.host_is_crates_io() {
+                bail!("crates cannot be published to crates.io with dependencies sourced from other\n\
+                       registries. `{}` needs to be published to crates.io before publishing this crate.\n\
+                       (crate `{}` is pulled from {})",
+                      dep.package_name(),
+                      dep.package_name(),
+                      dep.source_id());
             }
         }
     }

src/cargo/util/toml/mod.rs

@@ -801,23 +801,27 @@ impl TomlManifest {
         }
 
         fn map_dependency(config: &Config, dep: &TomlDependency) -> CargoResult<TomlDependency> {
-            match *dep {
-                TomlDependency::Detailed(ref d) => {
+            match dep {
+                TomlDependency::Detailed(d) => {
                     let mut d = d.clone();
-                    d.path.take(); // path dependencies become crates.io deps
-                                   // registry specifications are elaborated to the index URL
+                    // Path dependencies become crates.io deps.
+                    d.path.take();
+                    // Same with git dependencies.
+                    d.git.take();
+                    d.branch.take();
+                    d.tag.take();
+                    d.rev.take();
+                    // registry specifications are elaborated to the index URL
                     if let Some(registry) = d.registry.take() {
                         let src = SourceId::alt_registry(config, &registry)?;
                         d.registry_index = Some(src.url().to_string());
                     }
                     Ok(TomlDependency::Detailed(d))
                 }
-                TomlDependency::Simple(ref s) => {
-                    Ok(TomlDependency::Detailed(DetailedTomlDependency {
-                        version: Some(s.clone()),
-                        ..Default::default()
-                    }))
-                }
+                TomlDependency::Simple(s) => Ok(TomlDependency::Detailed(DetailedTomlDependency {
+                    version: Some(s.clone()),
+                    ..Default::default()
+                })),
             }
         }
     }

tests/testsuite/publish.rs

@@ -1,7 +1,7 @@
 use std::fs::{self, File};
 use std::io::prelude::*;
 
-use crate::support::git::repo;
+use crate::support::git::{self, repo};
 use crate::support::paths;
 use crate::support::registry::{self, registry_path, registry_url, Package};
 use crate::support::{basic_manifest, project, publish};
@@ -283,11 +283,10 @@ fn git_deps() {
         .with_stderr(
             "\
 [UPDATING] [..] index
-[ERROR] crates cannot be published with dependencies sourced from \
-a repository\neither publish `foo` as its own crate and \
-specify a version as a dependency or pull it into this \
-repository and specify it with a path and version\n\
-(crate `foo` has repository path `git://path/to/nowhere`)\
+[ERROR] all dependencies must have a version specified when publishing.
+dependency `foo` does not specify a version
+Note: The published dependency will use the version from crates.io,
+the `git` specification will be removed from the dependency declaration.
 ",
         )
         .run();
@@ -323,8 +322,10 @@ fn path_dependency_no_version() {
         .with_stderr(
             "\
 [UPDATING] [..] index
-[ERROR] all path dependencies must have a version specified when publishing.
+[ERROR] all dependencies must have a version specified when publishing.
 dependency `bar` does not specify a version
+Note: The published dependency will use the version from crates.io,
+the `path` specification will be removed from the dependency declaration.
 ",
         )
         .run();
@@ -367,7 +368,7 @@ fn dont_publish_dirty() {
     registry::init();
     let p = project().file("bar", "").build();
 
-    let _ = repo(&paths::root().join("foo"))
+    let _ = git::repo(&paths::root().join("foo"))
         .file(
             "Cargo.toml",
             r#"
@@ -1070,3 +1071,110 @@ Check for a source-replacement in .cargo/config.
         )
         .run();
 }
+
+#[cargo_test]
+fn publish_git_with_version() {
+    // A dependency with both `git` and `version`.
+    Package::new("dep1", "1.0.1")
+        .file("src/lib.rs", "pub fn f() -> i32 {1}")
+        .publish();
+
+    let git_project = git::new("dep1", |project| {
+        project
+            .file("Cargo.toml", &basic_manifest("dep1", "1.0.0"))
+            .file("src/lib.rs", "pub fn f() -> i32 {2}")
+    });
+
+    let p = project()
+        .file(
+            "Cargo.toml",
+            &format!(
+                r#"
+                [package]
+                name = "foo"
+                version = "0.1.0"
+                authors = []
+                edition = "2018"
+                license = "MIT"
+                description = "foo"
+
+                [dependencies]
+                dep1 = {{version = "1.0", git="{}"}}
+                "#,
+                git_project.url()
+            ),
+        )
+        .file(
+            "src/main.rs",
+            r#"
+            pub fn main() {
+                println!("{}", dep1::f());
+            }
+            "#,
+        )
+        .build();
+
+    p.cargo("run").with_stdout("2").run();
+    p.cargo("publish --no-verify --index")
+        .arg(registry_url().to_string())
+        .run();
+
+    publish::validate_upload_with_contents(
+        r#"
+        {
+          "authors": [],
+          "badges": {},
+          "categories": [],
+          "deps": [
+            {
+              "default_features": true,
+              "features": [],
+              "kind": "normal",
+              "name": "dep1",
+              "optional": false,
+              "registry": "https://github.com/rust-lang/crates.io-index",
+              "target": null,
+              "version_req": "^1.0"
+            }
+          ],
+          "description": "foo",
+          "documentation": null,
+          "features": {},
+          "homepage": null,
+          "keywords": [],
+          "license": "MIT",
+          "license_file": null,
+          "links": null,
+          "name": "foo",
+          "readme": null,
+          "readme_file": null,
+          "repository": null,
+          "vers": "0.1.0"
+          }
+        "#,
+        "foo-0.1.0.crate",
+        &["Cargo.lock", "Cargo.toml", "Cargo.toml.orig", "src/main.rs"],
+        &[
+            (
+                "Cargo.toml",
+                // Check that only `version` is included in Cargo.toml.
+                "[..]\n\
+                 [dependencies.dep1]\n\
+                 version = \"1.0\"\n\
+                 ",
+            ),
+            (
+                "Cargo.lock",
+                // The important check here is that it is 1.0.1 in the registry.
+                "[..]\n\
+                 [[package]]\n\
+                 name = \"foo\"\n\
+                 version = \"0.1.0\"\n\
+                 dependencies = [\n\
+                 \x20\"dep1 1.0.1 (registry+https://github.com/rust-lang/crates.io-index)\",\n\
+                 ]\n\
+                 [..]",
+            ),
+        ],
+    );
+}

tests/testsuite/support/publish.rs

@@ -3,8 +3,8 @@ use std::fs::File;
 use std::io::{self, prelude::*, SeekFrom};
 use std::path::{Path, PathBuf};
 
-use crate::support::find_json_mismatch;
 use crate::support::registry::{self, alt_api_path};
+use crate::support::{find_json_mismatch, lines_match};
 
 use flate2::read::GzDecoder;
 use tar::Archive;
@@ -26,6 +26,24 @@ pub fn validate_upload(expected_json: &str, expected_crate_name: &str, expected_
         expected_json,
         expected_crate_name,
         expected_files,
+        &[],
+    );
+}
+
+/// Checks the result of a crate publish, along with the contents of the files.
+pub fn validate_upload_with_contents(
+    expected_json: &str,
+    expected_crate_name: &str,
+    expected_files: &[&str],
+    expected_contents: &[(&str, &str)],
+) {
+    let new_path = registry::api_path().join("api/v1/crates/new");
+    _validate_upload(
+        &new_path,
+        expected_json,
+        expected_crate_name,
+        expected_files,
+        expected_contents,
     );
 }
 
@@ -41,6 +59,7 @@ pub fn validate_alt_upload(
         expected_json,
         expected_crate_name,
         expected_files,
+        &[],
     );
 }
 
@@ -49,6 +68,7 @@ fn _validate_upload(
     expected_json: &str,
     expected_crate_name: &str,
     expected_files: &[&str],
+    expected_contents: &[(&str, &str)],
 ) {
     let mut f = File::open(new_path).unwrap();
     // 32-bit little-endian integer of length of JSON data.
@@ -69,7 +89,12 @@ fn _validate_upload(
     assert_eq!(f.seek(SeekFrom::End(0)).unwrap(), current);
 
     // Verify the tarball.
-    validate_crate_contents(&krate_bytes[..], expected_crate_name, expected_files, &[]);
+    validate_crate_contents(
+        &krate_bytes[..],
+        expected_crate_name,
+        expected_files,
+        expected_contents,
+    );
 }
 
 /// Checks the contents of a `.crate` file.
@@ -126,7 +151,16 @@ pub fn validate_crate_contents(
             let actual_contents = files
                 .get(&full_e_name)
                 .unwrap_or_else(|| panic!("file `{}` missing in archive", e_file_name));
-            assert_eq!(actual_contents, e_file_contents);
+            if !lines_match(e_file_contents, actual_contents) {
+                panic!(
+                    "Crate contents mismatch for {:?}:\n\
+                     --- expected\n\
+                     {}\n\
+                     --- actual \n\
+                     {}\n",
+                    e_file_name, e_file_contents, actual_contents
+                );
+            }
         }
     }
 }