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

refactor: Use compilation target instead of build machine target for MSRV checks #825

Merged
merged 3 commits into from
Nov 12, 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
4 changes: 2 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ the [issue tracker](https://github.com/foresterre/cargo-msrv/issues), or open a
* `cargo-msrv` now requires paths to be UTF-8.
* `--write-msrv` now writes two, instead of three component version numbers .


#### Infra

* Changed release artifact name of `cargo-msrv` packages on Github, such that they can be installed with `cargo-binstall` out of the box.
Expand All @@ -41,6 +40,7 @@ the [issue tracker](https://github.com/foresterre/cargo-msrv/issues), or open a
* The program will no longer return an unformatted message when a command failed and the output format was set to json.
* Fix issue where reading the fallback MSRV from a TOML inline table was not possible.
* Fix an index out-of-bounds panic which occurred if the filtered Rust releases search space was empty
* Use compilation target instead of build machine target for MSRV checks

### Removed

Expand Down Expand Up @@ -70,7 +70,7 @@ This release does not contain user-facing changes, hence the lack of changelog e
### Changed

* ⚠️ Breaking change: Changed default cargo-msrv (find) check command from `cargo check --all` to `cargo check`.
* Revert to the old behaviour by running cargo-msrv with a custom check command: `cargo msrv -- cargo check --all`.
* To revert to the old behaviour, run cargo-msrv with the following custom check command: `cargo msrv -- cargo check --all`.

### Removed

Expand Down
3 changes: 2 additions & 1 deletion src/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ mod rustup_toolchain_check;
mod testing;

use crate::{Outcome, TResult};
pub use rustup_toolchain_check::RustupToolchainCheck;
pub use rustup_toolchain_check::{RunCommand, RustupToolchainCheck};

#[cfg(test)]
pub use testing::TestRunner;

Expand Down
233 changes: 132 additions & 101 deletions src/check/rustup_toolchain_check.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::check::Check;
use crate::command::RustupCommand;
use crate::context::{CheckCmdContext, EnvironmentContext};
use crate::context::EnvironmentContext;
use crate::download::{DownloadToolchain, ToolchainDownloader};
use crate::error::{IoError, IoErrorSource};
use crate::lockfile::LockfileHandler;
Expand All @@ -9,12 +9,32 @@
use crate::{lockfile, CargoMSRVError, Outcome, Reporter, TResult};
use camino::{Utf8Path, Utf8PathBuf};

pub struct RustupToolchainCheck<'reporter, 'env, 'cc, R: Reporter> {
pub struct RustupToolchainCheck<'reporter, 'env, R: Reporter> {
reporter: &'reporter R,
settings: Settings<'env, 'cc>,
settings: Settings<'env>,
}

impl<'reporter, 'env, 'cc, R: Reporter> Check for RustupToolchainCheck<'reporter, 'env, 'cc, R> {
impl<'reporter, 'env, R: Reporter> RustupToolchainCheck<'reporter, 'env, R> {
pub fn new(
reporter: &'reporter R,
ignore_lockfile: bool,
no_check_feedback: bool,
environment: &'env EnvironmentContext,
run_command: RunCommand,
) -> Self {
Self {
reporter,
settings: Settings {
ignore_lockfile,
no_check_feedback,
environment,
check_cmd: run_command,
},
}
}

Check warning on line 34 in src/check/rustup_toolchain_check.rs

View check run for this annotation

Codecov / codecov/patch

src/check/rustup_toolchain_check.rs#L18-L34

Added lines #L18 - L34 were not covered by tests
}

impl<'reporter, 'env, R: Reporter> Check for RustupToolchainCheck<'reporter, 'env, R> {
fn check(&self, toolchain: &ToolchainSpec) -> TResult<Outcome> {
let settings = &self.settings;

Expand All @@ -28,22 +48,24 @@
.map(|handle| handle.move_lockfile())
.transpose()?;

self.setup_toolchain(toolchain)?;
setup_toolchain(self.reporter, toolchain)?;

Check warning on line 51 in src/check/rustup_toolchain_check.rs

View check run for this annotation

Codecov / codecov/patch

src/check/rustup_toolchain_check.rs#L51

Added line #L51 was not covered by tests

if handle_wrap.is_some() {
remove_lockfile(&settings.lockfile_path())?;
}

let crate_root = settings.crate_root_path();
let cmd = &self.settings.check_cmd;

Check warning on line 58 in src/check/rustup_toolchain_check.rs

View check run for this annotation

Codecov / codecov/patch

src/check/rustup_toolchain_check.rs#L58

Added line #L58 was not covered by tests

let outcome = self.run_check_command_via_rustup(
let outcome = run_check_command_via_rustup(
self.reporter,

Check warning on line 61 in src/check/rustup_toolchain_check.rs

View check run for this annotation

Codecov / codecov/patch

src/check/rustup_toolchain_check.rs#L60-L61

Added lines #L60 - L61 were not covered by tests
toolchain,
crate_root,
settings.check_cmd.rustup_command.iter().map(|s| s.as_str()),
cmd.components(),

Check warning on line 64 in src/check/rustup_toolchain_check.rs

View check run for this annotation

Codecov / codecov/patch

src/check/rustup_toolchain_check.rs#L64

Added line #L64 was not covered by tests
)?;

// report outcome to UI
self.report_outcome(&outcome, settings.no_check_feedback())?;
report_outcome(self.reporter, &outcome, settings.no_check_feedback())?;

Check warning on line 68 in src/check/rustup_toolchain_check.rs

View check run for this annotation

Codecov / codecov/patch

src/check/rustup_toolchain_check.rs#L68

Added line #L68 was not covered by tests

// move the lockfile back
if let Some(handle) = handle_wrap {
Expand All @@ -55,103 +77,87 @@
}
}

impl<'reporter, 'env, 'cc, R: Reporter> RustupToolchainCheck<'reporter, 'env, 'cc, R> {
pub fn new(
reporter: &'reporter R,
ignore_lockfile: bool,
no_check_feedback: bool,
environment: &'env EnvironmentContext,
check_cmd: &'cc CheckCmdContext,
) -> Self {
Self {
reporter,
settings: Settings {
ignore_lockfile,
no_check_feedback,
environment,
check_cmd,
},
}
}
fn setup_toolchain(reporter: &impl Reporter, toolchain: &ToolchainSpec) -> TResult<()> {
let downloader = ToolchainDownloader::new(reporter);
downloader.download(toolchain)?;

Check warning on line 82 in src/check/rustup_toolchain_check.rs

View check run for this annotation

Codecov / codecov/patch

src/check/rustup_toolchain_check.rs#L80-L82

Added lines #L80 - L82 were not covered by tests

fn setup_toolchain(&self, toolchain: &ToolchainSpec) -> TResult<()> {
let downloader = ToolchainDownloader::new(self.reporter);
downloader.download(toolchain)?;
Ok(())
}

Check warning on line 85 in src/check/rustup_toolchain_check.rs

View check run for this annotation

Codecov / codecov/patch

src/check/rustup_toolchain_check.rs#L84-L85

Added lines #L84 - L85 were not covered by tests

Ok(())
}
fn run_check_command_via_rustup(
reporter: &impl Reporter,
toolchain: &ToolchainSpec,
dir: &Utf8Path,
check: &[String],
) -> TResult<Outcome> {
let version = format!("{}", toolchain.version());
let mut cmd = vec![version.as_str()];
cmd.extend(check.iter().map(|s| s.as_str()));

reporter.report_event(CheckMethod::new(
toolchain.to_owned(),
Method::rustup_run(&cmd, dir),
))?;

Check warning on line 100 in src/check/rustup_toolchain_check.rs

View check run for this annotation

Codecov / codecov/patch

src/check/rustup_toolchain_check.rs#L87-L100

Added lines #L87 - L100 were not covered by tests

let rustup_output = RustupCommand::new()
.with_args(cmd.iter())
.with_dir(dir)
.with_stderr()
.run()
.map_err(|_| CargoMSRVError::UnableToRunCheck {
command: cmd[1..].join(" "),
cwd: dir.to_path_buf(),
})?;

Check warning on line 110 in src/check/rustup_toolchain_check.rs

View check run for this annotation

Codecov / codecov/patch

src/check/rustup_toolchain_check.rs#L102-L110

Added lines #L102 - L110 were not covered by tests

let status = rustup_output.exit_status();

Check warning on line 112 in src/check/rustup_toolchain_check.rs

View check run for this annotation

Codecov / codecov/patch

src/check/rustup_toolchain_check.rs#L112

Added line #L112 was not covered by tests

fn run_check_command_via_rustup<'arg>(
&self,
toolchain: &'arg ToolchainSpec,
dir: &Utf8Path,
check: impl Iterator<Item = &'arg str>,
) -> TResult<Outcome> {
let mut cmd: Vec<&str> = vec![toolchain.spec()];
cmd.extend(check);
if status.success() {
Ok(Outcome::new_success(toolchain.to_owned()))

Check warning on line 115 in src/check/rustup_toolchain_check.rs

View check run for this annotation

Codecov / codecov/patch

src/check/rustup_toolchain_check.rs#L114-L115

Added lines #L114 - L115 were not covered by tests
} else {
let stderr = rustup_output.stderr();
let command = cmd.join(" ");

Check warning on line 118 in src/check/rustup_toolchain_check.rs

View check run for this annotation

Codecov / codecov/patch

src/check/rustup_toolchain_check.rs#L117-L118

Added lines #L117 - L118 were not covered by tests

self.reporter.report_event(CheckMethod::new(
info!(
?toolchain,
stderr,
cmd = command.as_str(),
"try_building run failed"
);

Check warning on line 125 in src/check/rustup_toolchain_check.rs

View check run for this annotation

Codecov / codecov/patch

src/check/rustup_toolchain_check.rs#L120-L125

Added lines #L120 - L125 were not covered by tests

Ok(Outcome::new_failure(

Check warning on line 127 in src/check/rustup_toolchain_check.rs

View check run for this annotation

Codecov / codecov/patch

src/check/rustup_toolchain_check.rs#L127

Added line #L127 was not covered by tests
toolchain.to_owned(),
Method::rustup_run(&cmd, dir),
))?;

let rustup_output = RustupCommand::new()
.with_args(cmd.iter())
.with_dir(dir)
.with_stderr()
.run()
.map_err(|_| CargoMSRVError::UnableToRunCheck {
command: cmd[1..].join(" "),
cwd: dir.to_path_buf(),
})?;

let status = rustup_output.exit_status();

if status.success() {
Ok(Outcome::new_success(toolchain.to_owned()))
} else {
let stderr = rustup_output.stderr();
let command = cmd.join(" ");

info!(
?toolchain,
stderr,
cmd = command.as_str(),
"try_building run failed"
);

Ok(Outcome::new_failure(
toolchain.to_owned(),
stderr.to_string(),
))
}
stderr.to_string(),
))

Check warning on line 130 in src/check/rustup_toolchain_check.rs

View check run for this annotation

Codecov / codecov/patch

src/check/rustup_toolchain_check.rs#L129-L130

Added lines #L129 - L130 were not covered by tests
}
}

Check warning on line 132 in src/check/rustup_toolchain_check.rs

View check run for this annotation

Codecov / codecov/patch

src/check/rustup_toolchain_check.rs#L132

Added line #L132 was not covered by tests

fn report_outcome(&self, outcome: &Outcome, no_error_report: bool) -> TResult<()> {
match outcome {
Outcome::Success(outcome) => {
// report compatibility with this toolchain
self.reporter
.report_event(CheckResult::compatible(outcome.toolchain_spec.to_owned()))?
}
Outcome::Failure(outcome) if no_error_report => {
// report incompatibility with this toolchain
self.reporter.report_event(CheckResult::incompatible(
outcome.toolchain_spec.to_owned(),
None,
))?
}
Outcome::Failure(outcome) => {
// report incompatibility with this toolchain
self.reporter.report_event(CheckResult::incompatible(
outcome.toolchain_spec.to_owned(),
Some(outcome.error_message.clone()),
))?
}
};

Ok(())
}
fn report_outcome(
reporter: &impl Reporter,
outcome: &Outcome,
no_error_report: bool,
) -> TResult<()> {
match outcome {
Outcome::Success(outcome) => {
// report compatibility with this toolchain
reporter.report_event(CheckResult::compatible(outcome.toolchain_spec.to_owned()))?

Check warning on line 142 in src/check/rustup_toolchain_check.rs

View check run for this annotation

Codecov / codecov/patch

src/check/rustup_toolchain_check.rs#L134-L142

Added lines #L134 - L142 were not covered by tests
}
Outcome::Failure(outcome) if no_error_report => {
// report incompatibility with this toolchain
reporter.report_event(CheckResult::incompatible(
outcome.toolchain_spec.to_owned(),
None,
))?

Check warning on line 149 in src/check/rustup_toolchain_check.rs

View check run for this annotation

Codecov / codecov/patch

src/check/rustup_toolchain_check.rs#L144-L149

Added lines #L144 - L149 were not covered by tests
}
Outcome::Failure(outcome) => {
// report incompatibility with this toolchain
reporter.report_event(CheckResult::incompatible(
outcome.toolchain_spec.to_owned(),
Some(outcome.error_message.clone()),
))?

Check warning on line 156 in src/check/rustup_toolchain_check.rs

View check run for this annotation

Codecov / codecov/patch

src/check/rustup_toolchain_check.rs#L151-L156

Added lines #L151 - L156 were not covered by tests
}
};

Ok(())

Check warning on line 160 in src/check/rustup_toolchain_check.rs

View check run for this annotation

Codecov / codecov/patch

src/check/rustup_toolchain_check.rs#L160

Added line #L160 was not covered by tests
}

/// Creates a lockfile handle, iff the lockfile exists and the user opted
Expand All @@ -177,15 +183,15 @@
Ok(())
}

struct Settings<'env, 'cc> {
struct Settings<'env> {
ignore_lockfile: bool,
no_check_feedback: bool,

environment: &'env EnvironmentContext,
check_cmd: &'cc CheckCmdContext,
check_cmd: RunCommand,
}

impl<'env, 'cc> Settings<'env, 'cc> {
impl<'env> Settings<'env> {
pub fn ignore_lockfile(&self) -> bool {
self.ignore_lockfile
}
Expand All @@ -202,3 +208,28 @@
self.environment.lock()
}
}

pub struct RunCommand {
command: Vec<String>,
}

impl RunCommand {
pub fn default(target: impl ToString) -> Self {
let command = vec![
"cargo".to_string(),
"check".to_string(),
"--target".to_string(),
target.to_string(),
];

Self { command }
}

pub fn custom(command: Vec<String>) -> Self {
Self { command }
}

Check warning on line 230 in src/check/rustup_toolchain_check.rs

View check run for this annotation

Codecov / codecov/patch

src/check/rustup_toolchain_check.rs#L228-L230

Added lines #L228 - L230 were not covered by tests

pub fn components(&self) -> &[String] {
self.command.as_ref()
}
}
4 changes: 2 additions & 2 deletions src/cli/custom_check_opts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,6 @@
#[command(next_help_heading = "Custom check options")]
pub struct CustomCheckOpts {
/// Supply a custom `check` command to be used by cargo msrv
#[arg(last = true, required = false)]
pub custom_check_command: Vec<String>,
#[arg(last = true)]
pub custom_check_command: Option<Vec<String>>,

Check warning on line 8 in src/cli/custom_check_opts.rs

View check run for this annotation

Codecov / codecov/patch

src/cli/custom_check_opts.rs#L8

Added line #L8 was not covered by tests
}
4 changes: 4 additions & 0 deletions src/command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@
self.execute(OsStr::new("show"))
}

pub fn target(self) -> TResult<RustupOutput> {
self.execute(OsStr::new("target"))
}

Check warning on line 61 in src/command.rs

View check run for this annotation

Codecov / codecov/patch

src/command.rs#L59-L61

Added lines #L59 - L61 were not covered by tests

/// Execute a given `rustup` command.
///
/// See also:
Expand Down
20 changes: 5 additions & 15 deletions src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
//!
//! Unlike the opts, the context is top down, not bottom up.

use crate::cli::custom_check_opts::CustomCheckOpts;
use crate::cli::rust_releases_opts::RustReleasesOpts;
use crate::cli::shared_opts::{SharedOpts, UserOutputOpts};
use crate::cli::toolchain_opts::ToolchainOpts;
Expand All @@ -26,6 +25,7 @@
pub mod show;
pub mod verify;

use crate::cli::custom_check_opts::CustomCheckOpts;
use crate::cli::rust_releases_opts::Edition;
use crate::cli::{CargoMsrvOpts, SubCommand};
use crate::default_target::default_target;
Expand Down Expand Up @@ -193,24 +193,14 @@
#[derive(Debug)]
pub struct CheckCmdContext {
/// The custom `Rustup` command to invoke for a toolchain.
pub rustup_command: Vec<String>,
pub rustup_command: Option<Vec<String>>,
}

impl From<CustomCheckOpts> for CheckCmdContext {
fn from(opts: CustomCheckOpts) -> Self {
let rustup_command = if opts.custom_check_command.is_empty() {
vec!["cargo".to_string(), "check".to_string()]
} else {
opts.custom_check_command
};

Self { rustup_command }
}
}

impl CheckCmdContext {
pub fn custom_rustup_command(&self) -> &[String] {
&self.rustup_command
Self {
rustup_command: opts.custom_check_command,
}

Check warning on line 203 in src/context.rs

View check run for this annotation

Codecov / codecov/patch

src/context.rs#L201-L203

Added lines #L201 - L203 were not covered by tests
}
}

Expand Down
Loading
Loading