Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrate Clippy into the project #1724

Merged
merged 6 commits into from
Sep 26, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions collector/benchlib/src/benchmark.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,15 @@ impl<'a> BenchmarkGroup<'a> {

fn profile_benchmark(self, args: ProfileArgs) -> anyhow::Result<()> {
let Some(benchmark) = self.benchmarks.get(args.benchmark.as_str()) else {
return Err(anyhow::anyhow!("Benchmark `{}` not found. Available benchmarks: {}", args.benchmark,
self.benchmarks.keys().map(|s| s.to_string()).collect::<Vec<_>>().join(", ")));
return Err(anyhow::anyhow!(
"Benchmark `{}` not found. Available benchmarks: {}",
args.benchmark,
self.benchmarks
.keys()
.map(|s| s.to_string())
.collect::<Vec<_>>()
.join(", ")
));
};
(benchmark.profile_fn)();

Expand Down
39 changes: 22 additions & 17 deletions collector/src/bin/collector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ use collector::runtime::{
use collector::runtime::{profile_runtime, RuntimeCompilationOpts};
use collector::toolchain::{
create_toolchain_from_published_version, get_local_toolchain, Sysroot, Toolchain,
ToolchainConfig,
};
use collector::utils::cachegrind::cachegrind_diff;
use collector::utils::{is_installed, wait_for_future};
Expand Down Expand Up @@ -375,6 +376,10 @@ struct CompileTimeOptions {
/// The path to the local rustdoc to measure
#[arg(long)]
rustdoc: Option<PathBuf>,

/// The path to the local clippy to measure
#[arg(long)]
clippy: Option<PathBuf>,
}

#[derive(Debug, clap::Args)]
Expand Down Expand Up @@ -660,9 +665,7 @@ fn main_result() -> anyhow::Result<i32> {
let toolchain = get_local_toolchain(
&[Profile::Opt],
rustc,
None,
None,
None,
ToolchainConfig::default(),
id,
target_triple.clone(),
)?;
Expand Down Expand Up @@ -718,9 +721,7 @@ fn main_result() -> anyhow::Result<i32> {
let toolchain = get_local_toolchain(
&[Profile::Opt],
rustc,
None,
None,
None,
ToolchainConfig::default(),
id,
target_triple.clone(),
)?;
Expand Down Expand Up @@ -756,9 +757,11 @@ fn main_result() -> anyhow::Result<i32> {
let toolchain = get_local_toolchain(
&profiles,
&local.rustc,
opts.rustdoc.as_deref(),
local.cargo.as_deref(),
local.id.as_deref(),
*ToolchainConfig::default()
.rustdoc(opts.rustdoc.as_deref())
.clippy(opts.clippy.as_deref())
.cargo(local.cargo.as_deref())
.id(local.id.as_deref()),
"",
target_triple,
)?;
Expand Down Expand Up @@ -948,9 +951,11 @@ fn main_result() -> anyhow::Result<i32> {
let toolchain = get_local_toolchain(
profiles,
rustc,
opts.rustdoc.as_deref(),
local.cargo.as_deref(),
local.id.as_deref(),
*ToolchainConfig::default()
.rustdoc(opts.rustdoc.as_deref())
.clippy(opts.clippy.as_deref())
.cargo(local.cargo.as_deref())
.id(local.id.as_deref()),
suffix,
target_triple.clone(),
)?;
Expand Down Expand Up @@ -1062,9 +1067,9 @@ fn get_local_toolchain_for_runtime_benchmarks(
get_local_toolchain(
&[Profile::Opt],
&local.rustc,
None,
local.cargo.as_deref(),
local.id.as_deref(),
*ToolchainConfig::default()
.cargo(local.cargo.as_deref())
.id(local.id.as_deref()),
"",
target_triple.to_string(),
)
Expand Down Expand Up @@ -1193,9 +1198,9 @@ fn bench_published_artifact(
let artifact_id = ArtifactId::Tag(toolchain.id.clone());

let profiles = if collector::version_supports_doc(&toolchain.id) {
Profile::all()
vec![Profile::Check, Profile::Debug, Profile::Doc, Profile::Opt]
} else {
Profile::all_non_doc()
vec![Profile::Check, Profile::Debug, Profile::Opt]
};
let scenarios = if collector::version_supports_incremental(&toolchain.id) {
Scenario::all()
Expand Down
3 changes: 3 additions & 0 deletions collector/src/bin/rustc-fake.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,11 @@ fn main() {
let mut args = args_os.collect::<Vec<_>>();
let rustc = env::var_os("RUSTC_REAL").unwrap();
let actually_rustdoc = name.ends_with("rustdoc-fake");
let actually_clippy = name.ends_with("clippy-fake");
let tool = if actually_rustdoc {
env::var_os("RUSTDOC_REAL").unwrap()
} else if actually_clippy {
env::var_os("CLIPPY_REAL").unwrap()
} else {
rustc
};
Expand Down
2 changes: 2 additions & 0 deletions collector/src/compile/benchmark/profile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,15 @@ pub enum Profile {
Debug,
Doc,
Opt,
Clippy,
}

impl Profile {
pub fn all() -> Vec<Self> {
Kobzol marked this conversation as resolved.
Show resolved Hide resolved
vec![Profile::Check, Profile::Debug, Profile::Doc, Profile::Opt]
}

// This also leaves Clippy out
pub fn all_non_doc() -> Vec<Self> {
vec![Profile::Check, Profile::Debug, Profile::Opt]
}
Expand Down
1 change: 1 addition & 0 deletions collector/src/compile/execute/bencher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ impl<'a> BenchProcessor<'a> {
Profile::Debug => database::Profile::Debug,
Profile::Doc => database::Profile::Doc,
Profile::Opt => database::Profile::Opt,
Profile::Clippy => database::Profile::Clippy,
};

if let Some(files) = stats.2 {
Expand Down
22 changes: 21 additions & 1 deletion collector/src/compile/execute/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ impl PerfTool {
}
ProfileTool(LlvmLines) => match profile {
Profile::Debug | Profile::Opt => Some("llvm-lines"),
Profile::Check | Profile::Doc => None,
Profile::Check | Profile::Doc | Profile::Clippy => None,
},
}
}
Expand Down Expand Up @@ -168,6 +168,10 @@ impl<'a> CargoProcess<'a> {

if let Some(r) = &self.toolchain.components.rustdoc {
cmd.env("RUSTDOC", &*FAKE_RUSTDOC).env("RUSTDOC_REAL", r);
};

if let Some(c) = &self.toolchain.components.clippy {
cmd.env("CLIPPY", &*FAKE_CLIPPY).env("CLIPPY_REAL", c);
}
cmd
}
Expand Down Expand Up @@ -236,6 +240,7 @@ impl<'a> CargoProcess<'a> {
}
Profile::Debug => {}
Profile::Doc => {}
Profile::Clippy => {}
Profile::Opt => {
cmd.arg("--release");
}
Expand Down Expand Up @@ -354,6 +359,21 @@ lazy_static::lazy_static! {
}
fake_rustdoc
};
static ref FAKE_CLIPPY: PathBuf = {
let mut fake_clippy = env::current_exe().unwrap();
fake_clippy.pop();
fake_clippy.push("clippy-fake");
// link from rustc-fake to rustdoc-fake
if !fake_clippy.exists() {
#[cfg(unix)]
use std::os::unix::fs::symlink;
#[cfg(windows)]
use std::os::windows::fs::symlink_file as symlink;

symlink(&*FAKE_RUSTC, &fake_clippy).expect("failed to make symbolic link");
}
fake_clippy
};
}

/// Used to indicate if we need to retry a run.
Expand Down
85 changes: 75 additions & 10 deletions collector/src/toolchain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,10 @@ impl SysrootDownload {
let components = ToolchainComponents::from_binaries_and_libdir(
sysroot_bin("rustc")?,
Some(sysroot_bin("rustdoc")?),
match sysroot_bin("cargo-clippy") {
Kobzol marked this conversation as resolved.
Show resolved Hide resolved
Err(_) => None,
Ok(path) => Some(path),
},
sysroot_bin("cargo")?,
&self.directory.join(&self.rust_sha).join("lib"),
)?;
Expand Down Expand Up @@ -241,6 +245,7 @@ impl Toolchain {
pub struct ToolchainComponents {
pub rustc: PathBuf,
pub rustdoc: Option<PathBuf>,
pub clippy: Option<PathBuf>,
pub cargo: PathBuf,
pub lib_rustc: Option<PathBuf>,
pub lib_std: Option<PathBuf>,
Expand All @@ -252,12 +257,14 @@ impl ToolchainComponents {
fn from_binaries_and_libdir(
rustc: PathBuf,
rustdoc: Option<PathBuf>,
clippy: Option<PathBuf>,
cargo: PathBuf,
libdir: &Path,
) -> anyhow::Result<Self> {
let mut component = ToolchainComponents {
rustc,
rustdoc,
clippy,
cargo,
..Default::default()
};
Expand Down Expand Up @@ -288,6 +295,36 @@ impl ToolchainComponents {
}
}

#[derive(Clone, Copy, Default)]
pub struct ToolchainConfig<'a> {
rustdoc: Option<&'a Path>,
clippy: Option<&'a Path>,
cargo: Option<&'a Path>,
id: Option<&'a str>,
}

impl<'a> ToolchainConfig<'a> {
pub fn rustdoc(&mut self, rustdoc: Option<&'a Path>) -> &mut Self {
self.rustdoc = rustdoc;
self
}

pub fn clippy(&mut self, clippy: Option<&'a Path>) -> &mut Self {
self.clippy = clippy;
self
}

pub fn cargo(&mut self, cargo: Option<&'a Path>) -> &mut Self {
self.cargo = cargo;
self
}

pub fn id(&mut self, id: Option<&'a str>) -> &mut Self {
self.id = id;
self
}
}

/// Get a toolchain from the input.
/// - `rustc`: check if the given one is acceptable.
/// - `rustdoc`: if one is given, check if it is acceptable. Otherwise, if
Expand All @@ -297,9 +334,7 @@ impl ToolchainComponents {
pub fn get_local_toolchain(
profiles: &[Profile],
rustc: &str,
rustdoc: Option<&Path>,
cargo: Option<&Path>,
id: Option<&str>,
toolchain_config: ToolchainConfig<'_>,
id_suffix: &str,
target_triple: String,
) -> anyhow::Result<Toolchain> {
Expand Down Expand Up @@ -354,7 +389,7 @@ pub fn get_local_toolchain(
debug!("found rustc: {:?}", &rustc);

// When the id comes from a +toolchain, the suffix is *not* added.
let id = if let Some(id) = id {
let id = if let Some(id) = toolchain_config.id {
let mut id = id.to_owned();
id.push_str(id_suffix);
id
Expand All @@ -369,7 +404,7 @@ pub fn get_local_toolchain(

// When specifying rustc via a path, the suffix is always added to the
// id.
let mut id = if let Some(id) = id {
let mut id = if let Some(id) = toolchain_config.id {
id.to_owned()
} else {
"Id".to_string()
Expand All @@ -380,7 +415,7 @@ pub fn get_local_toolchain(
};

let rustdoc =
if let Some(rustdoc) = &rustdoc {
if let Some(rustdoc) = &toolchain_config.rustdoc {
Some(rustdoc.canonicalize().with_context(|| {
format!("failed to canonicalize rustdoc executable {:?}", rustdoc)
})?)
Expand All @@ -400,7 +435,28 @@ pub fn get_local_toolchain(
None
};

let cargo = if let Some(cargo) = &cargo {
let clippy = if let Some(clippy) = &toolchain_config.clippy {
Some(
clippy.canonicalize().with_context(|| {
format!("failed to canonicalize clippy executable {:?}", clippy)
})?,
)
} else if profiles.contains(&Profile::Clippy) {
// We need a `clippy`. Look for one next to `rustc`.
if let Ok(clippy) = rustc.with_file_name("cargo-clippy").canonicalize() {
debug!("found clippy: {:?}", &clippy);
Some(clippy)
} else {
anyhow::bail!(
"'Clippy' build specified but '--cargo-clippy' not specified and no 'cargo-clippy' found \
next to 'rustc'"
);
}
} else {
// No `clippy` provided, but none needed.
None
};
let cargo = if let Some(cargo) = &toolchain_config.cargo {
cargo
.canonicalize()
.with_context(|| format!("failed to canonicalize cargo executable {:?}", cargo))?
Expand Down Expand Up @@ -428,7 +484,9 @@ pub fn get_local_toolchain(
let lib_dir = get_lib_dir_from_rustc(&rustc).context("Cannot find libdir for rustc")?;

Ok(Toolchain {
components: ToolchainComponents::from_binaries_and_libdir(rustc, rustdoc, cargo, &lib_dir)?,
components: ToolchainComponents::from_binaries_and_libdir(
rustc, rustdoc, clippy, cargo, &lib_dir,
)?,
id,
triple: target_triple,
})
Expand Down Expand Up @@ -465,16 +523,23 @@ pub fn create_toolchain_from_published_version(
};
let rustc = which("rustc")?;
let rustdoc = which("rustdoc")?;
let clippy = which("clippy")?;
let cargo = which("cargo")?;

debug!("Found rustc: {}", rustc.display());
debug!("Found rustdoc: {}", rustdoc.display());
debug!("Found clippy: {}", clippy.display());
debug!("Found cargo: {}", cargo.display());

let lib_dir = get_lib_dir_from_rustc(&rustc)?;

let components =
ToolchainComponents::from_binaries_and_libdir(rustc, Some(rustdoc), cargo, &lib_dir)?;
let components = ToolchainComponents::from_binaries_and_libdir(
rustc,
Some(rustdoc),
Some(clippy),
cargo,
&lib_dir,
)?;

Ok(Toolchain {
components,
Expand Down
Loading
Loading