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

Fixed toolchain version drift, improved error handling, config loading and refactoring. #217

Merged
merged 1 commit into from
Aug 18, 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
20 changes: 11 additions & 9 deletions Cargo.lock

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

6 changes: 4 additions & 2 deletions cargo-marker/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,17 @@ license = "MIT OR Apache-2.0"
repository = "https://github.com/rust-marker/marker"

# 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
# Cargo requires it's subcommands to use a dash like `cargo-marker`. Unable to
# rename the create on crates.io we now have this hack... At least it works
[[bin]]
name = "cargo-marker"
path = "src/main.rs"

[dependencies]
camino = { version = "1.1", features = ["serde1"] }
cargo_metadata = "0.15.4"
clap = { version = "4.0", features = ["string", "derive"] }
once_cell = "1.17.0"
serde = { version = "1.0", features = ["derive"] }
toml = { version = "0.7" }
serde_json = "1.0"
toml = "0.7"
6 changes: 2 additions & 4 deletions cargo-marker/src/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use crate::{config::LintDependencyEntry, ExitStatus};

use self::{lints::LintCrate, toolchain::Toolchain};

pub mod cargo;
pub mod driver;
pub mod lints;
pub mod toolchain;
Expand All @@ -37,8 +38,6 @@ pub struct Config {
pub build_rustc_flags: String,
/// Indicates if this is a release or debug build.
pub debug_build: bool,
/// Indicates if this is a development build.
pub dev_build: bool,
pub toolchain: Toolchain,
}

Expand All @@ -49,7 +48,6 @@ impl Config {
lints: HashMap::default(),
build_rustc_flags: String::new(),
debug_build: false,
dev_build: cfg!(feature = "dev-build"),
toolchain,
})
}
Expand Down Expand Up @@ -78,7 +76,7 @@ pub fn prepare_check(config: &Config) -> Result<CheckInfo, ExitStatus> {
("RUSTC_WORKSPACE_WRAPPER", config.toolchain.driver_path.as_os_str().to_os_string()),
("MARKER_LINT_CRATES", to_marker_lint_crates_env(&lints)),
];
if let Some(toolchain) = &config.toolchain.toolchain {
if let Some(toolchain) = &config.toolchain.cargo.toolchain {
env.push(("RUSTUP_TOOLCHAIN", toolchain.into()));
}

Expand Down
80 changes: 80 additions & 0 deletions cargo-marker/src/backend/cargo.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
use std::process::Command;

use camino::Utf8PathBuf;
use cargo_metadata::MetadataCommand;
use serde::Deserialize;

use crate::ExitStatus;

#[derive(Debug, Default)]
pub struct Cargo {
/// The rustc toolchain this driver belongs to. This can be `None` during
/// execution commands such as `cargo locate-project`
pub(crate) toolchain: Option<String>,
}

#[derive(Deserialize, Debug)]
struct ProjectLocation {
root: Utf8PathBuf,
}

impl Cargo {
pub fn with_toolchain(toolchain: impl Into<String>) -> Self {
Self {
toolchain: Some(toolchain.into()),
}
}

/// 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.
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.
// This also means that cargo needs to be invoked via rustup, to have
// the libraries available when Marker is invoked. This is such a mess...
// All of this would be so, so much simpler if marker was part of rustup :/
xFrednet marked this conversation as resolved.
Show resolved Hide resolved
if let Some(toolchain) = &self.toolchain {
let mut cmd = Command::new("rustup");
cmd.args(["run", toolchain, "cargo"]);
cmd
} else {
// for cargo locate-project - this command can be run without
// specified toolchain
Command::new("cargo")
}
}

pub fn cargo_locate_project(&self) -> Result<Utf8PathBuf, ExitStatus> {
let mut cmd = self.command();

let output = cmd
.arg("locate-project")
.arg("--workspace")
xFrednet marked this conversation as resolved.
Show resolved Hide resolved
.output()
.map_err(|err| ExitStatus::fatal(err, "error locating workspace manifest Cargo.toml"))?;

if !output.status.success() {
let stderr = String::from_utf8_lossy(&output.stderr);
return Err(ExitStatus::fatal(
stderr.trim(),
format!("cargo locate-project failed with {}", output.status),
));
}

let manifest_location: ProjectLocation = serde_json::from_slice(&output.stdout)
.map_err(|err| ExitStatus::fatal(err, "failed to deserialize cargo locate-project output"))?;

Ok(manifest_location.root)
}

// Keep self for future changes. It's implemented in such way that clippy
// doesn't ask to write it as an associative function.
#[allow(clippy::unused_self)]
pub fn metadata(&self) -> MetadataCommand {
MetadataCommand::new()
xFrednet marked this conversation as resolved.
Show resolved Hide resolved
}
}
6 changes: 3 additions & 3 deletions cargo-marker/src/backend/driver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::{path::Path, process::Command, str::from_utf8};

use once_cell::sync::Lazy;

use crate::ExitStatus;
use crate::{utils::is_local_driver, ExitStatus};

use super::toolchain::{get_toolchain_folder, rustup_which, Toolchain};

Expand Down Expand Up @@ -125,7 +125,7 @@ fn install_toolchain(toolchain: &str) -> Result<(), ExitStatus> {

/// This tries to compile the driver.
fn build_driver(toolchain: &str, version: &str, additional_rustc_flags: &str) -> Result<(), ExitStatus> {
if cfg!(debug_assertions) {
if is_local_driver() {
println!("Compiling rustc driver");
} else {
println!("Compiling rustc driver v{version} with {toolchain}");
Expand All @@ -135,7 +135,7 @@ fn build_driver(toolchain: &str, version: &str, additional_rustc_flags: &str) ->

// Build driver
let mut cmd = Command::new("cargo");
if cfg!(debug_assertions) {
if is_local_driver() {
cmd.args(["build", "--bin", "marker_rustc_driver"]);
} else {
cmd.env("RUSTUP_TOOLCHAIN", toolchain);
Expand Down
4 changes: 2 additions & 2 deletions cargo-marker/src/backend/lints/fetch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ const DUMMY_MAIN_CONTENT: &str = r#"
"#;

fn call_cargo_fetch(manifest: &Path, config: &Config) -> Result<(), ExitStatus> {
let mut cmd = config.toolchain.cargo_command();
let mut cmd = config.toolchain.cargo.command();
cmd.arg("fetch");
cmd.arg("--manifest-path");
cmd.arg(manifest.as_os_str());
Expand All @@ -141,7 +141,7 @@ fn call_cargo_fetch(manifest: &Path, config: &Config) -> Result<(), ExitStatus>
}

fn call_cargo_metadata(manifest: &PathBuf, config: &Config) -> Result<Metadata, ExitStatus> {
let res = config.toolchain.cargo_metadata_command().manifest_path(manifest).exec();
let res = config.toolchain.cargo.metadata().manifest_path(manifest).exec();

// FIXME(xFrednet): Handle errors properly.
res.map_err(|_| ExitStatus::LintCrateFetchFailed)
Expand Down
Loading