correctly handle cargo +toolchain public-api

cargo-public-api/src/main.rs

@@ -113,10 +113,13 @@ pub struct Args {
     verbose: bool,
 
     /// Allows you to build rustdoc JSON with a toolchain other than `+nightly`.
+    ///
+    /// Consider using `cargo +toolchain public-api` instead.
+    ///
     /// Useful if you have built a toolchain from source for example, or if you
     /// want to use a fixed toolchain in CI.
-    #[clap(long, default_value = "+nightly")]
-    toolchain: String,
+    #[clap(long)]
+    toolchain: Option<String>,
 
     /// Build for the target triple
     #[clap(long)]
@@ -153,7 +156,12 @@ struct PostProcessing {
 }
 
 fn main_() -> Result<()> {
-    let args = get_args();
+    let mut args = get_args();
+
+    // check if using a stable compiler, and use nightly if it is.
+    if probably_stable() {
+        args.toolchain = Some("+nightly".to_owned());
+    }
 
     let post_processing = if let Some(commits) = &args.diff_git_checkouts {
         print_diff_between_two_commits(&args, commits)?
@@ -166,6 +174,23 @@ fn main_() -> Result<()> {
     post_processing.perform(&args)
 }
 
+fn probably_stable() -> bool {
+    let mut cmd = std::process::Command::new("cargo");
+    cmd.arg("--version");
+
+    let output = match cmd.output() {
+        Ok(output) => output,
+        Err(_) => return false,
+    };
+
+    let version = match String::from_utf8(output.stdout) {
+        Ok(version) => version,
+        Err(_) => return false,
+    };
+
+    version.starts_with("cargo 1") && !version.contains("nightly")
+}
+
 fn check_diff(args: &Args, diff: &Option<PublicItemsDiff>) -> Result<()> {
     match (&args.deny, diff) {
         // We were requested to deny diffs, so make sure there is no diff
@@ -199,7 +224,7 @@ fn check_diff(args: &Args, diff: &Option<PublicItemsDiff>) -> Result<()> {
 }
 
 fn print_public_items_of_current_commit(args: &Args) -> Result<PostProcessing> {
-    let (public_items, branch_to_restore) = collect_public_api_from_commit(None)?;
+    let (public_items, branch_to_restore) = collect_public_api_from_commit(args, None)?;
 
     if args.verbose {
         public_items.missing_item_ids.iter().for_each(|i| {
@@ -219,10 +244,10 @@ fn print_public_items_of_current_commit(args: &Args) -> Result<PostProcessing> {
 
 fn print_diff_between_two_commits(args: &Args, commits: &[String]) -> Result<PostProcessing> {
     let old_commit = commits.get(0).expect("clap makes sure first commit exist");
-    let (old, branch_to_restore) = collect_public_api_from_commit(Some(old_commit))?;
+    let (old, branch_to_restore) = collect_public_api_from_commit(args, Some(old_commit))?;
 
     let new_commit = commits.get(1).expect("clap makes sure second commit exist");
-    let (new, _) = collect_public_api_from_commit(Some(new_commit))?;
+    let (new, _) = collect_public_api_from_commit(args, Some(new_commit))?;
 
     let diff_to_check = Some(print_diff(args, old.items, new.items)?);
 
@@ -301,9 +326,10 @@ fn get_options(args: &Args) -> Options {
 /// Collects public items from either the current commit or a given commit. If
 /// `commit` is `Some` and thus a `git checkout` will be made, also return the
 /// original branch.
-fn collect_public_api_from_commit(commit: Option<&str>) -> Result<(PublicApi, Option<String>)> {
-    let args = get_args();
-
+fn collect_public_api_from_commit(
+    args: &Args,
+    commit: Option<&str>,
+) -> Result<(PublicApi, Option<String>)> {
     // Do a git checkout of a specific commit unless we are supposed to simply
     // use the current commit
     let original_branch = if let Some(commit) = commit {
@@ -315,9 +341,8 @@ fn collect_public_api_from_commit(commit: Option<&str>) -> Result<(PublicApi, Op
     } else {
         None
     };
-
     let mut build_options = BuildOptions::default()
-        .toolchain(&args.toolchain)
+        .toolchain(args.toolchain.clone())
         .manifest_path(&args.manifest_path)
         .all_features(args.all_features)
         .no_default_features(args.no_default_features)
@@ -337,7 +362,7 @@ fn collect_public_api_from_commit(commit: Option<&str>) -> Result<(PublicApi, Op
     if args.verbose {
         println!("Processing {:?}", json_path);
     }
-    let options = get_options(&args);
+    let options = get_options(args);
 
     Ok((
         public_api_from_rustdoc_json_path(json_path, options)?,

docs/CONTRIBUTING.md

@@ -61,7 +61,7 @@ You can also combine both ways:
 If you have built rustdoc yourself to try some rustdoc JSON fix, you can run `cargo public-api` with your [custom toolchain](https://rustc-dev-guide.rust-lang.org/building/how-to-build-and-run.html#creating-a-rustup-toolchain) like this:
 
 ```
-cargo public-api --toolchain +custom
+cargo +custom public-api
 ```
 
 Another option is the `RUSTDOC_JSON_OVERRIDDEN_TOOLCHAIN_HACK` env var. Use it like this:

public-api/tests/utils/mod.rs

@@ -10,7 +10,7 @@ pub fn rustdoc_json_path_for_crate(test_crate: &str) -> PathBuf {
     // so build quietly to make running tests much less noisy
     rustdoc_json::build(
         BuildOptions::default()
-            .toolchain("+nightly")
+            .toolchain("+nightly".to_owned())
             .manifest_path(&format!("{}/Cargo.toml", test_crate))
             .quiet(true),
     )

rustdoc-json/examples/build-rustdoc-json.rs

@@ -8,7 +8,7 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
     // Build it
     let json_path = rustdoc_json::build(
         BuildOptions::default()
-            .toolchain("+nightly")
+            .toolchain("+nightly".to_owned())
             .manifest_path(&std::env::args().nth(1).unwrap()),
     )?;
     println!("Built and wrote rustdoc JSON to {:?}", &json_path);

rustdoc-json/public-api.txt

@@ -19,7 +19,7 @@ pub fn rustdoc_json::BuildOptions::no_default_features(self, no_default_features
 pub fn rustdoc_json::BuildOptions::package(self, package: impl AsRef<str>) -> Self
 pub fn rustdoc_json::BuildOptions::quiet(self, quiet: bool) -> Self
 pub fn rustdoc_json::BuildOptions::target(self, target: String) -> Self
-pub fn rustdoc_json::BuildOptions::toolchain(self, toolchain: impl AsRef<OsStr>) -> Self
+pub fn rustdoc_json::BuildOptions::toolchain(self, toolchain: impl Into<Option<String>>) -> Self
 pub fn rustdoc_json::build(options: BuildOptions) -> Result<PathBuf, BuildError>
 pub mod rustdoc_json
 pub struct rustdoc_json::BuildOptions

rustdoc-json/src/build.rs

@@ -2,7 +2,6 @@ use super::BuildError;
 use super::BuildOptions;
 
 use std::{
-    ffi::OsStr,
     path::{Path, PathBuf},
     process::Command,
 };
@@ -24,7 +23,7 @@ pub(crate) fn run_cargo_rustdoc(options: BuildOptions) -> Result<PathBuf, BuildE
         )
     } else {
         let manifest = cargo_toml::Manifest::from_path(&options.manifest_path)?;
-        if manifest.workspace.is_some() {
+        if manifest.package.is_none() && manifest.workspace.is_some() {
             Err(BuildError::VirtualManifest(options.manifest_path))
         } else {
             Err(BuildError::General(String::from("See above")))
@@ -50,12 +49,7 @@ fn cargo_rustdoc_command(options: &BuildOptions) -> Command {
     } = options;
     let mut command = Command::new("cargo");
 
-    // These can override our `+nightly` with `+stable` unless we clear them
-    command.env_remove("RUSTDOC");
-    command.env_remove("RUSTC");
-
-    let overridden_toolchain = OVERRIDDEN_TOOLCHAIN.map(OsStr::new);
-    if let Some(toolchain) = overridden_toolchain.or(requested_toolchain.as_deref()) {
+    if let Some(toolchain) = OVERRIDDEN_TOOLCHAIN.or(requested_toolchain.as_deref()) {
         command.arg(toolchain);
     }
 
@@ -147,12 +141,13 @@ impl Default for BuildOptions {
 }
 
 impl BuildOptions {
-    /// Set the toolchain. Default: `None`, which in practice means `"+stable"`.
+    /// Set the toolchain. Default: `None`.
     /// Until rustdoc JSON has stabilized, you will want to set this to
-    /// `"+nightly"` or similar.
+    /// be `"+nightly"` or similar.
+    /// If the toolchain is set as `None`, the current active toolchain will be used.
     #[must_use]
-    pub fn toolchain(mut self, toolchain: impl AsRef<OsStr>) -> Self {
-        self.toolchain = Some(toolchain.as_ref().to_owned());
+    pub fn toolchain(mut self, toolchain: impl Into<Option<String>>) -> Self {
+        self.toolchain = toolchain.into();
         self
     }
 

rustdoc-json/src/lib.rs

@@ -8,7 +8,7 @@
 //!
 //!    let json_path = rustdoc_json::build(
 //!        BuildOptions::default()
-//!            .toolchain("+nightly")
+//!            .toolchain("+nightly".to_owned())
 //!            .manifest_path("Cargo.toml"),
 //!    ).unwrap();
 //!
@@ -21,7 +21,7 @@
 // deny in CI, only warn here
 #![warn(clippy::all, clippy::pedantic, missing_docs)]
 
-use std::{ffi::OsString, path::PathBuf};
+use std::path::PathBuf;
 
 mod build;
 
@@ -55,7 +55,7 @@ pub enum BuildError {
 /// See [crate] for an example on how to use it.
 #[derive(Debug)]
 pub struct BuildOptions {
-    toolchain: Option<OsString>,
+    toolchain: Option<String>,
     manifest_path: std::path::PathBuf,
     target: Option<String>,
     quiet: bool,

scripts/test-invocation-variants.sh

@@ -75,3 +75,7 @@ assert_progress_and_output "cargo public-api"
 
 # Make sure we can run the tool on an external directory as a cargo sub-command
 assert_progress_and_output "cargo public-api --manifest-path $(pwd)/Cargo.toml"
+
+# Make sure we can run the tool with MINIMUM_RUSTDOC_JSON_VERSION
+rustup toolchain install nightly-2022-08-15
+assert_progress_and_output "cargo +nightly-2022-08-15 public-api --manifest-path $(pwd)/Cargo.toml"