Auto merge of #6466 - ehuss:registry-cleanup, r=alexcrichton

bors · bors · commit 1dff476dcadb · 2018-12-20T15:40:49.000Z
Rewrite `login` and registry cleanups. - Login: - Change `login` so that it reads the API host from the registry config so it knows the `{api}/me` URL to display. - The `--host` flag is deprecated/unused. It wasn't used for much before. - `--registry` supports interactive entry of the token (does not require it on the command line). - Displays a message after saving the token (fixes #5868). - Because `login` now requires an index, some of the tests had to be updated. - Fix so that if `api` is missing from the config, it will display a nice error message instead of panicking with unwrap.
diff --git a/src/bin/cargo/commands/login.rs b/src/bin/cargo/commands/login.rs
@@ -1,11 +1,6 @@
 use crate::command_prelude::*;
 
-use std::io::{self, BufRead};
-
-use cargo::core::{Source, SourceId};
 use cargo::ops;
-use cargo::sources::RegistrySource;
-use cargo::util::CargoResultExt;
 
 pub fn cli() -> App {
     subcommand("login")
@@ -14,46 +9,19 @@ pub fn cli() -> App {
              If token is not specified, it will be read from stdin.",
         )
         .arg(Arg::with_name("token"))
-        .arg(opt("host", "Host to set the token for").value_name("HOST"))
+        .arg(
+            opt("host", "Host to set the token for")
+                .value_name("HOST")
+                .hidden(true),
+        )
         .arg(opt("registry", "Registry to use").value_name("REGISTRY"))
 }
 
 pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
-    let registry = args.registry(config)?;
-
-    let token = match args.value_of("token") {
-        Some(token) => token.to_string(),
-        None => {
-            let host = match registry {
-                Some(ref _registry) => {
-                    return Err(failure::format_err!(
-                        "token must be provided when \
-                         --registry is provided."
-                    )
-                    .into());
-                }
-                None => {
-                    let src = SourceId::crates_io(config)?;
-                    let mut src = RegistrySource::remote(src, config);
-                    src.update()?;
-                    let config = src.config()?.unwrap();
-                    args.value_of("host")
-                        .map(|s| s.to_string())
-                        .unwrap_or_else(|| config.api.unwrap())
-                }
-            };
-            println!("please visit {}/me and paste the API Token below", host);
-            let mut line = String::new();
-            let input = io::stdin();
-            input
-                .lock()
-                .read_line(&mut line)
-                .chain_err(|| "failed to read stdin")
-                .map_err(failure::Error::from)?;
-            line.trim().to_string()
-        }
-    };
-
-    ops::registry_login(config, token, registry)?;
+    ops::registry_login(
+        config,
+        args.value_of("token").map(String::from),
+        args.value_of("registry").map(String::from),
+    )?;
     Ok(())
 }
diff --git a/src/bin/cargo/commands/package.rs b/src/bin/cargo/commands/package.rs
@@ -42,7 +42,6 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
             allow_dirty: args.is_present("allow-dirty"),
             target: args.target(),
             jobs: args.jobs()?,
-            registry: None,
         },
     )?;
     Ok(())
diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs
@@ -26,7 +26,6 @@ pub struct PackageOpts<'cfg> {
     pub verify: bool,
     pub jobs: Option<u32>,
     pub target: Option<String>,
-    pub registry: Option<String>,
 }
 
 static VCS_INFO_FILE: &'static str = ".cargo_vcs_info.json";
diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs
@@ -1,5 +1,6 @@
 use std::collections::BTreeMap;
 use std::fs::{self, File};
+use std::io::{self, BufRead};
 use std::iter::repeat;
 use std::str;
 use std::time::Duration;
@@ -65,6 +66,7 @@ pub fn publish(ws: &Workspace<'_>, opts: &PublishOpts<'_>) -> CargoResult<()> {
         opts.token.clone(),
         opts.index.clone(),
         opts.registry.clone(),
+        true,
     )?;
     verify_dependencies(pkg, reg_id)?;
 
@@ -80,7 +82,6 @@ pub fn publish(ws: &Workspace<'_>, opts: &PublishOpts<'_>) -> CargoResult<()> {
             allow_dirty: opts.allow_dirty,
             target: opts.target.clone(),
             jobs: opts.jobs,
-            registry: opts.registry.clone(),
         },
     )?
     .unwrap();
@@ -320,6 +321,7 @@ pub fn registry(
     token: Option<String>,
     index: Option<String>,
     registry: Option<String>,
+    force_update: bool,
 ) -> CargoResult<(Registry, SourceId)> {
     // Parse all configuration options
     let RegistryConfig {
@@ -330,9 +332,17 @@ pub fn registry(
     let sid = get_source_id(config, index_config.or(index), registry)?;
     let api_host = {
         let mut src = RegistrySource::remote(sid, config);
-        src.update()
-            .chain_err(|| format!("failed to update {}", sid))?;
-        (src.config()?).unwrap().api.unwrap()
+        // Only update the index if the config is not available or `force` is set.
+        let cfg = src.config();
+        let cfg = if force_update || cfg.is_err() {
+            src.update()
+                .chain_err(|| format!("failed to update {}", sid))?;
+            cfg.or_else(|_| src.config())?
+        } else {
+            cfg.unwrap()
+        };
+        cfg.and_then(|cfg| cfg.api)
+            .ok_or_else(|| failure::format_err!("{} does not support API commands", sid))?
     };
     let handle = http_handle(config)?;
     Ok((Registry::new_handle(api_host, token, handle), sid))
@@ -503,18 +513,51 @@ fn http_proxy_exists(config: &Config) -> CargoResult<bool> {
     }
 }
 
-pub fn registry_login(config: &Config, token: String, registry: Option<String>) -> CargoResult<()> {
+pub fn registry_login(
+    config: &Config,
+    token: Option<String>,
+    reg: Option<String>,
+) -> CargoResult<()> {
+    let (registry, _) = registry(config, token.clone(), None, reg.clone(), false)?;
+
+    let token = match token {
+        Some(token) => token,
+        None => {
+            println!(
+                "please visit {}/me and paste the API Token below",
+                registry.host()
+            );
+            let mut line = String::new();
+            let input = io::stdin();
+            input
+                .lock()
+                .read_line(&mut line)
+                .chain_err(|| "failed to read stdin")
+                .map_err(failure::Error::from)?;
+            line.trim().to_string()
+        }
+    };
+
     let RegistryConfig {
         token: old_token, ..
-    } = registry_configuration(config, registry.clone())?;
+    } = registry_configuration(config, reg.clone())?;
 
     if let Some(old_token) = old_token {
         if old_token == token {
+            config.shell().status("Login", "already logged in")?;
             return Ok(());
         }
     }
 
-    config::save_credentials(config, token, registry)
+    config::save_credentials(config, token, reg.clone())?;
+    config.shell().status(
+        "Login",
+        format!(
+            "token for `{}` saved",
+            reg.as_ref().map_or("crates.io", String::as_str)
+        ),
+    )?;
+    Ok(())
 }
 
 pub struct OwnersOptions {
@@ -542,6 +585,7 @@ pub fn modify_owners(config: &Config, opts: &OwnersOptions) -> CargoResult<()> {
         opts.token.clone(),
         opts.index.clone(),
         opts.registry.clone(),
+        true,
     )?;
 
     if let Some(ref v) = opts.to_add {
@@ -602,7 +646,7 @@ pub fn yank(
         None => failure::bail!("a version must be specified to yank"),
     };
 
-    let (mut registry, _) = registry(config, token, index, reg)?;
+    let (mut registry, _) = registry(config, token, index, reg, true)?;
 
     if undo {
         config
@@ -658,22 +702,7 @@ pub fn search(
         prefix
     }
 
-    let sid = get_source_id(config, index, reg)?;
-
-    let mut regsrc = RegistrySource::remote(sid, config);
-    let cfg = match regsrc.config() {
-        Ok(c) => c,
-        Err(_) => {
-            regsrc
-                .update()
-                .chain_err(|| format!("failed to update {}", &sid))?;
-            regsrc.config()?
-        }
-    };
-
-    let api_host = cfg.unwrap().api.unwrap();
-    let handle = http_handle(config)?;
-    let mut registry = Registry::new_handle(api_host, None, handle);
+    let (mut registry, _) = registry(config, None, index, reg, false)?;
     let (crates, total_crates) = registry
         .search(query, limit)
         .chain_err(|| "failed to retrieve search results from the registry")?;
diff --git a/src/cargo/sources/registry/mod.rs b/src/cargo/sources/registry/mod.rs
@@ -211,6 +211,7 @@ pub struct RegistryConfig {
 
     /// API endpoint for the registry. This is what's actually hit to perform
     /// operations like yanks, owner modifications, publish new crates, etc.
+    /// If this is None, the registry does not support API commands.
     pub api: Option<String>,
 }
 
diff --git a/src/crates-io/lib.rs b/src/crates-io/lib.rs
@@ -15,8 +15,12 @@ use url::percent_encoding::{percent_encode, QUERY_ENCODE_SET};
 pub type Result<T> = std::result::Result<T, failure::Error>;
 
 pub struct Registry {
+    /// The base URL for issuing API requests.
     host: String,
+    /// Optional authorization token.
+    /// If None, commands requiring authorization will fail.
     token: Option<String>,
+    /// Curl handle for issuing requests.
     handle: Easy,
 }
 
@@ -51,7 +55,8 @@ pub struct NewCrate {
     pub license_file: Option<String>,
     pub repository: Option<String>,
     pub badges: BTreeMap<String, BTreeMap<String, String>>,
-    #[serde(default)] pub links: Option<String>,
+    #[serde(default)]
+    pub links: Option<String>,
 }
 
 #[derive(Serialize)]
@@ -131,6 +136,10 @@ impl Registry {
         }
     }
 
+    pub fn host(&self) -> &str {
+        &self.host
+    }
+
     pub fn add_owners(&mut self, krate: &str, owners: &[&str]) -> Result<String> {
         let body = serde_json::to_string(&OwnersReq { users: owners })?;
         let body = self.put(&format!("/crates/{}/owners", krate), body.as_bytes())?;
diff --git a/tests/testsuite/alt_registry.rs b/tests/testsuite/alt_registry.rs
@@ -1,6 +1,6 @@
 use crate::support::registry::{self, alt_api_path, Package};
-use crate::support::{basic_manifest, paths, project};
-use std::fs::File;
+use crate::support::{basic_manifest, git, paths, project};
+use std::fs::{self, File};
 use std::io::Write;
 
 #[test]
@@ -203,13 +203,13 @@ fn depend_on_alt_registry_depends_on_crates_io() {
 
     p.cargo("build")
         .masquerade_as_nightly_cargo()
-        .with_stderr(&format!(
+        .with_stderr_unordered(&format!(
             "\
 [UPDATING] `{alt_reg}` index
 [UPDATING] `{reg}` index
 [DOWNLOADING] crates ...
-[DOWNLOADED] [..] v0.0.1 (registry `[ROOT][..]`)
-[DOWNLOADED] [..] v0.0.1 (registry `[ROOT][..]`)
+[DOWNLOADED] baz v0.0.1 (registry `[ROOT][..]`)
+[DOWNLOADED] bar v0.0.1 (registry `[ROOT][..]`)
 [COMPILING] baz v0.0.1 (registry `[ROOT][..]`)
 [COMPILING] bar v0.0.1 (registry `[ROOT][..]`)
 [COMPILING] foo v0.0.1 ([CWD])
@@ -605,3 +605,93 @@ Caused by:
             .run();
     }
 }
+
+#[test]
+fn no_api() {
+    Package::new("bar", "0.0.1").alternative(true).publish();
+    // Configure without `api`.
+    let repo = git2::Repository::open(registry::alt_registry_path()).unwrap();
+    let cfg_path = registry::alt_registry_path().join("config.json");
+    fs::write(
+        cfg_path,
+        format!(r#"{{"dl": "{}"}}"#, registry::alt_dl_url()),
+    )
+    .unwrap();
+    git::add(&repo);
+    git::commit(&repo);
+
+    // First check that a dependency works.
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+             cargo-features = ["alternative-registries"]
+             [package]
+             name = "foo"
+             version = "0.0.1"
+
+             [dependencies.bar]
+             version = "0.0.1"
+             registry = "alternative"
+         "#,
+        )
+        .file("src/lib.rs", "")
+        .build();
+
+    p.cargo("build")
+        .masquerade_as_nightly_cargo()
+        .with_stderr(&format!(
+            "\
+[UPDATING] `{reg}` index
+[DOWNLOADING] crates ...
+[DOWNLOADED] bar v0.0.1 (registry `[ROOT][..]`)
+[COMPILING] bar v0.0.1 (registry `[ROOT][..]`)
+[COMPILING] foo v0.0.1 ([CWD])
+[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]s
+",
+            reg = registry::alt_registry_path().to_str().unwrap()
+        ))
+        .run();
+
+    // Check all of the API commands.
+    let err = format!(
+        "[ERROR] registry `{}` does not support API commands",
+        registry::alt_registry_path().display()
+    );
+
+    p.cargo("login --registry alternative TOKEN -Zunstable-options")
+        .masquerade_as_nightly_cargo()
+        .with_status(101)
+        .with_stderr_contains(&err)
+        .run();
+
+    p.cargo("publish --registry alternative -Zunstable-options")
+        .masquerade_as_nightly_cargo()
+        .with_status(101)
+        .with_stderr_contains(&err)
+        .run();
+
+    p.cargo("search --registry alternative -Zunstable-options")
+        .masquerade_as_nightly_cargo()
+        .with_status(101)
+        .with_stderr_contains(&err)
+        .run();
+
+    p.cargo("owner --registry alternative -Zunstable-options --list")
+        .masquerade_as_nightly_cargo()
+        .with_status(101)
+        .with_stderr_contains(&err)
+        .run();
+
+    p.cargo("yank --registry alternative -Zunstable-options --vers=0.0.1 bar")
+        .masquerade_as_nightly_cargo()
+        .with_status(101)
+        .with_stderr_contains(&err)
+        .run();
+
+    p.cargo("yank --registry alternative -Zunstable-options --vers=0.0.1 bar")
+        .masquerade_as_nightly_cargo()
+        .with_stderr_contains(&err)
+        .with_status(101)
+        .run();
+}
diff --git a/tests/testsuite/login.rs b/tests/testsuite/login.rs
diff --git a/tests/testsuite/main.rs b/tests/testsuite/main.rs
diff --git a/tests/testsuite/registry.rs b/tests/testsuite/registry.rs

Original file line number	Diff line number	Diff line change
`@@ -26,7 +26,6 @@ pub struct PackageOpts<'cfg> {`
`26`	`26`	`pub verify: bool,`
`27`	`27`	`pub jobs: Option<u32>,`
`28`	`28`	`pub target: Option<String>,`
`29`		`- pub registry: Option<String>,`
`30`	`29`	`}`
`31`	`30`
`32`	`31`	`static VCS_INFO_FILE: &'static str = ".cargo_vcs_info.json";`
Original file line number	Diff line number	Diff line change
`@@ -211,6 +211,7 @@ pub struct RegistryConfig {`
`211`	`211`
`212`	`212`	`/// API endpoint for the registry. This is what's actually hit to perform`
`213`	`213`	`/// operations like yanks, owner modifications, publish new crates, etc.`
	`214`	`+ /// If this is None, the registry does not support API commands.`
`214`	`215`	`pub api: Option<String>,`
`215`	`216`	`}`
`216`	`217`