From 072e3089079929d539ff5abc47e10cf640e1838c Mon Sep 17 00:00:00 2001 From: Veetaha Date: Wed, 20 Sep 2023 01:25:07 +0200 Subject: [PATCH] Simplify lint crates build to a single `cargo build` invocation `cargo build` can be specified to build the dependent crates only. It means we can use the build workspace to both fetch and build the lint crate dlls with a single `cargo build` command. So instead of ```bash cargo fetch cargo metadata cargo build --manifest-path /path/to/lint-crate/a/Cargo.toml cargo build --manifest-path /path/to/lint-crate/b/Cargo.toml ``` We have just ```bash cargo build -p a -p b ``` This also removes the usage of the unstable `--out-dir` parameter for `cargo build`. --- cargo-marker/src/backend.rs | 14 +- cargo-marker/src/backend/cargo.rs | 9 +- cargo-marker/src/backend/lints.rs | 87 ++++++++--- cargo-marker/src/backend/lints/build.rs | 144 ------------------ cargo-marker/src/backend/lints/build_space.rs | 64 ++++++++ cargo-marker/src/backend/lints/fetch.rs | 143 ----------------- cargo-marker/src/backend/toolchain.rs | 35 +---- marker_uilints/tests/ui/lint_ice_message.rs | 2 +- 8 files changed, 141 insertions(+), 357 deletions(-) delete mode 100644 cargo-marker/src/backend/lints/build.rs create mode 100644 cargo-marker/src/backend/lints/build_space.rs delete mode 100644 cargo-marker/src/backend/lints/fetch.rs diff --git a/cargo-marker/src/backend.rs b/cargo-marker/src/backend.rs index fce4bfbf..ffebaca8 100644 --- a/cargo-marker/src/backend.rs +++ b/cargo-marker/src/backend.rs @@ -4,7 +4,7 @@ //! `cargo-marker` CLI. However, `cargo-marker` might also be used as a library for UI //! tests later down the line. -use self::{lints::LintCrate, toolchain::Toolchain}; +use self::{lints::LintDll, toolchain::Toolchain}; use crate::config::LintDependencyEntry; use crate::error::prelude::*; use crate::observability::display::{self, print_stage}; @@ -50,14 +50,6 @@ impl Config { toolchain, }) } - - fn markers_target_dir(&self) -> Utf8PathBuf { - self.marker_dir.join("target") - } - - fn lint_crate_dir(&self) -> Utf8PathBuf { - self.marker_dir.join("lints") - } } /// This struct contains all information to use rustc as a driver. @@ -68,9 +60,9 @@ pub struct CheckInfo { pub fn prepare_check(config: &Config) -> Result { print_stage("compiling lints"); - let lints = lints::build_lints(config)? + let lints = lints::build(config)? .iter() - .map(|LintCrate { name, file }| format!("{name}:{file}")) + .map(|LintDll { name, file }| format!("{name}:{file}")) .join(";"); #[rustfmt::skip] diff --git a/cargo-marker/src/backend/cargo.rs b/cargo-marker/src/backend/cargo.rs index 49956ed8..b6e6d992 100644 --- a/cargo-marker/src/backend/cargo.rs +++ b/cargo-marker/src/backend/cargo.rs @@ -24,12 +24,9 @@ impl Cargo { } } - /// This returns a command calling rustup, which acts as a proxy for the - /// Cargo of the selected toolchain. - /// It may add additional flags for verbose output. - /// - /// See also [`super::toolchain::Toolchain::cargo_build_command`] if the - /// commands is intended to build a crate. + /// This returns a command calling rustup, which acts as a proxy for + /// `cargo` of the selected toolchain. If no toolchain is selected, it + /// will return a command calling the default `cargo` command. pub fn command(&self) -> Command { // Marker requires rustc's shared libraries to be available. These are // added by rustup, when it acts like a proxy for cargo/rustc/etc. diff --git a/cargo-marker/src/backend/lints.rs b/cargo-marker/src/backend/lints.rs index eb2ba71f..d9a1e65a 100644 --- a/cargo-marker/src/backend/lints.rs +++ b/cargo-marker/src/backend/lints.rs @@ -1,37 +1,80 @@ -use super::Config; +mod build_space; + +use crate::backend::Config; use crate::error::prelude::*; +use crate::observability::prelude::*; use camino::Utf8PathBuf; - -mod build; -mod fetch; - -/// This struct contains all information of a lint crate required to compile -/// the crate. See the [fetch] module for how external crates are fetched and -/// this info is retrieved. -#[derive(Debug)] -pub struct LintCrateSource { - /// The name of the package, for now we can assume that this is the name - /// that will be used to construct the dynamic library. - name: String, - /// The absolute path to the manifest of this lint crate - manifest: Utf8PathBuf, -} +use std::env::consts::{DLL_PREFIX, DLL_SUFFIX}; /// The information of a compiled lint crate. #[derive(Debug)] -pub struct LintCrate { +pub(crate) struct LintDll { /// The name of the crate - pub name: String, + pub(crate) name: String, + /// The absolute path of the compiled crate, as a dynamic library. - pub file: Utf8PathBuf, + pub(crate) file: Utf8PathBuf, } /// This function fetches and builds all lints specified in the given [`Config`] -pub fn build_lints(config: &Config) -> Result> { +pub(crate) fn build(config: &Config) -> Result> { + // Check that there are at least some lints that we don't attempt to + // run a `cargo build` with no packages. + if config.lints.is_empty() { + return Err(Error::root("No lints were specified")); + } + // FIXME(xFrednet): Potentially handle local crates compiled for UI tests // differently. Like running the build command in the project root. This // would allow cargo to cache the compilation better. Right now normal // Cargo and cargo-marker might invalidate each others caches. - let sources = fetch::fetch_crates(config)?; - build::build_lints(&sources, config) + build_space::init(config)?; + + let packages = config.lints.keys().flat_map(|package| ["-p", package]); + + let mut cmd = config.toolchain.cargo.command(); + cmd.current_dir(&config.marker_dir); + cmd.arg("build"); + cmd.args(packages); + + // Potential "--release" flag + if !config.debug_build { + cmd.arg("--release"); + } + + // Environment + cmd.env("RUSTFLAGS", &config.build_rustc_flags); + + let exit_status = cmd + .log() + .spawn() + .expect("could not run cargo") + .wait() + .expect("failed to wait for cargo?"); + + if !exit_status.success() { + return Err(Error::root("Failed to compile the lint crates")); + } + + let profile = if config.debug_build { "debug" } else { "release" }; + + let dlls = config + .lints + .keys() + .map(|package| { + let dll_name = package.replace('-', "_"); + let file = config + .marker_dir + .join("target") + .join(profile) + .join(format!("{DLL_PREFIX}{dll_name}{DLL_SUFFIX}")); + + LintDll { + name: package.clone(), + file, + } + }) + .collect(); + + Ok(dlls) } diff --git a/cargo-marker/src/backend/lints/build.rs b/cargo-marker/src/backend/lints/build.rs deleted file mode 100644 index ce4d046d..00000000 --- a/cargo-marker/src/backend/lints/build.rs +++ /dev/null @@ -1,144 +0,0 @@ -use super::{LintCrate, LintCrateSource}; -use crate::backend::Config; -use crate::error::prelude::*; -use crate::observability::prelude::*; -use crate::utils::utf8::IntoUtf8; -use camino::Utf8Path; -use itertools::Itertools; -use std::{collections::HashSet, ffi::OsStr}; -use yansi::Paint; - -#[cfg(target_os = "linux")] -const DYNAMIC_LIB_FILE_ENDING: &str = "so"; -#[cfg(target_os = "macos")] -const DYNAMIC_LIB_FILE_ENDING: &str = "dylib"; -#[cfg(target_os = "windows")] -const DYNAMIC_LIB_FILE_ENDING: &str = "dll"; - -/// A list of file endings which are expected to be inside the lint crate dir. -/// It's assumed that these can be safely removed. -const ARTIFACT_ENDINGS: &[&str] = &[ - DYNAMIC_LIB_FILE_ENDING, - #[cfg(target_os = "windows")] - "exp", - #[cfg(target_os = "windows")] - "lib", - #[cfg(target_os = "windows")] - "pdb", -]; - -pub fn build_lints(sources: &[LintCrateSource], config: &Config) -> Result> { - // By default Cargo doesn't provide the path of the compiled lint crate. - // As a work around, we use the `--out-dir` option to make cargo copy all - // created binaries into one folder. We then scan that folder as we build our crates - // and collect all dynamic libraries that show up, we link the name of the crate we just built - // with the file(s) we found after we built it, assuming that they are related. - // - // TODO Look into how to optimize this a bit better: - // Another option that would be potentially more performant is if we built each - // crate in it's own target dir. this would eliminate the need for HashSet<_> below, without - // changing too much else about the implementation. - // - // This would be so much simpler if we could get an output name from Cargo - - // Clear previously build lints - let lints_dir = config.lint_crate_dir(); - clear_lints_dir(&lints_dir)?; - - // Build lint crates and find the output of those builds - let mut found_paths = HashSet::new(); - let ending = DYNAMIC_LIB_FILE_ENDING; - let mut lints = Vec::with_capacity(sources.len()); - - for lint_src in sources { - build_lint(lint_src, config)?; - match std::fs::read_dir(&lints_dir) { - Ok(dir) => { - for file in dir { - let file = file.unwrap().path().into_utf8()?; - if file.extension() == Some(ending) && !found_paths.contains(&file) { - found_paths.insert(file.clone()); - lints.push(LintCrate { - file, - name: lint_src.name.clone(), - }); - } - } - }, - Err(err) => { - // This shouldn't really be a point of failure. In this case, I'm - // more interested in the HOW? - panic!("unable to read lints dir after lint compilation: {lints_dir} ({err:#?})"); - }, - } - } - Ok(lints) -} - -/// This function clears the `marker/lints` directory holding all compiled lints. This -/// is required, as Marker uses the content of that directory to determine which lints -/// should be run. -/// -/// This is an extra function to not call `delete_dir_all` and just accidentally delete -/// the entire system. -fn clear_lints_dir(lints_dir: &Utf8Path) -> Result { - // Delete all files - let dir = match std::fs::read_dir(lints_dir) { - Ok(dir) => dir, - Err(err) if std::io::ErrorKind::NotFound == err.kind() => return Ok(()), - Err(err) => return Err(Error::wrap(err, "Failed to read lints artifacts directory")), - }; - - let endings: Vec<_> = ARTIFACT_ENDINGS.iter().map(OsStr::new).collect(); - - let (files, errors): (Vec<_>, Vec<_>) = dir.map(|result| result.map_err(Error::transparent)).partition_result(); - - if !errors.is_empty() { - return Err(Error::many(errors, "Failed to read the lints directory entries")); - } - - for file in files { - let file = file.path(); - - let is_expected_ending = file.extension().map(|ending| endings.contains(&ending)) == Some(true); - - if !is_expected_ending { - return Err(Error::root(format!( - "Marker's lint directory contains an unexpected file: {}", - file.display() - ))); - } - - std::fs::remove_file(&file) - .context(|| format!("Failed to remove the lint artifact file {}", file.display()))?; - } - - // The dir should now be empty - std::fs::remove_dir(lints_dir).context(|| format!("Failed to remove lints directory {lints_dir}")) -} - -fn build_lint(lint_src: &LintCrateSource, config: &Config) -> Result { - let mut cmd = config.toolchain.cargo_build_command(config, &lint_src.manifest); - - // Set output dir. This currently requires unstable options - cmd.arg("-Z"); - cmd.arg("unstable-options"); - cmd.arg("--out-dir"); - cmd.arg(config.lint_crate_dir().as_os_str()); - - let exit_status = cmd - .log() - .spawn() - .expect("could not run cargo") - .wait() - .expect("failed to wait for cargo?"); - - if exit_status.success() { - return Ok(()); - } - - Err(Error::root(format!( - "Failed to compile the lint crate {}", - lint_src.name.red().bold() - ))) -} diff --git a/cargo-marker/src/backend/lints/build_space.rs b/cargo-marker/src/backend/lints/build_space.rs new file mode 100644 index 00000000..15de69bb --- /dev/null +++ b/cargo-marker/src/backend/lints/build_space.rs @@ -0,0 +1,64 @@ +use crate::error::prelude::*; +use crate::{backend::Config, config::LintDependencyEntry}; +use camino::Utf8Path; +use itertools::Itertools; +use std::collections::BTreeMap; + +/// Initializes a cargo workspace for building the lint crates dynamic libraries. +/// Creates a manifest file where all lint crates are mentioned as dependencies. +pub(crate) fn init(config: &Config) -> Result<()> { + /// A small hack, to have the lints namespaced under the `[dependencies]` section + #[derive(serde::Serialize)] + struct DepNamespace<'a> { + dependencies: &'a BTreeMap, + } + + let lints_as_deps = toml::to_string(&DepNamespace { + dependencies: &config.lints, + }) + .expect("DepNamespace can be represented as TOML"); + + let toml_comments = comments("# "); + let manifest_content = format!("{toml_comments}\n{MANIFEST_HEADER}{lints_as_deps}"); + let manifest_path = config.marker_dir.join("Cargo.toml"); + + write_to_file(&manifest_path, &manifest_content)?; + write_to_file(&config.marker_dir.join("src").join("lib.rs"), &comments("//! ")) +} + +fn write_to_file(path: &Utf8Path, content: &str) -> Result { + let parent = path + .parent() + .unwrap_or_else(|| panic!("The file must have a parent directory. Path: {path}")); + + std::fs::create_dir_all(parent).context(|| format!("Failed to create the directory structure for {parent}"))?; + + std::fs::write(path, content).context(|| format!("Failed to write a file at {path}")) +} + +fn comments(comment_prefix: &str) -> String { + COMMENTS + .trim() + .lines() + .format_with("\n", |line, f| f(&format_args!("{comment_prefix}{line}"))) + .to_string() +} + +const COMMENTS: &str = r#" +Welcome 👋! This is an internal cargo workspace used by Marker to build lint crates 📦. +We put them as dependencies of this cargo package, and build them all at once 🔨. +As a result we get dynamic libraries 📚 for each of the lint crates and load them in +a custom rustc driver (marker_rustc_driver) at a later stage. +"#; + +const MANIFEST_HEADER: &str = r#" +[package] +name = "marker-lint-crates-build-space" +version = "0.1.0" +edition = "2021" +publish = false + +# This prevents Cargo from searching the parent directories for a workspace. +[workspace] + +"#; diff --git a/cargo-marker/src/backend/lints/fetch.rs b/cargo-marker/src/backend/lints/fetch.rs deleted file mode 100644 index 00ff0588..00000000 --- a/cargo-marker/src/backend/lints/fetch.rs +++ /dev/null @@ -1,143 +0,0 @@ -//! This module is responsible for fetching the lint crates and returning the -//! absolute path of the lints, for further processing. -//! -//! Cargo sadly doesn't provide an official public interface, to fetch crates -//! from git and registries. This will hopefully come at one point (rust-lang/cargo#1861) -//! but until then, we have to do some manual fetching. -//! -//! This module uses a small hack. Marker creates a new Cargo project, with the -//! specified lint crates as dependencies. Then `cargo fetch` is called, which -//! will download the crates into Cargo's cache. The absolute path to the lints -//! can then be retrieved from `cargo metadata`. - -use super::LintCrateSource; -use crate::error::prelude::*; -use crate::observability::prelude::*; -use crate::{backend::Config, config::LintDependencyEntry}; -use camino::{Utf8Path, Utf8PathBuf}; -use cargo_metadata::Metadata; -use std::collections::BTreeMap; - -/// This function fetches and locates all lint crates specified in the given -/// configuration. -pub fn fetch_crates(config: &Config) -> Result> { - // FIXME(xFrednet): Only create the dummy crate, if there is a non - // local dependency. - - let manifest = setup_dummy_crate(config)?; - - call_cargo_fetch(&manifest, config)?; - - let metadata = call_cargo_metadata(&manifest, config)?; - - Ok(extract_lint_crate_sources(&metadata, config)) -} - -/// This function sets up the dummy crate with all the lints listed as dependencies. -/// It returns the path of the manifest, if everything was successful. -fn setup_dummy_crate(config: &Config) -> Result { - /// A small hack, to have the lints namespaced under the `[dependencies]` section - #[derive(serde::Serialize)] - struct DepNamespace<'a> { - dependencies: &'a BTreeMap, - } - - // Manifest - let lints_as_deps = toml::to_string(&DepNamespace { - dependencies: &config.lints, - }) - .expect("DepNamespace can be represented as TOML"); - - let manifest_content = format!("{DUMMY_MANIFEST_TEMPLATE}{lints_as_deps}"); - let manifest_path = config.marker_dir.join("Cargo.toml"); - write_to_file(&manifest_path, &manifest_content)?; - - // `./src/main.rs` file - write_to_file(&config.marker_dir.join("src").join("main.rs"), DUMMY_MAIN_CONTENT)?; - - Ok(manifest_path) -} - -fn write_to_file(path: &Utf8Path, content: &str) -> Result { - let parent = path - .parent() - .unwrap_or_else(|| panic!("The file must have a parent directory. Path: {path}")); - - std::fs::create_dir_all(parent).context(|| format!("Failed to create the directory structure for {parent}"))?; - - std::fs::write(path, content).context(|| format!("Failed to write a file at {path}")) -} - -const DUMMY_MANIFEST_TEMPLATE: &str = r#" -# This is a dummy crate used by Marker, to get Cargo to fetch the lint crates -# as normal dependencies. The location of the fetched crates is then read using -# `cargo metadata`. - -[package] -name = "markers-dummy-crate-for-fetching" -version = "0.1.0" -edition = "2021" -publish = false - -# This prevents Cargo from searching the parent directories for a workspace. -[workspace] - -"#; - -const DUMMY_MAIN_CONTENT: &str = r#" - fn main() { - println!("Hey curious mind and welcome to Marker's internals."); - println!("This is just a dummy crate to fetch some dependencies."); - println!(); - println!("Achievement Unlocked: [Marker's hidden crate]"); - } -"#; - -fn call_cargo_fetch(manifest: &Utf8Path, config: &Config) -> Result { - let mut cmd = config.toolchain.cargo.command(); - cmd.arg("fetch"); - cmd.arg("--manifest-path"); - cmd.arg(manifest.as_os_str()); - - // Only fetch for the specified target. Cargo will just fetch everything, - // if the `--target` flag is not specified. - if let Ok(target) = std::env::var("TARGET") { - cmd.arg("--target"); - cmd.arg(target); - } - - let status = cmd - .log() - .spawn() - .expect("unable to start `cargo fetch` to fetch lint crates") - .wait() - .expect("unable to wait for `cargo fetch` to fetch lint crates"); - - if status.success() { - return Ok(()); - } - - Err(Error::root("cargo fetch failed for lint crates")) -} - -fn call_cargo_metadata(manifest: &Utf8Path, config: &Config) -> Result { - config - .toolchain - .cargo - .metadata() - .manifest_path(manifest) - .exec() - .context(|| format!("Failed to get cargo metadata for the lint crates at {manifest}")) -} - -fn extract_lint_crate_sources(metadata: &Metadata, marker_config: &Config) -> Vec { - metadata - .packages - .iter() - .filter(|pkg| marker_config.lints.contains_key(&pkg.name)) - .map(|pkg| LintCrateSource { - name: pkg.name.clone(), - manifest: pkg.manifest_path.clone(), - }) - .collect() -} diff --git a/cargo-marker/src/backend/toolchain.rs b/cargo-marker/src/backend/toolchain.rs index af5f3eb0..59acc39b 100644 --- a/cargo-marker/src/backend/toolchain.rs +++ b/cargo-marker/src/backend/toolchain.rs @@ -1,17 +1,15 @@ +use super::{ + cargo::Cargo, + driver::{default_driver_info, marker_driver_bin_name}, +}; use crate::error::prelude::*; use crate::observability::prelude::*; use crate::utils::{is_local_driver, utf8::IntoUtf8}; use crate::Result; -use camino::{Utf8Path, Utf8PathBuf}; +use camino::Utf8PathBuf; use std::process::Command; use yansi::Paint; -use super::{ - cargo::Cargo, - driver::{default_driver_info, marker_driver_bin_name}, - Config, -}; - #[derive(Debug)] pub struct Toolchain { pub(crate) driver_path: Utf8PathBuf, @@ -30,29 +28,6 @@ impl Toolchain { cmd } - pub fn cargo_build_command(&self, config: &Config, manifest: &Utf8Path) -> Command { - let mut cmd = self.cargo.command(); - cmd.arg("build"); - - // Manifest - cmd.arg("--manifest-path"); - cmd.arg(manifest.as_os_str()); - - // Target dir - cmd.arg("--target-dir"); - cmd.arg(config.markers_target_dir().as_os_str()); - - // Potential "--release" flag - if !config.debug_build { - cmd.arg("--release"); - } - - // Environment - cmd.env("RUSTFLAGS", &config.build_rustc_flags); - - cmd - } - pub fn find_target_dir(&self) -> Result { let metadata = self .cargo diff --git a/marker_uilints/tests/ui/lint_ice_message.rs b/marker_uilints/tests/ui/lint_ice_message.rs index e7e72b82..cbc75a7f 100644 --- a/marker_uilints/tests/ui/lint_ice_message.rs +++ b/marker_uilints/tests/ui/lint_ice_message.rs @@ -1,5 +1,5 @@ //@rustc-env:RUST_BACKTRACE=0 -//@normalize-stderr-test: "lib.rs:.*" -> "lib.rs" +//@normalize-stderr-test: "at .*/marker_uilints/src/lib.rs:.*" -> "at marker_uilints/src/lib.rs" // This function will trigger a panic in the `uilints` lint crate. // I want to see how the ICE message looks and make sure that it