Skip to content

Commit

Permalink
Auto merge of #10918 - ehuss:fix-formats_source, r=Eh2406
Browse files Browse the repository at this point in the history
Fix formats_source test requiring rustfmt.

The requirements added in #9892 that `rustfmt` must be present doesn't work in the `rust-lang/rust` environment. There are two issues:

* Cargo is run without using rustup. If you also have rustup installed, the test will fail because the `rustfmt` binary found in `PATH` will fail to choose a toolchain because HOME points to the sandbox home which does not have a rustup configuration.
* rust-lang/rust CI uninstalls rustup, and does not have rustfmt in PATH at all.  It is not practical to make rustfmt available there.

The solution here is to just revert the behavior back to where it was where it checks if it can run `rustfmt` in the sandbox. This should work for anyone who has a normal rustup installation (including Cargo's CI). If running the testsuite without rustup, then the test will be skipped.

This also includes a small enhancement to provide better error information when rustfmt fails.
  • Loading branch information
bors committed Aug 2, 2022
2 parents 3ccf7dc + f62ce1e commit 55b3073
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 12 deletions.
17 changes: 7 additions & 10 deletions src/cargo/ops/cargo_new.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@ use std::collections::BTreeMap;
use std::fmt;
use std::io::{BufRead, BufReader, ErrorKind};
use std::path::{Path, PathBuf};
use std::process::Command;
use std::str::{from_utf8, FromStr};
use std::str::FromStr;
use toml_edit::easy as toml;

#[derive(Clone, Copy, Debug, PartialEq)]
Expand Down Expand Up @@ -830,14 +829,12 @@ mod tests {
paths::write(&path_of_source_file, default_file_content)?;

// Format the newly created source file
match Command::new("rustfmt").arg(&path_of_source_file).output() {
Err(e) => log::warn!("failed to call rustfmt: {}", e),
Ok(output) => {
if !output.status.success() {
log::warn!("rustfmt failed: {:?}", from_utf8(&output.stdout));
}
}
};
if let Err(e) = cargo_util::ProcessBuilder::new("rustfmt")
.arg(&path_of_source_file)
.exec_with_output()
{
log::warn!("failed to call rustfmt: {:#}", e);
}
}
}

Expand Down
12 changes: 10 additions & 2 deletions tests/testsuite/init/formats_source/mod.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,19 @@
use cargo_test_support::compare::assert_ui;
use cargo_test_support::prelude::*;
use cargo_test_support::Project;
use cargo_test_support::{process, Project};

use cargo_test_support::curr_dir;

#[cargo_test(requires_rustfmt)]
#[cargo_test]
fn formats_source() {
// This cannot use `requires_rustfmt` because rustfmt is not available in
// the rust-lang/rust environment. Additionally, if running cargo without
// rustup (but with rustup installed), this test also fails due to HOME
// preventing the proxy from choosing a toolchain.
if let Err(e) = process("rustfmt").arg("-V").exec_with_output() {
eprintln!("skipping test, rustfmt not available:\n{e:?}");
return;
}
let project = Project::from_template(curr_dir!().join("in"));
let project_root = &project.root();

Expand Down

0 comments on commit 55b3073

Please sign in to comment.