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

Fail early if required tools can't be found. #1640

Merged
merged 1 commit into from
Dec 17, 2024
Merged

Conversation

duckinator
Copy link
Contributor

@duckinator duckinator commented Dec 13, 2024

image

text version of the above screenshot
~/test/sometest main+ dist build

  × The following tools are required to run this task, but are missing:
  │ - cargo-auditable
  │ - cargo-cyclonedx
  │ - omnibor-cli
  help: Please install the tools mentioned above and try again.

~/test/sometest main+ dist build --target x86_64-pc-windows-msvc

  × The following tools are required to run this task, but are missing:
  │ - cargo-auditable
  │ - cargo-cyclonedx
  │ - omnibor-cli
  │ - cargo-xwin
  help: Please install the tools mentioned above and try again.

@duckinator

This comment was marked as outdated.

@duckinator

This comment was marked as outdated.

@duckinator duckinator changed the title [WIP] Bail early if required tools can't be found. Fail early if required tools can't be found. Dec 16, 2024
Copy link
Contributor

@mistydemeo mistydemeo left a comment

Choose a reason for hiding this comment

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

Looks like this has tripped up the tests - let's take a look and make sure we're not introducing tool dep bounds on things that don't actually need them.

cargo-dist/src/lib.rs Show resolved Hide resolved
cargo-dist/src/tasks.rs Outdated Show resolved Hide resolved
cargo-dist/src/lib.rs Outdated Show resolved Hide resolved
cargo-dist/src/lib.rs Outdated Show resolved Hide resolved
cargo-dist/src/lib.rs Outdated Show resolved Hide resolved
@duckinator duckinator force-pushed the missing-tools branch 2 times, most recently from 09b5f3f to 1e2133e Compare December 16, 2024 21:22
cargo-dist/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@mistydemeo mistydemeo left a comment

Choose a reason for hiding this comment

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

Looking good. It might be nice to handle Cargo itself at some point, for users building non-Rust software, but this all looks great.

@duckinator
Copy link
Contributor Author

Turns out Cargo is handled separately, and tasks::gather_work() can't succeed unless we either don't need or need and have Cargo, so checking for it in do_env_test doesn't make much sense.

As a note for the future, in case that changes, the approach I found that would theoretically work (if the code ever got run) is adding Tools::cargo_tool() -> DistResult<&Tool> alongside Tools::cargo() -> DistResult<&CargoInfo>, then in do_env_test() you can treat it as a normal tool but use .cargo_tool() instead of .cargo().

diff
diff --git a/cargo-dist/src/lib.rs b/cargo-dist/src/lib.rs
index b02525e7..7c1906ed 100644
--- a/cargo-dist/src/lib.rs
+++ b/cargo-dist/src/lib.rs
@@ -70,6 +70,7 @@ pub fn do_env_test(cfg: &Config) -> DistResult<()> {
     let need_omnibor = builds.omnibor;
     let mut need_xwin = false;
     let mut need_zigbuild = false;
+    let mut need_cargo = false;
 
     let tools = dist.tools;
     let host = tools.host_target.parse()?;
@@ -82,6 +83,8 @@ pub fn do_env_test(cfg: &Config) -> DistResult<()> {
 
         match step {
             BuildStep::Cargo(step) => {
+                need_cargo = true;
+
                 let target = step.target_triple.parse()?;
                 let wrapper = tasks::build_wrapper_for_cross(&host, &target)?;
 
@@ -104,6 +107,7 @@ pub fn do_env_test(cfg: &Config) -> DistResult<()> {
     // bool::then(f) returns an Option, so we start with a
     // Vec<Option<Result<&Tool, DistResult>>>.
     let all_tools: Vec<Option<DistResult<&Tool>>> = vec![
+        need_cargo.then(|| tools.cargo_tool()),
         need_cargo_auditable.then(|| tools.cargo_auditable()),
         need_cargo_cyclonedx.then(|| tools.cargo_cyclonedx()),
         need_omnibor.then(|| tools.omnibor()),
diff --git a/cargo-dist/src/tasks.rs b/cargo-dist/src/tasks.rs
index 4c83ec88..2dfad37e 100644
--- a/cargo-dist/src/tasks.rs
+++ b/cargo-dist/src/tasks.rs
@@ -289,9 +289,20 @@ pub struct Tools {
     pub cargo_xwin: Option<Tool>,
     /// cargo-zigbuild, for some cross builds
     pub cargo_zigbuild: Option<Tool>,
+
+    // This is a kludge to avoid type issues. See cargo_tool() docs.
+    cargo_tool: Option<Tool>,
 }
 
 impl Tools {
+    /// Return the cargo info, as a Tool instead of a CargoInfo, or an error.
+    /// This is a kludge to avoid needing to handle type issues.
+    pub fn cargo_tool(&self) -> DistResult<&Tool> {
+        self.cargo_tool.as_ref().ok_or(DistError::ToolMissing {
+            tool: "cargo".to_owned(),
+        })
+    }
+
     /// Returns the cargo info or an error
     pub fn cargo(&self) -> DistResult<&CargoInfo> {
         self.cargo.as_ref().ok_or(DistError::ToolMissing {
@@ -3224,6 +3235,8 @@ fn tool_info() -> DistResult<Tools> {
         cargo_cyclonedx: find_cargo_subcommand("cargo", "cyclonedx", "--version"),
         cargo_xwin: find_cargo_subcommand("cargo", "xwin", "--version"),
         cargo_zigbuild: find_tool("cargo-zigbuild", "--version"),
+
+        cargo_tool: find_tool("cargo", "-V"),
     })
 }

@duckinator duckinator merged commit 73148c3 into main Dec 17, 2024
18 checks passed
@duckinator duckinator deleted the missing-tools branch December 17, 2024 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants