From 873ba6e752b0bf2fbea42109f7528d4a359b2210 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Sat, 26 Nov 2022 00:40:12 +0100 Subject: [PATCH 1/6] Add `cargo marker install rustc-driver` --- cargo-marker/src/driver.rs | 89 ++++++++++++++++++++++++++++++++++++++ cargo-marker/src/main.rs | 43 +++++++++++++++--- util/update-toolchain.sh | 2 +- 3 files changed, 128 insertions(+), 6 deletions(-) create mode 100644 cargo-marker/src/driver.rs diff --git a/cargo-marker/src/driver.rs b/cargo-marker/src/driver.rs new file mode 100644 index 00000000..a5ed70d2 --- /dev/null +++ b/cargo-marker/src/driver.rs @@ -0,0 +1,89 @@ +//! This module hosts functions required to run rustc as a driver. +//! +//! The rustc driver depends on rustc, which interfaces is unstable. This means +//! that each driver version is bound to a specific version of rustc. The same +//! goes for Clippy. However, Clippy has the advantage, that it's distributes via +//! rustup, which handles the version matching for it. We're not so lucky, at +//! least not yet. Therefore, we're responsible that the driver is compiled and +//! run with the correct toolchain. +//! +//! If no driver is installed, the user will be requested to run the setup command. +//! That command will first ensure that the required toolchain is installed and then +//! run `cargo install` for the driver with a specific toolchain. The version and +//! toolchain are hardcoded in this crate. +//! +//! If a driver is already installed. We'll first run the driver to request the +//! required toolchain and then run the driver using that toolchain. Requesting +//! the toolchain works, since the argument will be processed before rustc is run. +//! At least, that's the idea. + +use std::process::Command; + +use once_cell::sync::Lazy; + +use crate::ExitStatus; + +/// This is the driver version and toolchain, that is used by the setup command +/// to install the driver. +static DEFAULT_DRIVER_INFO: Lazy = Lazy::new(|| RustcDriverInfo { + toolchain: "nightly-2022-11-03".to_string(), + version: "0.1.0".to_string(), +}); + +struct RustcDriverInfo { + toolchain: String, + version: String, +} + +/// This function checks if the specified toolchain is installed. This requires +/// rustup. A dependency we have to live with for now. +fn check_toolchain(toolchain: &str) -> Result<(), ExitStatus> { + let mut cmd = Command::new("cargo"); + cmd.args([&format!("+{toolchain}"), "-V"]); + if cmd.output().is_err() { + eprintln!("error: the required toolchain `{toolchain}` can't be found"); + eprintln!(); + eprintln!("You can install the toolchain by running: rustup toolchain install {toolchain}"); + Err(ExitStatus::InvalidToolchain) + } else { + Ok(()) + } +} + +/// This tries to install the rustc driver specified in [`DEFAULT_DRIVER_INFO`]. +pub fn install_driver(verbose: bool) -> Result<(), ExitStatus> { + // Prerequisites + let toolchain = &DEFAULT_DRIVER_INFO.toolchain; + check_toolchain(&toolchain)?; + + // Build driver + let mut cmd = Command::new("cargo"); + cmd.arg(&format!("+{toolchain}")); + + if verbose { + cmd.arg("--verbose"); + } + + #[cfg(feature = "dev-build")] + cmd.args(["build", "--bin", "linter_driver_rustc"]); + #[cfg(not(feature = "dev-build"))] + cmd.args([ + "install", + "marker_rustc_driver", + "--version", + &DEFAULT_DRIVER_INFO.version, + ]); + + let status = cmd + .spawn() + .expect("unable to start cargo install for the driver") + .wait() + .expect("unable to wait on cargo install for the driver"); + if status.success() { + Ok(()) + } else { + // The user can see cargo's output, as the command output was passed on + // to the user via the `.spawn()` call. + Err(ExitStatus::DriverInstallationFailed) + } +} diff --git a/cargo-marker/src/main.rs b/cargo-marker/src/main.rs index 93f7fa88..2bcb1313 100644 --- a/cargo-marker/src/main.rs +++ b/cargo-marker/src/main.rs @@ -9,13 +9,32 @@ use std::{ }; use clap::{self, Arg, ArgAction}; -use once_cell::sync::Lazy; +use once_cell::sync::{Lazy, OnceCell}; + +mod driver; const CARGO_ARGS_SEPARATOR: &str = "--"; const VERSION: &str = concat!("cargo-marker ", env!("CARGO_PKG_VERSION")); const LINT_KRATES_BASE_DIR: &str = "./target/marker"; static LINT_KRATES_TARGET_DIR: Lazy = Lazy::new(|| prepare_lint_build_dir("build", "target")); static LINT_KRATES_OUT_DIR: Lazy = Lazy::new(|| prepare_lint_build_dir("lints", "out")); +static VERBOSE: OnceCell = OnceCell::new(); + +#[derive(Debug)] +pub enum ExitStatus { + /// The toolchain validation failed. This could happen, if rustup is not + /// installed or the required toolchain is not installed. + InvalidToolchain = 100, + DriverInstallationFailed = 200, +} + +fn set_verbose(verbose: bool) { + VERBOSE.set(verbose).unwrap(); +} + +fn is_verbose() -> bool { + VERBOSE.get().copied().unwrap_or_default() +} /// This creates the absolute path for a given build directory. fn prepare_lint_build_dir(dir_name: &str, info_name: &str) -> String { @@ -38,7 +57,7 @@ fn prepare_lint_build_dir(dir_name: &str, info_name: &str) -> String { .to_string() } -fn main() { +fn main() -> Result<(), ExitStatus> { let matches = get_clap_config().get_matches_from( std::env::args() .enumerate() @@ -46,10 +65,15 @@ fn main() { .take_while(|s| s != CARGO_ARGS_SEPARATOR), ); + set_verbose(matches.get_flag("verbose")); + if matches.get_flag("version") { - let version_info = env!("CARGO_PKG_VERSION"); - println!("cargo-marker version: {version_info}"); - exit(0); + print_version(); + return Ok(()); + } + + if !validate_toolchain() { + return Err(ExitStatus::InvalidToolchain); } let verbose = matches.get_flag("verbose"); @@ -87,6 +111,15 @@ fn main() { } } +fn print_version() { + println!("cargo-marker version: {}", env!("CARGO_PKG_VERSION")); + + if is_verbose() { + println!("driver toolchain: {DRIVER_TOOLCHAIN}") + } +} + + fn run_driver(driver_path: &PathBuf, lint_crates: &str) { println!(); println!("Start linting:"); diff --git a/util/update-toolchain.sh b/util/update-toolchain.sh index 8bdf2c3f..f026a884 100755 --- a/util/update-toolchain.sh +++ b/util/update-toolchain.sh @@ -2,7 +2,7 @@ if [[ $1 == nightly-????-??-?? ]] then - sed -i "s/nightly-2022-11-03/$1/g" ./marker_driver_rustc/src/main.rs ./rust-toolchain .github/workflows/* ./util/update-toolchain.sh + sed -i "s/nightly-2022-11-03/$1/g" ./marker_driver_rustc/src/main.rs ./rust-toolchain .github/workflows/* ./util/update-toolchain.sh cargo-marker/src/main.rs else echo "Please enter a valid toolchain like \`nightly-2022-01-01\`" fi; From 58459d02a86891dde2d5daa9ef0cfb7242018b16 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Sat, 26 Nov 2022 10:38:38 +0100 Subject: [PATCH 2/6] Extract `cargo-marker`s `main.rs` into separate modules --- cargo-marker/Cargo.toml | 4 +- cargo-marker/src/cli.rs | 62 ++++++++++++++++ cargo-marker/src/driver.rs | 97 +++++++++++++++++++------ cargo-marker/src/lints.rs | 1 + cargo-marker/src/main.rs | 145 ++++++++----------------------------- 5 files changed, 172 insertions(+), 137 deletions(-) create mode 100644 cargo-marker/src/cli.rs create mode 100644 cargo-marker/src/lints.rs diff --git a/cargo-marker/Cargo.toml b/cargo-marker/Cargo.toml index 116037d8..e791d15c 100644 --- a/cargo-marker/Cargo.toml +++ b/cargo-marker/Cargo.toml @@ -10,6 +10,6 @@ once_cell = "1.16.0" [features] default = [] -# Indicates that development features like auto driver building etc should be enabled. -# This option assumes that it's being executed at the project root. +# This enables developer features used to automatically build the local version +# assuming, that it's being executed at the root of the repo. dev-build = [] diff --git a/cargo-marker/src/cli.rs b/cargo-marker/src/cli.rs new file mode 100644 index 00000000..536b9507 --- /dev/null +++ b/cargo-marker/src/cli.rs @@ -0,0 +1,62 @@ +use clap::{Arg, ArgAction, Command}; + +use crate::VERSION; + +const AFTER_HELP_MSG: &str = r#"CARGO ARGS + All arguments after double dashes(`--`) will be passed to cargo. + These options are the same as for `cargo check`. + +EXAMPLES: + * `cargo marker -l ./marker_lints` +"#; + +pub fn get_clap_config() -> Command { + Command::new(VERSION) + .arg( + Arg::new("version") + .short('V') + .long("version") + .action(ArgAction::SetTrue) + .help("Print version info and exit"), + ) + .arg( + Arg::new("verbose") + .short('v') + .long("verbose") + .action(ArgAction::SetTrue) + .help("Print additional debug information to the console"), + ) + .arg( + Arg::new("test-setup") + .long("test-setup") + .action(ArgAction::SetTrue) + .help("This flag will compile the lint crate and print all relevant environment values"), + ) + .subcommand(setup_command()) + .subcommand(check_command()) + .args(check_command_args()) + .after_help(AFTER_HELP_MSG) + .override_usage("cargo-marker [OPTIONS] -- ") +} + +fn setup_command() -> Command { + Command::new("setup") + .about("A collection of commands to setup marker") + .after_help("By default this will install the driver for rustc.") +} + +fn check_command() -> Command { + Command::new("check") + .about("Run marker on a local package") + .args(check_command_args()) +} + +fn check_command_args() -> impl IntoIterator> { + vec![ + Arg::new("lints") + .short('l') + .long("lints") + .num_args(1..) + .help("Defines a set of lints crates that should be used"), + ] +} diff --git a/cargo-marker/src/driver.rs b/cargo-marker/src/driver.rs index a5ed70d2..593a308f 100644 --- a/cargo-marker/src/driver.rs +++ b/cargo-marker/src/driver.rs @@ -11,13 +11,8 @@ //! That command will first ensure that the required toolchain is installed and then //! run `cargo install` for the driver with a specific toolchain. The version and //! toolchain are hardcoded in this crate. -//! -//! If a driver is already installed. We'll first run the driver to request the -//! required toolchain and then run the driver using that toolchain. Requesting -//! the toolchain works, since the argument will be processed before rustc is run. -//! At least, that's the idea. -use std::process::Command; +use std::{path::PathBuf, process::Command}; use once_cell::sync::Lazy; @@ -28,11 +23,35 @@ use crate::ExitStatus; static DEFAULT_DRIVER_INFO: Lazy = Lazy::new(|| RustcDriverInfo { toolchain: "nightly-2022-11-03".to_string(), version: "0.1.0".to_string(), + api_version: "0.1.0".to_string(), }); struct RustcDriverInfo { toolchain: String, version: String, + #[allow(unused)] + api_version: String, +} + +/// This tries to install the rustc driver specified in [`DEFAULT_DRIVER_INFO`]. +pub fn install_driver(verbose: bool) -> Result<(), ExitStatus> { + // The toolchain, driver version and api version should ideally be configurable. + // However, that will require more prototyping and has a low priority rn. + // See #60 + + // Prerequisites + let toolchain = &DEFAULT_DRIVER_INFO.toolchain; + check_toolchain(toolchain)?; + + build_driver( + toolchain, + &DEFAULT_DRIVER_INFO.version, + verbose, + cfg!(feature = "dev-build"), + )?; + + // We don't want to advice the user, to install the driver again. + check_driver(verbose, false) } /// This function checks if the specified toolchain is installed. This requires @@ -41,7 +60,7 @@ fn check_toolchain(toolchain: &str) -> Result<(), ExitStatus> { let mut cmd = Command::new("cargo"); cmd.args([&format!("+{toolchain}"), "-V"]); if cmd.output().is_err() { - eprintln!("error: the required toolchain `{toolchain}` can't be found"); + eprintln!("Error: The required toolchain `{toolchain}` can't be found"); eprintln!(); eprintln!("You can install the toolchain by running: rustup toolchain install {toolchain}"); Err(ExitStatus::InvalidToolchain) @@ -50,29 +69,32 @@ fn check_toolchain(toolchain: &str) -> Result<(), ExitStatus> { } } -/// This tries to install the rustc driver specified in [`DEFAULT_DRIVER_INFO`]. -pub fn install_driver(verbose: bool) -> Result<(), ExitStatus> { - // Prerequisites - let toolchain = &DEFAULT_DRIVER_INFO.toolchain; - check_toolchain(&toolchain)?; +/// This tries to compile the driver. If successful the driver binary will +/// be places next to the executable of `cargo-linter`. +fn build_driver(toolchain: &str, version: &str, verbose: bool, dev_build: bool) -> Result<(), ExitStatus> { + if dev_build { + println!("Compiling rustc driver"); + } else { + println!("Compiling rustc driver v{version} with {toolchain}"); + } // Build driver let mut cmd = Command::new("cargo"); - cmd.arg(&format!("+{toolchain}")); + + if !dev_build { + cmd.arg(&format!("+{toolchain}")); + } if verbose { cmd.arg("--verbose"); } - #[cfg(feature = "dev-build")] - cmd.args(["build", "--bin", "linter_driver_rustc"]); - #[cfg(not(feature = "dev-build"))] - cmd.args([ - "install", - "marker_rustc_driver", - "--version", - &DEFAULT_DRIVER_INFO.version, - ]); + if dev_build { + cmd.args(["build", "--bin", "marker_driver_rustc"]); + } else { + // TODO Set output path to the local branch thingy + cmd.args(["install", "marker_rustc_driver", "--version", version]); + } let status = cmd .spawn() @@ -87,3 +109,34 @@ pub fn install_driver(verbose: bool) -> Result<(), ExitStatus> { Err(ExitStatus::DriverInstallationFailed) } } + +fn check_driver(verbose: bool, print_advice: bool) -> Result<(), ExitStatus> { + let path = get_driver_path(); + if verbose { + println!("Searching for driver at: {}", path.display()); + } + + if !path.exists() || !path.is_file() { + if print_advice { + eprintln!("Error: The driver binary could not be found."); + eprintln!(); + eprintln!("Try installing it via `cargo marker setup`"); + } + + Err(ExitStatus::MissingDriver) + } else { + Ok(()) + } +} + +pub fn get_driver_path() -> PathBuf { + #[allow(unused_mut)] + let mut path = std::env::current_exe() + .expect("unable to retrieve the path of the current executable") + .with_file_name("marker_driver_rustc"); + + #[cfg(target_os = "windows")] + path.set_extension("exe"); + + path +} diff --git a/cargo-marker/src/lints.rs b/cargo-marker/src/lints.rs new file mode 100644 index 00000000..8b137891 --- /dev/null +++ b/cargo-marker/src/lints.rs @@ -0,0 +1 @@ + diff --git a/cargo-marker/src/main.rs b/cargo-marker/src/main.rs index 2bcb1313..c64dbd59 100644 --- a/cargo-marker/src/main.rs +++ b/cargo-marker/src/main.rs @@ -1,5 +1,10 @@ #![warn(clippy::pedantic)] #![warn(clippy::index_refutable_slice)] +#![allow(clippy::module_name_repetitions)] + +mod cli; +mod driver; +mod lints; use std::{ ffi::OsStr, @@ -8,32 +13,30 @@ use std::{ process::{exit, Command}, }; -use clap::{self, Arg, ArgAction}; -use once_cell::sync::{Lazy, OnceCell}; - -mod driver; +use cli::get_clap_config; +use driver::get_driver_path; +use once_cell::sync::Lazy; const CARGO_ARGS_SEPARATOR: &str = "--"; const VERSION: &str = concat!("cargo-marker ", env!("CARGO_PKG_VERSION")); const LINT_KRATES_BASE_DIR: &str = "./target/marker"; static LINT_KRATES_TARGET_DIR: Lazy = Lazy::new(|| prepare_lint_build_dir("build", "target")); static LINT_KRATES_OUT_DIR: Lazy = Lazy::new(|| prepare_lint_build_dir("lints", "out")); -static VERBOSE: OnceCell = OnceCell::new(); #[derive(Debug)] pub enum ExitStatus { /// The toolchain validation failed. This could happen, if rustup is not /// installed or the required toolchain is not installed. InvalidToolchain = 100, - DriverInstallationFailed = 200, -} - -fn set_verbose(verbose: bool) { - VERBOSE.set(verbose).unwrap(); -} - -fn is_verbose() -> bool { - VERBOSE.get().copied().unwrap_or_default() + /// Unable to find the driver binary + MissingDriver = 200, + /// Nothing we can really do, but good to know. The user will have to analyze + /// the forwarded cargo output. + DriverInstallationFailed = 300, + /// A general collection status, for failures originating from the driver + DriverFailed = 400, + /// Check failed + MarkerCheckFailed = 1000, } /// This creates the absolute path for a given build directory. @@ -65,20 +68,22 @@ fn main() -> Result<(), ExitStatus> { .take_while(|s| s != CARGO_ARGS_SEPARATOR), ); - set_verbose(matches.get_flag("verbose")); + let verbose = matches.get_flag("verbose"); if matches.get_flag("version") { print_version(); return Ok(()); } - if !validate_toolchain() { - return Err(ExitStatus::InvalidToolchain); + match matches.subcommand() { + Some(("setup", _args)) => driver::install_driver(verbose), + Some(("check", args)) => run_check(args, verbose), + None => run_check(&matches, verbose), + _ => unreachable!(), } +} - let verbose = matches.get_flag("verbose"); - validate_driver(verbose); - +fn run_check(matches: &clap::ArgMatches, verbose: bool) -> Result<(), ExitStatus> { let mut lint_crates = vec![]; if let Some(cmd_lint_crates) = matches.get_many::("lints") { println!(); @@ -106,21 +111,17 @@ fn main() -> Result<(), ExitStatus> { if matches.get_flag("test-setup") { println!("env:RUSTC_WORKSPACE_WRAPPER={}", driver_path.display()); println!("env:MARKER_LINT_CRATES={marker_crates_env}"); + Ok(()) } else { - run_driver(&driver_path, &marker_crates_env); + run_driver(&driver_path, &marker_crates_env) } } fn print_version() { println!("cargo-marker version: {}", env!("CARGO_PKG_VERSION")); - - if is_verbose() { - println!("driver toolchain: {DRIVER_TOOLCHAIN}") - } } - -fn run_driver(driver_path: &PathBuf, lint_crates: &str) { +fn run_driver(driver_path: &PathBuf, lint_crates: &str) -> Result<(), ExitStatus> { println!(); println!("Start linting:"); @@ -137,8 +138,10 @@ fn run_driver(driver_path: &PathBuf, lint_crates: &str) { .wait() .expect("failed to wait for cargo?"); - if !exit_status.success() { - exit(exit_status.code().unwrap_or(-1)); + if exit_status.success() { + Ok(()) + } else { + Err(ExitStatus::MarkerCheckFailed) } } @@ -197,49 +200,6 @@ fn prepare_lint_crate(krate: &str, verbose: bool) -> Result { Ok(krate_path.display().to_string()) } -/// On release builds this will exit with a message and `-1` if the driver is missing. -#[allow(unused_variables)] // `verbose` is only used if `feature = dev-build` -fn validate_driver(verbose: bool) { - #[cfg(feature = "dev-build")] - { - println!(); - println!("Compiling Driver:"); - - let mut cmd = cargo_command(verbose); - - let exit_status = cmd - .args(["build", "-p", "marker_driver_rustc"]) - .env("RUSTFLAGS", "--cap-lints=allow") - .spawn() - .expect("could not run cargo") - .wait() - .expect("failed to wait for cargo?"); - - if !exit_status.success() { - exit(exit_status.code().unwrap_or(-1)) - } - } - - let path = get_driver_path(); - if !path.exists() || !path.is_file() { - eprintln!("Unable to find driver, searched at: {}", path.display()); - - exit(-1) - } -} - -fn get_driver_path() -> PathBuf { - #[allow(unused_mut)] - let mut path = std::env::current_exe() - .expect("current executable path invalid") - .with_file_name("marker_driver_rustc"); - - #[cfg(target_os = "windows")] - path.set_extension("exe"); - - path -} - fn cargo_command(verbose: bool) -> Command { // Here we want to use the normal cargo command, to go through the rustup // cargo executable and with that, set the required toolchain version. @@ -253,44 +213,3 @@ fn cargo_command(verbose: bool) -> Command { } cmd } - -fn get_clap_config() -> clap::Command { - clap::Command::new(VERSION) - .arg( - Arg::new("version") - .short('V') - .long("version") - .action(ArgAction::SetTrue) - .help("Print version info and exit"), - ) - .arg( - Arg::new("verbose") - .short('v') - .long("verbose") - .action(ArgAction::SetTrue) - .help("Print additional debug information to the console"), - ) - .arg( - Arg::new("lints") - .short('l') - .long("lints") - .num_args(1..) - .help("Defines a set of lints crates that should be used"), - ) - .arg( - Arg::new("test-setup") - .long("test-setup") - .action(ArgAction::SetTrue) - .help("This flag will compile the lint crate and print all relevant environment values"), - ) - .after_help(AFTER_HELP_MSG) - .override_usage("cargo-marker [OPTIONS] -- ") -} - -const AFTER_HELP_MSG: &str = r#"CARGO ARGS - All arguments after double dashes(`--`) will be passed to cargo. - These options are the same as for `cargo check`. - -EXAMPLES: - * `cargo marker -l ./marker_lints` -"#; From 60b49bcba5b2ed48002f482ed06ab3f2ef73fd31 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Tue, 29 Nov 2022 10:57:03 +0100 Subject: [PATCH 3/6] Change last mentionings from Rust-linting to Rust-marker --- LICENSE-MIT | 2 +- README.md | 4 ++-- marker_api/src/ast/item.rs | 2 +- marker_lints/tests/compile_test.rs | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/LICENSE-MIT b/LICENSE-MIT index 0aaf47ad..b9d5e7dc 100644 --- a/LICENSE-MIT +++ b/LICENSE-MIT @@ -1,6 +1,6 @@ MIT License -Copyright (c) 2022-2022 Rust-linting +Copyright (c) 2022-2022 Rust-marker Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated diff --git a/README.md b/README.md index 348c0f5c..c8ad32e2 100644 --- a/README.md +++ b/README.md @@ -12,9 +12,9 @@ The project is currently only available in this GitHub repo. For a quick test, c ## License -Copyright (c) 2022-2022 Rust-linting +Copyright (c) 2022-2022 Rust-marker -Rust-linting is distributed under the terms of both the MIT license +Rust-marker is distributed under the terms of both the MIT license and the Apache License (Version 2.0). See [LICENSE-APACHE](./LICENSE-APACHE), [LICENSE-MIT](./LICENSE-MIT). diff --git a/marker_api/src/ast/item.rs b/marker_api/src/ast/item.rs index 598f3173..5f47d77d 100644 --- a/marker_api/src/ast/item.rs +++ b/marker_api/src/ast/item.rs @@ -239,7 +239,7 @@ impl<'ast> CommonItemData<'ast> { } } -/// FIXME: Add function as discussed in +/// FIXME: Add function as discussed in /// this will require new driver callback functions #[repr(C)] #[derive(Debug, PartialEq, Eq, Hash)] diff --git a/marker_lints/tests/compile_test.rs b/marker_lints/tests/compile_test.rs index b7c56280..e6b39339 100644 --- a/marker_lints/tests/compile_test.rs +++ b/marker_lints/tests/compile_test.rs @@ -57,7 +57,7 @@ struct TestSetup { fn run_test_setup() -> TestSetup { const CARGO_MARKER_INVOCATION: &[&str] = &["run", "--bin", "cargo-marker", "--features", "dev-build", "--"]; - // ../rust-linting/marker_lints + // ../rust-marker/marker_lints let current_dir = env::current_dir().unwrap(); let lint_crate_src = fs::canonicalize(¤t_dir).unwrap(); let mut cmd = Command::new("cargo"); From 7254a21936f4491993b83ce8932d126fddb30245 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Wed, 30 Nov 2022 23:38:06 +0100 Subject: [PATCH 4/6] Use OsString and paths in cargo-marker --- cargo-marker/Cargo.toml | 2 +- cargo-marker/src/cli.rs | 3 +- cargo-marker/src/driver.rs | 37 ++++++++- cargo-marker/src/lints.rs | 62 ++++++++++++++ cargo-marker/src/main.rs | 166 +++++++++++++++---------------------- 5 files changed, 168 insertions(+), 102 deletions(-) diff --git a/cargo-marker/Cargo.toml b/cargo-marker/Cargo.toml index e791d15c..89d7d9c6 100644 --- a/cargo-marker/Cargo.toml +++ b/cargo-marker/Cargo.toml @@ -5,7 +5,7 @@ edition = "2021" license = "MIT OR Apache-2.0" [dependencies] -clap = "4.0.26" +clap = {version = "4.0.26", features = ["string"]} once_cell = "1.16.0" [features] diff --git a/cargo-marker/src/cli.rs b/cargo-marker/src/cli.rs index 536b9507..a1a75c38 100644 --- a/cargo-marker/src/cli.rs +++ b/cargo-marker/src/cli.rs @@ -1,4 +1,4 @@ -use clap::{Arg, ArgAction, Command}; +use clap::{builder::ValueParser, Arg, ArgAction, Command}; use crate::VERSION; @@ -57,6 +57,7 @@ fn check_command_args() -> impl IntoIterator> { .short('l') .long("lints") .num_args(1..) + .value_parser(ValueParser::os_string()) .help("Defines a set of lints crates that should be used"), ] } diff --git a/cargo-marker/src/driver.rs b/cargo-marker/src/driver.rs index 593a308f..57db698b 100644 --- a/cargo-marker/src/driver.rs +++ b/cargo-marker/src/driver.rs @@ -12,7 +12,7 @@ //! run `cargo install` for the driver with a specific toolchain. The version and //! toolchain are hardcoded in this crate. -use std::{path::PathBuf, process::Command}; +use std::{ffi::OsString, path::PathBuf, process::Command}; use once_cell::sync::Lazy; @@ -33,6 +33,13 @@ struct RustcDriverInfo { api_version: String, } +pub fn print_driver_version() { + println!( + "rustc driver version: {} (toolchain: {}, api: {})", + DEFAULT_DRIVER_INFO.version, DEFAULT_DRIVER_INFO.toolchain, DEFAULT_DRIVER_INFO.api_version + ); +} + /// This tries to install the rustc driver specified in [`DEFAULT_DRIVER_INFO`]. pub fn install_driver(verbose: bool) -> Result<(), ExitStatus> { // The toolchain, driver version and api version should ideally be configurable. @@ -129,6 +136,34 @@ fn check_driver(verbose: bool, print_advice: bool) -> Result<(), ExitStatus> { } } +pub fn run_driver( + env: Vec<(OsString, OsString)>, + cargo_args: impl Iterator, + verbose: bool, +) -> Result<(), ExitStatus> { + check_driver(verbose, true)?; + println!(); + println!("Start linting:"); + + let mut cmd = Command::new("cargo"); + cmd.envs(env).arg("check").args(cargo_args); + if verbose { + cmd.arg("--verbose"); + } + + let exit_status = cmd + .spawn() + .expect("could not run cargo") + .wait() + .expect("failed to wait for cargo?"); + + if exit_status.success() { + Ok(()) + } else { + Err(ExitStatus::MarkerCheckFailed) + } +} + pub fn get_driver_path() -> PathBuf { #[allow(unused_mut)] let mut path = std::env::current_exe() diff --git a/cargo-marker/src/lints.rs b/cargo-marker/src/lints.rs index 8b137891..da7b2f54 100644 --- a/cargo-marker/src/lints.rs +++ b/cargo-marker/src/lints.rs @@ -1 +1,63 @@ +use std::{ + ffi::OsStr, + path::{Path, PathBuf}, + process::Command, +}; +use crate::ExitStatus; + +/// This creates a debug build for a local crate. The path of the build library +/// will be returned, if the operation was successful. +pub fn build_local_lint_crate(crate_dir: &Path, target_dir: &Path, verbose: bool) -> Result { + if !crate_dir.exists() { + eprintln!("The given lint can't be found, searched at: `{}`", crate_dir.display()); + return Err(ExitStatus::LintCrateNotFound); + } + + // Compile the lint crate + let mut cmd = Command::new("cargo"); + if verbose { + cmd.arg("--verbose"); + } + let exit_status = cmd + .current_dir(std::fs::canonicalize(crate_dir).unwrap()) + .args(["build", "--lib", "--target-dir"]) + .arg(target_dir.as_os_str()) + .env("RUSTFLAGS", "--cap-lints=allow") + .spawn() + .expect("could not run cargo") + .wait() + .expect("failed to wait for cargo?"); + + if !exit_status.success() { + return Err(ExitStatus::LintCrateBuildFail); + } + + // Find the final binary and return the string + #[cfg(any(target_os = "linux", target_os = "macos"))] + let lib_file_prefix = "lib"; + #[cfg(target_os = "windows")] + let lib_file_prefix = ""; + + // FIXME: currently this expect, that the lib name is the same as the crate dir. + // See marker#60 + let file_name = format!( + "{lib_file_prefix}{}", + crate_dir.file_name().and_then(OsStr::to_str).unwrap_or_default() + ); + // Here `debug` is attached as the crate is build with the `cargo build` command + let mut krate_path = target_dir.join("debug").join(file_name); + + #[cfg(target_os = "linux")] + krate_path.set_extension("so"); + #[cfg(target_os = "macos")] + krate_path.set_extension("dylib"); + #[cfg(target_os = "windows")] + krate_path.set_extension("dll"); + + if !krate_path.exists() && !krate_path.is_file() { + Err(ExitStatus::LintCrateLibNotFound) + } else { + Ok(krate_path) + } +} diff --git a/cargo-marker/src/main.rs b/cargo-marker/src/main.rs index c64dbd59..96289837 100644 --- a/cargo-marker/src/main.rs +++ b/cargo-marker/src/main.rs @@ -7,21 +7,25 @@ mod driver; mod lints; use std::{ - ffi::OsStr, + ffi::{OsStr, OsString}, fs::create_dir_all, + io::{self, Write}, + os::unix::prelude::OsStrExt, path::{Path, PathBuf}, - process::{exit, Command}, + process::exit, }; use cli::get_clap_config; -use driver::get_driver_path; +use driver::{get_driver_path, run_driver}; +use lints::build_local_lint_crate; use once_cell::sync::Lazy; +use crate::driver::print_driver_version; + const CARGO_ARGS_SEPARATOR: &str = "--"; const VERSION: &str = concat!("cargo-marker ", env!("CARGO_PKG_VERSION")); const LINT_KRATES_BASE_DIR: &str = "./target/marker"; -static LINT_KRATES_TARGET_DIR: Lazy = Lazy::new(|| prepare_lint_build_dir("build", "target")); -static LINT_KRATES_OUT_DIR: Lazy = Lazy::new(|| prepare_lint_build_dir("lints", "out")); +static MARKER_LINT_DIR: Lazy = Lazy::new(|| prepare_lint_build_dir("marker", "marker")); #[derive(Debug)] pub enum ExitStatus { @@ -35,6 +39,12 @@ pub enum ExitStatus { DriverInstallationFailed = 300, /// A general collection status, for failures originating from the driver DriverFailed = 400, + /// The lint crate build failed for some reason + LintCrateBuildFail = 500, + /// Lint crate could not be found + LintCrateNotFound = 501, + /// The lint crate has been build, but the resulting binary could not be found. + LintCrateLibNotFound = 502, /// Check failed MarkerCheckFailed = 1000, } @@ -71,7 +81,7 @@ fn main() -> Result<(), ExitStatus> { let verbose = matches.get_flag("verbose"); if matches.get_flag("version") { - print_version(); + print_version(verbose); return Ok(()); } @@ -85,14 +95,15 @@ fn main() -> Result<(), ExitStatus> { fn run_check(matches: &clap::ArgMatches, verbose: bool) -> Result<(), ExitStatus> { let mut lint_crates = vec![]; - if let Some(cmd_lint_crates) = matches.get_many::("lints") { + if let Some(cmd_lint_crates) = matches.get_many::("lints") { println!(); println!("Compiling Lints:"); lint_crates.reserve(cmd_lint_crates.len()); + let target_dir = Path::new(&*MARKER_LINT_DIR); for krate in cmd_lint_crates { - if let Ok(krate_dir) = prepare_lint_crate(krate, verbose) { - lint_crates.push(krate_dir); - } + let src_dir = PathBuf::from(krate); + let crate_file = build_local_lint_crate(src_dir.as_path(), target_dir, verbose)?; + lint_crates.push(crate_file.as_os_str().to_os_string()); } } @@ -101,115 +112,72 @@ fn run_check(matches: &clap::ArgMatches, verbose: bool) -> Result<(), ExitStatus exit(-1); } - if lint_crates.iter().any(|path| path.contains(';')) { + if lint_crates.iter().any(|path| path.to_string_lossy().contains(';')) { eprintln!("The absolute paths of lint crates are not allowed to contain a `;`"); exit(-1); } - let driver_path = get_driver_path(); - let marker_crates_env = lint_crates.join(";"); + #[rustfmt::skip] + let env = vec![ + (OsString::from("RUSTC_WORKSPACE_WRAPPER"), get_driver_path().as_os_str().to_os_string()), + (OsString::from("MARKER_LINT_CRATES"), lint_crates.join(OsStr::new(";"))) + ]; if matches.get_flag("test-setup") { - println!("env:RUSTC_WORKSPACE_WRAPPER={}", driver_path.display()); - println!("env:MARKER_LINT_CRATES={marker_crates_env}"); + print_env(env).unwrap(); Ok(()) } else { - run_driver(&driver_path, &marker_crates_env) + let cargo_args = std::env::args().skip_while(|c| c != CARGO_ARGS_SEPARATOR).skip(1); + run_driver(env, cargo_args, verbose) } } -fn print_version() { +fn print_version(verbose: bool) { println!("cargo-marker version: {}", env!("CARGO_PKG_VERSION")); -} - -fn run_driver(driver_path: &PathBuf, lint_crates: &str) -> Result<(), ExitStatus> { - println!(); - println!("Start linting:"); - let mut cmd = Command::new("cargo"); - let cargo_args = std::env::args().skip_while(|c| c != CARGO_ARGS_SEPARATOR).skip(1); - cmd.env("RUSTC_WORKSPACE_WRAPPER", driver_path) - .env("MARKER_LINT_CRATES", lint_crates) - .arg("check") - .args(cargo_args); - - let exit_status = cmd - .spawn() - .expect("could not run cargo") - .wait() - .expect("failed to wait for cargo?"); - - if exit_status.success() { - Ok(()) - } else { - Err(ExitStatus::MarkerCheckFailed) + if verbose { + print_driver_version(); } } -/// This function ensures that the given crate is compiled as a library and -/// returns the compiled library path if everything was successful. Otherwise -/// it'll issue a warning and return `Err` -fn prepare_lint_crate(krate: &str, verbose: bool) -> Result { - let path = Path::new(krate); - if !path.exists() { - eprintln!("The given lint can't be found, searched at: `{}`", path.display()); - return Err(()); - } - - let mut cmd = cargo_command(verbose); - let exit_status = cmd - .current_dir(std::fs::canonicalize(path).unwrap()) - .args([ - "build", - "--lib", - "--target-dir", - &*LINT_KRATES_TARGET_DIR, - "-Z", - "unstable-options", - "--out-dir", - &*LINT_KRATES_OUT_DIR, - ]) - .env("RUSTFLAGS", "--cap-lints=allow") - .spawn() - .expect("could not run cargo") - .wait() - .expect("failed to wait for cargo?"); - - if !exit_status.success() { - return Err(()); - } +fn print_env(env: Vec<(OsString, OsString)>) -> io::Result<()> { + // Operating systems are fun... So, this function prints out the environment + // values to the standard output. For Unix systems, this requires `OsStr` + // objects, as file names are just bytes and don't need to be valid UTF-8. + // Windows, on the other hand, restricts file names, but uses UTF-16. The + // restriction only makes it slightly better, since windows `OsString` version + // doesn't have a `bytes()` method. Rust additionally has a restriction on the + // stdout of windows, that it has to be valid UTF-8, which means more conversion. + // + // This would be so much easier if everyone followed the "UTF-8 Everywhere Manifesto" #[cfg(any(target_os = "linux", target_os = "macos"))] - let lib_file_prefix = "lib"; - #[cfg(target_os = "windows")] - let lib_file_prefix = ""; + { + use std::os::unix::prelude::OsStrExt; + + // stdout is used directly, to print the `OsString`s without requiring + // them to be valid UTF-8 + let mut lock = io::stdout().lock(); + for (name, value) in env { + write!(lock, "env:")?; + lock.write_all(name.as_bytes())?; + write!(lock, "=")?; + lock.write_all(value.as_bytes())?; + writeln!(lock)?; + } - // FIXME: currently this expect, that the lib name is the same as the crate dir. - let file_name = format!( - "{lib_file_prefix}{}", - path.file_name().and_then(OsStr::to_str).unwrap_or_default() - ); - let mut krate_path = Path::new(&*LINT_KRATES_OUT_DIR).join(file_name); + return Ok(()); + } - #[cfg(target_os = "linux")] - krate_path.set_extension("so"); - #[cfg(target_os = "macos")] - krate_path.set_extension("dylib"); #[cfg(target_os = "windows")] - krate_path.set_extension("dll"); - - Ok(krate_path.display().to_string()) -} - -fn cargo_command(verbose: bool) -> Command { - // Here we want to use the normal cargo command, to go through the rustup - // cargo executable and with that, set the required toolchain version. - // This will add a slight overhead to each cargo call. This feels a bit - // unavoidable, until marker is delivered as part of the toolchain. Let's - // hope that day will happen! - let mut cmd = Command::new("cargo"); + { + for (name, value) in env { + if let (Some(name), Some(value)) = (name.to_str(), value.to_str()) { + println!("env:{name}={value}"); + } else { + unreachable!("Windows requires it's file path to be valid UTF-16 AFAIK"); + } + } - if verbose { - cmd.arg("--verbose"); + return Ok(()); } - cmd } From 60e1fa4dbe71732de9dc7cff4cb7c2e523135f78 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Thu, 1 Dec 2022 12:09:55 +0100 Subject: [PATCH 5/6] Adjust `cargo-marker` package, because of reasons --- .github/workflows/rust.yml | 2 +- .github/workflows/rust_bors.yml | 2 +- Cargo.lock | 2 +- cargo-marker/Cargo.toml | 9 ++++++++- cargo-marker/src/main.rs | 1 - 5 files changed, 11 insertions(+), 5 deletions(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 1836d3ca..1a26a0af 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -66,6 +66,6 @@ jobs: - run: rustc -vV # Test - - run: cargo build --package cargo-marker + - run: cargo build --package cargo_marker - run: cargo build --package marker_api - run: cargo build --package marker_lints diff --git a/.github/workflows/rust_bors.yml b/.github/workflows/rust_bors.yml index 25e38a6d..3f6464d1 100644 --- a/.github/workflows/rust_bors.yml +++ b/.github/workflows/rust_bors.yml @@ -61,6 +61,6 @@ jobs: - run: rustc -vV # Test - - run: cargo build --package cargo-marker + - run: cargo build --package cargo_marker - run: cargo build --package marker_api - run: cargo build --package marker_lints diff --git a/Cargo.lock b/Cargo.lock index d5ed8ada..68b554f7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -30,7 +30,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "572f695136211188308f16ad2ca5c851a712c464060ae6974944458eb83880ba" [[package]] -name = "cargo-marker" +name = "cargo_marker" version = "0.1.0" dependencies = [ "clap", diff --git a/cargo-marker/Cargo.toml b/cargo-marker/Cargo.toml index 89d7d9c6..46dffd6c 100644 --- a/cargo-marker/Cargo.toml +++ b/cargo-marker/Cargo.toml @@ -1,9 +1,16 @@ [package] -name = "cargo-marker" +name = "cargo_marker" version = "0.1.0" edition = "2021" license = "MIT OR Apache-2.0" +# Crate names in Rust are fun. I reserved `cargo_marker` as a crate name. However, +# Cargo requires it's subcommands to use a dash like `cargo-marker`. Unable to rename +# rename the create on crates.io we now have this hack... At least it works +[[bin]] +name = "cargo-marker" +path = "src/main.rs" + [dependencies] clap = {version = "4.0.26", features = ["string"]} once_cell = "1.16.0" diff --git a/cargo-marker/src/main.rs b/cargo-marker/src/main.rs index 96289837..9de2a347 100644 --- a/cargo-marker/src/main.rs +++ b/cargo-marker/src/main.rs @@ -10,7 +10,6 @@ use std::{ ffi::{OsStr, OsString}, fs::create_dir_all, io::{self, Write}, - os::unix::prelude::OsStrExt, path::{Path, PathBuf}, process::exit, }; From c26b8689981293c764db1cc07183d2be0477cdfe Mon Sep 17 00:00:00 2001 From: xFrednet Date: Fri, 2 Dec 2022 00:14:13 +0100 Subject: [PATCH 6/6] Last adjustments for the PR --- cargo-marker/src/driver.rs | 11 ++++++++++- cargo-marker/src/main.rs | 8 +++++--- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/cargo-marker/src/driver.rs b/cargo-marker/src/driver.rs index 57db698b..15f63ceb 100644 --- a/cargo-marker/src/driver.rs +++ b/cargo-marker/src/driver.rs @@ -99,7 +99,16 @@ fn build_driver(toolchain: &str, version: &str, verbose: bool, dev_build: bool) if dev_build { cmd.args(["build", "--bin", "marker_driver_rustc"]); } else { - // TODO Set output path to the local branch thingy + // FIXME: This currently installs the binary in Cargo's default location. + // Ideally this should install the driver in the toolchain folder for the + // installed nightly version. This would allow the user to have multiple + // drivers depending on the project toolchain. + // + // We can just reuse rustup to select the correct driver for a defined + // toolchain. This would also simulate how the driver would be delivered + // in a perfect world. + // + // See #60 cmd.args(["install", "marker_rustc_driver", "--version", version]); } diff --git a/cargo-marker/src/main.rs b/cargo-marker/src/main.rs index 9de2a347..51db32f5 100644 --- a/cargo-marker/src/main.rs +++ b/cargo-marker/src/main.rs @@ -9,7 +9,7 @@ mod lints; use std::{ ffi::{OsStr, OsString}, fs::create_dir_all, - io::{self, Write}, + io, path::{Path, PathBuf}, process::exit, }; @@ -138,6 +138,7 @@ fn print_version(verbose: bool) { } } +#[allow(clippy::unnecessary_wraps)] fn print_env(env: Vec<(OsString, OsString)>) -> io::Result<()> { // Operating systems are fun... So, this function prints out the environment // values to the standard output. For Unix systems, this requires `OsStr` @@ -151,6 +152,7 @@ fn print_env(env: Vec<(OsString, OsString)>) -> io::Result<()> { #[cfg(any(target_os = "linux", target_os = "macos"))] { + use std::io::Write; use std::os::unix::prelude::OsStrExt; // stdout is used directly, to print the `OsString`s without requiring @@ -164,7 +166,7 @@ fn print_env(env: Vec<(OsString, OsString)>) -> io::Result<()> { writeln!(lock)?; } - return Ok(()); + Ok(()) } #[cfg(target_os = "windows")] @@ -177,6 +179,6 @@ fn print_env(env: Vec<(OsString, OsString)>) -> io::Result<()> { } } - return Ok(()); + Ok(()) } }