Skip to content

Commit

Permalink
Auto merge of rust-lang#3330 - RossSmyth:win-fmt, r=RalfJung
Browse files Browse the repository at this point in the history
Fix .\miri fmt on Windows

This allows .\miri fmt to work on Windows. Closes rust-lang#3317.

To reiterate, the problem with using `miri fmt` on Windows is that the CLI arguments to rustfmt are too long. Currently over 65,000 characters are used in the call to rustfmt, [which is incompatible with Windows](https://devblogs.microsoft.com/oldnewthing/20031210-00/?p=41553) as it is limited to (2^15 - 1) for all arguments plus all env vars.

Two things are done do get around this limit:

1. Call out to cargo-fmt for the crates that exist.
2. Batch rustfmt calls by length

Another alternative would be to just use rustfmt for everything and don't use cargo-fmt for the crates.

I don't know how much you guys may care about `miri fmt` time to run. I don't know the diff as it did not work before on my computer.

[I have another branch that solves this, but in a less permanent way](RossSmyth/miri/tree/windows-fmt). That was my initial attempt, and I learned that even with cargo-fmt and relative paths, the rustfmt call still has 27k characters. This is closer to the limit than I expected, so it would not be a permanent solution. So I went back to absolute paths & batching.
  • Loading branch information
bors committed Mar 3, 2024
2 parents f51f923 + 83e2e2d commit f47732b
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 30 deletions.
46 changes: 18 additions & 28 deletions src/tools/miri/miri-script/src/commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -526,37 +526,27 @@ impl Command {
}

fn fmt(flags: Vec<OsString>) -> Result<()> {
use itertools::Itertools;

let e = MiriEnv::new()?;
let toolchain = &e.toolchain;
let config_path = path!(e.miri_dir / "rustfmt.toml");

let mut cmd = cmd!(
e.sh,
"rustfmt +{toolchain} --edition=2021 --config-path {config_path} --unstable-features --skip-children {flags...}"
);
eprintln!("$ {cmd} ...");

// Add all the filenames to the command.
// FIXME: `rustfmt` will follow the `mod` statements in these files, so we get a bunch of
// duplicate diffs.
for item in WalkDir::new(&e.miri_dir).into_iter().filter_entry(|entry| {
let name = entry.file_name().to_string_lossy();
let ty = entry.file_type();
if ty.is_file() {
name.ends_with(".rs")
} else {
// dir or symlink. skip `target` and `.git`.
&name != "target" && &name != ".git"
}
}) {
let item = item?;
if item.file_type().is_file() {
cmd = cmd.arg(item.into_path());
}
}
// Collect each rust file in the miri repo.
let files = WalkDir::new(&e.miri_dir)
.into_iter()
.filter_entry(|entry| {
let name = entry.file_name().to_string_lossy();
let ty = entry.file_type();
if ty.is_file() {
name.ends_with(".rs")
} else {
// dir or symlink. skip `target` and `.git`.
&name != "target" && &name != ".git"
}
})
.filter_ok(|item| item.file_type().is_file())
.map_ok(|item| item.into_path());

// We want our own error message, repeating the command is too much.
cmd.quiet().run().map_err(|_| anyhow!("`rustfmt` failed"))?;
Ok(())
e.format_files(files, &e.toolchain[..], &config_path, &flags[..])
}
}
48 changes: 46 additions & 2 deletions src/tools/miri/miri-script/src/util.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::ffi::{OsStr, OsString};
use std::path::PathBuf;
use std::path::{Path, PathBuf};

use anyhow::{Context, Result};
use anyhow::{anyhow, Context, Result};
use dunce::canonicalize;
use path_macro::path;
use xshell::{cmd, Shell};
Expand Down Expand Up @@ -145,4 +145,48 @@ impl MiriEnv {
.run()?;
Ok(())
}

/// Receives an iterator of files.
/// Will format each file with the miri rustfmt config.
/// Does not recursively format modules.
pub fn format_files(
&self,
files: impl Iterator<Item = Result<PathBuf, walkdir::Error>>,
toolchain: &str,
config_path: &Path,
flags: &[OsString],
) -> anyhow::Result<()> {
use itertools::Itertools;

let mut first = true;

// Format in batches as not all our files fit into Windows' command argument limit.
for batch in &files.chunks(256) {
// Build base command.
let mut cmd = cmd!(
self.sh,
"rustfmt +{toolchain} --edition=2021 --config-path {config_path} --unstable-features --skip-children {flags...}"
);
if first {
// Log an abbreviating command, and only once.
eprintln!("$ {cmd} ...");
first = false;
}
// Add files.
for file in batch {
// Make it a relative path so that on platforms with extremely tight argument
// limits (like Windows), we become immune to someone cloning the repo
// 50 directories deep.
let file = file?;
let file = file.strip_prefix(&self.miri_dir)?;
cmd = cmd.arg(file);
}

// Run rustfmt.
// We want our own error message, repeating the command is too much.
cmd.quiet().run().map_err(|_| anyhow!("`rustfmt` failed"))?;
}

Ok(())
}
}

0 comments on commit f47732b

Please sign in to comment.