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

Simplify lint crates build to a single cargo build invocation #250

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft
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 Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -64,4 +64,4 @@ visibility = "0.0.1"
yansi = "1.0.0-rc.1"

[workspace.metadata.marker.lints]
marker_lints = { path = "./marker_lints" }
marker_lints2 = { path = "./marker_lints", package = "marker_lints" }
14 changes: 3 additions & 11 deletions cargo-marker/src/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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.
Expand All @@ -68,9 +60,9 @@ pub struct CheckInfo {

pub fn prepare_check(config: &Config) -> Result<CheckInfo> {
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]
Expand Down
9 changes: 3 additions & 6 deletions cargo-marker/src/backend/cargo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
170 changes: 148 additions & 22 deletions cargo-marker/src/backend/lints.rs
Original file line number Diff line number Diff line change
@@ -1,37 +1,163 @@
use super::Config;
mod build_space;

use crate::backend::Config;
use crate::error::prelude::*;
use crate::observability::prelude::*;
use crate::utils::utf8::IntoUtf8;
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 itertools::Itertools;
use std::collections::BTreeMap;
use std::env::consts::DLL_EXTENSION;
use std::process::Stdio;

/// 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<Vec<LintCrate>> {
pub(crate) fn build(config: &Config) -> Result<Vec<LintDll>> {
// 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.iter().flat_map(|(name, entry)| {
let package = entry.package.as_deref().unwrap_or(name);
["-p", package]
});

let mut cmd = config.toolchain.cargo.command();
cmd.current_dir(&config.marker_dir);
cmd.arg("build");
cmd.arg("--lib");
cmd.arg("--message-format");
cmd.arg("json");
cmd.args(packages);

// Potential "--release" flag
if !config.debug_build {
cmd.arg("--release");
}

// Environment
cmd.env("RUSTFLAGS", &config.build_rustc_flags);

let output = cmd
.log()
.stdin(Stdio::null())
.stdout(Stdio::piped())
.spawn()
.expect("could not run cargo")
.wait_with_output()
.expect("failed to wait for cargo?");

if !output.status.success() {
return Err(Error::root("Failed to compile the lint crates"));
}

let output = output.stdout.into_utf8()?;

// Parse the output of `cargo build --message-format json` to find the
// compiled dynamic libraries.
let mut dlls: BTreeMap<_, _> = output
.lines()
.map(|line| {
serde_json::from_str(&line).context(|| {
format!(
"Failed to parse `cargo build` json output line:\n\
---\n{line}\n---"
)
})
})
.filter_map_ok(|message| {
let cargo_metadata::Message::CompilerArtifact(artifact) = message else {
return None;
};

if !artifact.target.kind.iter().any(|kind| kind == "cdylib") {
return None;
}

let file_name = artifact
.filenames
.into_iter()
.find(|file| file.extension() == Some(DLL_EXTENSION))?;

Some((artifact.package_id, file_name))
})
.collect::<Result<_>>()?;

let meta = config
.toolchain
.cargo
.metadata()
.current_dir(&config.marker_dir)
.exec()
.context(|| "Failed to run `cargo metadata` in the lint crates build space")?;

// If manipulations with all the various metadata structures fail, we
// may ask the user to enable the trace loggin to help us debug the issue.
trace!(
target: "build_space_metadata",
compiler_messages = %output,
cargo_metadata = %serde_json::to_string(&meta).unwrap(),
"Build space metadata"
);

let root_package = meta
.root_package()
.context(|| "The build space must contain a root package, but it doesn't")?
.clone();

let root_package = meta
.resolve
.context(|| "Dependency graph from `cargo metadata` in the build space was not resolved")?
.nodes
.into_iter()
.find(|node| node.id == root_package.id)
.context(|| {
format!(
"The root package `{}` was not found in the dependency graph",
root_package.name,
)
})?;

config
.lints
.keys()
.map(|name| {
let package_id = &root_package
.deps
.iter()
// FIXME: thsi doesn't work because the name is of the library target
.find(|dep| dep.name == *name)
.context(|| format!("The lint crate `{name}` was not found in the dependency graph"))?
.pkg;

let file = dlls.remove(package_id).context(|| {
format!(
"Failed to find the dll for `{package_id}`.\n\
The following dlls are available: {dlls:#?}",
)
})?;

Ok(LintDll {
name: name.clone(),
file,
})
})
.collect()
}
144 changes: 0 additions & 144 deletions cargo-marker/src/backend/lints/build.rs

This file was deleted.

Loading