remove extra caching logic from run and re-structure the changes

Author: Shourya742

src/bootstrap/src/utils/exec.rs

@@ -2,15 +2,12 @@
 //!
 //! This module provides a structured way to execute and manage commands efficiently,
 //! ensuring controlled failure handling and output management.
-#![allow(warnings)]
 
-use std::collections::HashMap;
 use std::ffi::{OsStr, OsString};
 use std::fmt::{Debug, Formatter};
-use std::hash::{Hash, Hasher};
+use std::hash::Hash;
 use std::path::Path;
 use std::process::{Command, CommandArgs, CommandEnvs, ExitStatus, Output, Stdio};
-use std::sync::Mutex;
 
 use build_helper::ci::CiEnv;
 use build_helper::drop_bomb::DropBomb;
@@ -59,7 +56,7 @@ impl OutputMode {
 pub struct CommandCacheKey {
     program: OsString,
     args: Vec<OsString>,
-    envs: Vec<(OsString, OsString)>,
+    envs: Vec<(OsString, Option<OsString>)>,
     cwd: Option<PathBuf>,
 }
 
@@ -90,18 +87,14 @@ pub struct BootstrapCommand {
 impl<'a> BootstrapCommand {
     #[track_caller]
     pub fn new<S: AsRef<OsStr>>(program: S) -> Self {
-        Self { should_cache: true, ..Command::new(program).into() }
+        Command::new(program).into()
     }
     pub fn arg<S: AsRef<OsStr>>(&mut self, arg: S) -> &mut Self {
         self.command.arg(arg.as_ref());
         self
     }
 
-    pub fn should_cache(&self) -> bool {
-        self.should_cache
-    }
-
-    pub fn cache_never(&mut self) -> &mut Self {
+    pub fn do_not_cache(&mut self) -> &mut Self {
         self.should_cache = false;
         self
     }
@@ -111,9 +104,7 @@ impl<'a> BootstrapCommand {
         I: IntoIterator<Item = S>,
         S: AsRef<OsStr>,
     {
-        args.into_iter().for_each(|arg| {
-            self.arg(arg);
-        });
+        self.command.args(args);
         self
     }
 
@@ -207,7 +198,7 @@ impl<'a> BootstrapCommand {
         // command will be handled. Caching must also be avoided here, as the inner command could be
         // modified externally without us being aware.
         self.mark_as_executed();
-        self.cache_never();
+        self.do_not_cache();
         &mut self.command
     }
 
@@ -235,7 +226,19 @@ impl<'a> BootstrapCommand {
     }
 
     pub fn cache_key(&self) -> Option<CommandCacheKey> {
-        (!self.should_cache).then(|| self.into())
+        if !self.should_cache {
+            return None;
+        }
+        let command = &self.command;
+        Some(CommandCacheKey {
+            program: command.get_program().into(),
+            args: command.get_args().map(OsStr::to_os_string).collect(),
+            envs: command
+                .get_envs()
+                .map(|(k, v)| (k.to_os_string(), v.map(|val| val.to_os_string())))
+                .collect(),
+            cwd: command.get_current_dir().map(Path::to_path_buf),
+        })
     }
 }
 
@@ -251,7 +254,7 @@ impl From<Command> for BootstrapCommand {
     fn from(command: Command) -> Self {
         let program = command.get_program().to_owned();
         Self {
-            should_cache: false,
+            should_cache: true,
             command,
             failure_behavior: BehaviorOnFailure::Exit,
             run_always: false,
@@ -260,21 +263,6 @@ impl From<Command> for BootstrapCommand {
     }
 }
 
-impl From<&BootstrapCommand> for CommandCacheKey {
-    fn from(value: &BootstrapCommand) -> Self {
-        let command = &value.command;
-        CommandCacheKey {
-            program: command.get_program().into(),
-            args: command.get_args().map(OsStr::to_os_string).collect(),
-            envs: command
-                .get_envs()
-                .filter_map(|(k, v_opt)| v_opt.map(|v| (k.to_owned(), v.to_owned())))
-                .collect(),
-            cwd: command.get_current_dir().map(Path::to_path_buf),
-        }
-    }
-}
-
 /// Represents the current status of `BootstrapCommand`.
 #[derive(Clone, PartialEq)]
 enum CommandStatus {

src/bootstrap/src/utils/execution_context.rs

@@ -3,7 +3,6 @@
 //! This module provides the [`ExecutionContext`] type, which holds global configuration
 //! relevant during the execution of commands in bootstrap. This includes dry-run
 //! mode, verbosity level, and behavior on failure.
-#![allow(warnings)]
 use std::collections::HashMap;
 use std::panic::Location;
 use std::process::Child;
@@ -46,10 +45,6 @@ pub struct DeferredCommand<'a> {
 }
 
 impl CommandCache {
-    pub fn new() -> Self {
-        Self { cache: Mutex::new(HashMap::new()) }
-    }
-
     pub fn get(&self, key: &CommandCacheKey) -> Option<CommandOutput> {
         self.cache.lock().unwrap().get(key).cloned()
     }
@@ -133,7 +128,7 @@ impl ExecutionContext {
         {
             command.mark_as_executed();
 
-            self.verbose(|| println!("Cache hit: {:?}", command));
+            self.verbose(|| println!("Cache hit: {command:?}"));
 
             return DeferredCommand { state: CommandState::Cached(cached_output) };
         }
@@ -191,29 +186,7 @@ impl ExecutionContext {
         stdout: OutputMode,
         stderr: OutputMode,
     ) -> CommandOutput {
-        let cache_key = command.cache_key();
-
-        if let Some(cached_output) = cache_key.as_ref().and_then(|key| self.command_cache.get(key))
-        {
-            command.mark_as_executed();
-
-            if self.dry_run() && !command.run_always {
-                return CommandOutput::default();
-            }
-
-            self.verbose(|| println!("Cache hit: {:?}", command));
-            return cached_output;
-        }
-
-        let output = self.start(command, stdout, stderr).wait_for_output(self);
-
-        if !self.dry_run() || command.run_always {
-            if let Some(cache_key) = cache_key {
-                self.command_cache.insert(cache_key, output.clone());
-            }
-        }
-
-        output
+        self.start(command, stdout, stderr).wait_for_output(self)
     }
 
     fn fail(&self, message: &str, output: CommandOutput) -> ! {
@@ -247,37 +220,34 @@ impl AsRef<ExecutionContext> for ExecutionContext {
 }
 
 impl<'a> DeferredCommand<'a> {
-    pub fn wait_for_output(mut self, exec_ctx: impl AsRef<ExecutionContext>) -> CommandOutput {
+    pub fn wait_for_output(self, exec_ctx: impl AsRef<ExecutionContext>) -> CommandOutput {
         match self.state {
             CommandState::Cached(output) => output,
             CommandState::Deferred { process, command, stdout, stderr, executed_at, cache_key } => {
                 let exec_ctx = exec_ctx.as_ref();
 
                 let output =
-                    Self::execute_process(process, command, stdout, stderr, executed_at, exec_ctx);
+                    Self::finish_process(process, command, stdout, stderr, executed_at, exec_ctx);
 
                 if (!exec_ctx.dry_run() || command.run_always)
-                    && cache_key.is_some()
-                    && output.status().is_some()
+                    && let (Some(cache_key), Some(_)) = (&cache_key, output.status())
                 {
-                    exec_ctx.command_cache.insert(cache_key.unwrap(), output.clone());
+                    exec_ctx.command_cache.insert(cache_key.clone(), output.clone());
                 }
 
                 output
             }
         }
     }
 
-    pub fn execute_process(
+    pub fn finish_process(
         mut process: Option<Result<Child, std::io::Error>>,
         command: &mut BootstrapCommand,
         stdout: OutputMode,
         stderr: OutputMode,
         executed_at: &'a std::panic::Location<'a>,
         exec_ctx: &ExecutionContext,
     ) -> CommandOutput {
-        let exec_ctx = exec_ctx.as_ref();
-
         let process = match process.take() {
             Some(p) => p,
             None => return CommandOutput::default(),
@@ -301,11 +271,11 @@ impl<'a> DeferredCommand<'a> {
                     writeln!(
                         message,
                         r#"
-Command {:?} did not execute successfully.
+Command {command:?} did not execute successfully.
 Expected success, got {}
 Created at: {created_at}
 Executed at: {executed_at}"#,
-                        command, result.status,
+                        result.status,
                     )
                     .unwrap();
 
@@ -326,9 +296,8 @@ Executed at: {executed_at}"#,
 
                     writeln!(
                         message,
-                        "\n\nCommand {:?} did not execute successfully.\
-                        \nIt was not possible to execute the command: {e:?}",
-                        command
+                        "\n\nCommand {command:?} did not execute successfully.\
+                        \nIt was not possible to execute the command: {e:?}"
                     )
                     .unwrap();
 
@@ -341,9 +310,8 @@ Executed at: {executed_at}"#,
 
                 writeln!(
                     message,
-                    "\n\nCommand {:?} did not execute successfully.\
-                    \nIt was not possible to execute the command: {e:?}",
-                    command
+                    "\n\nCommand {command:?} did not execute successfully.\
+                    \nIt was not possible to execute the command: {e:?}"
                 )
                 .unwrap();