Skip to content

cargo-marker refactoring and review #66

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

Merged
merged 6 commits into from
Jan 5, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
2 changes: 1 addition & 1 deletion .github/workflows/rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion .github/workflows/rust_bors.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion LICENSE-MIT
Original file line number Diff line number Diff line change
@@ -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
Expand Down
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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).
15 changes: 11 additions & 4 deletions cargo-marker/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,15 +1,22 @@
[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 = "4.0.26"
clap = {version = "4.0.26", features = ["string"]}
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 = []
63 changes: 63 additions & 0 deletions cargo-marker/src/cli.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
use clap::{builder::ValueParser, 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] -- <CARGO ARGS>")
}

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<Item = impl Into<Arg>> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, not to pile up on an already huge pile of work you're doing, but this impl return type, causes marker_driver_rustc to panic, when trying to lint with default test lint in marker_lints.

Seems like it's because of this TODO in here, just saying that you might want to get that fixed before merging this :)

Panic
thread '<unnamed>' panicked at 'not yet implemented: Ty {
    hir_id: HirId {
        owner: OwnerId {
            def_id: DefId(0:24 ~ cargo_marker[a207]::cli::check_command_args),
        },
        local_id: 37,
    },
    kind: OpaqueDef(
        ItemId {
            owner_id: OwnerId {
                def_id: DefId(0:165 ~ cargo_marker[a207]::cli::check_command_args::{opaque#0}),
            },
        },
        [],
        false,
    ),
    span: cargo-marker/src/cli.rs:54:28: 54:68 (#0),
}', marker_driver_rustc/src/conversion/ty.rs:88:17

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @Niki4tap, nice to see you in this repo. Welcome to marker!

not to pile up on an already huge pile of work you're doing

No worries at all. I appreciate reviews, it might take a bit more work, but that's better than fixing the code later again. It's also helpful to discuss alternative solutions.

The impl Trait type slipped my mind when I worked on the rustc backend. #55 includes a change in rustc that effects how impl Trait is represented. I waited for the nightly version until removing the Todo. Then life happened, and it's already the end of December ^^.

I might merge this PR before the nightly bump to avoid conflicts, but then I'll try to fix it in that PR :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welcome to marker!

Thanks!

I waited for the nightly version until removing the Todo. Then life happened, and it's already the end of December ^^.

Yeah, that tends to happen

I might merge this PR before the nightly bump to avoid conflicts, but then I'll try to fix it in that PR :)

No problem, just making sure you're aware of the issue :)

vec![
Arg::new("lints")
.short('l')
.long("lints")
.num_args(1..)
.value_parser(ValueParser::os_string())
.help("Defines a set of lints crates that should be used"),
]
}
186 changes: 186 additions & 0 deletions cargo-marker/src/driver.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,186 @@
//! 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.

use std::{ffi::OsString, path::PathBuf, 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<RustcDriverInfo> = 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,
}

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.
// 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
/// 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 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");

if !dev_build {
cmd.arg(&format!("+{toolchain}"));
}

if verbose {
cmd.arg("--verbose");
}

if dev_build {
cmd.args(["build", "--bin", "marker_driver_rustc"]);
} else {
// 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]);
}

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)
}
}

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 run_driver(
env: Vec<(OsString, OsString)>,
cargo_args: impl Iterator<Item = String>,
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()
.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
}
63 changes: 63 additions & 0 deletions cargo-marker/src/lints.rs
Original file line number Diff line number Diff line change
@@ -0,0 +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<PathBuf, ExitStatus> {
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)
}
}
Loading