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

rust-project check TARGET #767

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
144 changes: 123 additions & 21 deletions integrations/rust-project/src/buck.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ pub(crate) fn to_json_project(
aliases: FxHashMap<Target, AliasedTargetInfo>,
relative_paths: bool,
check_cycles: bool,
use_clippy: bool,
) -> Result<JsonProject, anyhow::Error> {
let mode = select_mode(None);
let buck = Buck::new(mode);
Expand Down Expand Up @@ -94,7 +95,7 @@ pub(crate) fn to_json_project(

let mut crates: Vec<Crate> = Vec::with_capacity(targets_vec.len());
for target in &targets_vec {
let info = target_index.get(&target).unwrap();
let info = target_index.get(target).unwrap();

let dep_targets = resolve_aliases(&info.deps, &aliases, &proc_macros);
let deps = as_deps(&dep_targets, info, &targets_to_ids, &target_index);
Expand Down Expand Up @@ -194,10 +195,35 @@ pub(crate) fn to_json_project(
check_cycles_in_crate_graph(&crates);
}

let jp = JsonProject {
sysroot,
crates,
runnables: vec![
let mut runnables = vec![
Runnable {
kind: RunnableKind::Flycheck,
Copy link
Contributor

Choose a reason for hiding this comment

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

whoops, i thought i called this check. we should probably resolve this in rust-analyzer, but i'm not sure in favor of what.

Copy link
Contributor Author

@cormacrelf cormacrelf Sep 6, 2024

Choose a reason for hiding this comment

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

I think check should, in VSCode, open the terminal with the text rendered diagnostics from cargo/buck etc. Very often the inline diagnostics from LSP do not live up to the standard set by rustc's text renderer and you need to see it in the terminal to understand what rustc is honking about. Flycheck should be for the JSON output.

The existing "check" runnable just runs buck2 build, which is what I'm describing already.

program: "rust-project".to_owned(),
args: {
let mut args = vec!["check".to_owned(), "{label}".to_owned()];
if !use_clippy {
args.push("--use-clippy".to_owned());
args.push("false".to_owned());
}
args
},
cwd: project_root.clone(),
},
Runnable {
kind: RunnableKind::Run,
program: "buck2".to_string(),
args: vec![
"run".to_owned(),
"-c=client.id=rust-project".to_owned(),
"{label}".to_string(),
],
cwd: project_root.clone(),
},
];

#[cfg(fbcode_build)]
{
runnables.extend([
Runnable {
program: "buck".to_owned(),
args: vec![
Expand All @@ -219,9 +245,48 @@ pub(crate) fn to_json_project(
"--print-passing-details".to_owned(),
],
cwd: project_root.to_owned(),
},
]);
}

// OSS `buck2 test` supports different args, chiefly --test-arg
//
#[cfg(not(fbcode_build))]
{
runnables.extend([
#[cfg(not(fbcode_build))]
Runnable {
kind: RunnableKind::TestOne,
program: "buck2".to_string(),
args: vec![
"test".to_owned(),
"-c=client.id=rust-project".to_owned(),
"{label}".to_owned(),
"--".to_owned(),
// R-A substitutes {test_id} with e.g. `mycrate::tests::one`
// --test-arg tells `buck2 test` to pass that string through to
// the test program, which we will assume to be the rust test
// harness. Same overall effect as `cargo test -- mycrate::tests::one`,
//
// But this is a bit less than ideal, because buck will build
// all of the related test binaries, including integration tests.
// We don't know your naming conventions for library tests.
// You might have a rust_test target named `mycrate-test`. So
// we might need a way (probably buck metadata in the library
// target, or just querying the related tests)
// to tell rust-project about these.
"--test-arg".to_owned(),
"{test_id}".to_owned(),
],
cwd: project_root.clone(),
},
],
]);
}

let jp = JsonProject {
sysroot,
crates,
runnables,
// needed to ignore the generated `rust-project.json` in diffs, but including the actual
// string will mark this file as generated
generated: String::from("\x40generated"),
Expand Down Expand Up @@ -485,6 +550,24 @@ impl Buck {
cmd
}

/// Return the absolute path of the CELL root for a given buck target label
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Return the absolute path of the CELL root for a given buck target label
/// Return the absolute path of the cell root for a given target.

pub(crate) fn resolve_target_cell_root(&self, label: &str) -> Result<PathBuf, anyhow::Error> {
let (cell, _path) = label
.split_once("//")
.unwrap_or(("root", label.trim_start_matches("//")));

let mut command = self.command_without_config(["audit", "cell", "--paths-only", cell]);

let mut stdout = utf8_output(command.output(), &command)?;
truncate_line_ending(&mut stdout);

if enabled!(Level::TRACE) {
trace!(%stdout, target=%label, cell=%cell, "got cell root from buck");
}

Ok(stdout.into())
}

/// Return the absolute path of the current Buck project root.
pub(crate) fn resolve_project_root(&self) -> Result<PathBuf, anyhow::Error> {
let mut command = self.command_without_config(["root"]);
Expand Down Expand Up @@ -558,17 +641,10 @@ impl Buck {

command.arg("prelude//rust/rust-analyzer/check.bxl:check");

let mut file_path = saved_file.to_owned();
if !file_path.is_absolute() {
if let Ok(cwd) = std::env::current_dir() {
file_path = cwd.join(saved_file);
}
}

// apply BXL scripts-specific arguments:
command.args(["--", "--file"]);
command.arg(file_path.as_os_str());

command.arg("--");
command.args(["--file"]);
command.arg(saved_file.as_os_str());
command.args(["--use-clippy", &use_clippy.to_string()]);

// Set working directory to the containing directory of the target file.
Expand All @@ -578,17 +654,40 @@ impl Buck {
command.current_dir(parent_dir);
}

tracing::debug!(?command, "running bxl");

let output = command.output();
if let Ok(output) = &output {
if output.stdout.is_empty() {
return Ok(vec![]);
}
}

let files = deserialize_output(output, &command)?;
Ok(files)
}

#[instrument(fields(use_clippy, target = %target))]
pub(crate) fn check_target(
&self,
use_clippy: bool,
target: &str,
) -> Result<Vec<PathBuf>, anyhow::Error> {
let mut command = self.command(["bxl"]);

if let Some(mode) = &self.mode {
command.arg(mode);
}

command.arg("prelude//rust/rust-analyzer/check.bxl:check");

command.arg("--");
command.arg("--target");
command.arg(target);
command.args(["--use-clippy", &use_clippy.to_string()]);

tracing::debug!(?command, "running bxl");

let output = command.output();

let files = deserialize_output(output, &command)?;
Ok(files)
}
#[instrument(skip_all)]
pub(crate) fn expand_and_resolve(
&self,
Expand All @@ -611,6 +710,9 @@ impl Buck {
"--targets",
]);
command.args(targets);

tracing::debug!(?command, "running bxl");

deserialize_output(command.output(), &command)
}

Expand Down
16 changes: 6 additions & 10 deletions integrations/rust-project/src/cli/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
*/

use std::path::Path;
use std::path::PathBuf;
use std::str::FromStr;

use tracing::info;
Expand All @@ -17,34 +16,31 @@ use tracing::instrument;
use crate::buck;
use crate::buck::select_mode;
use crate::diagnostics;
use crate::path::canonicalize;

pub(crate) struct Check {
pub(crate) buck: buck::Buck,
pub(crate) use_clippy: bool,
pub(crate) saved_file: PathBuf,
pub(crate) target: String,
}

impl Check {
pub(crate) fn new(mode: Option<String>, use_clippy: bool, saved_file: PathBuf) -> Self {
let saved_file = canonicalize(&saved_file).unwrap_or(saved_file);

pub(crate) fn new(mode: Option<String>, use_clippy: bool, target: String) -> Self {
let mode = select_mode(mode.as_deref());
let buck = buck::Buck::new(mode);
Self {
buck,
use_clippy,
saved_file,
target,
}
}

#[instrument(name = "check", skip_all, fields(saved_file = %self.saved_file.display()))]
#[instrument(name = "check", skip_all, fields(target = %self.target))]
pub(crate) fn run(&self) -> Result<(), anyhow::Error> {
let start = std::time::Instant::now();
let buck = &self.buck;

let cell_root = buck.resolve_root_of_file(&self.saved_file)?;
let diagnostic_files = buck.check_saved_file(self.use_clippy, &self.saved_file)?;
let cell_root = buck.resolve_target_cell_root(&self.target)?;
let diagnostic_files = buck.check_target(self.use_clippy, &self.target)?;

let mut diagnostics = vec![];
for path in diagnostic_files {
Expand Down
Loading