fix: reinstall should rebuild a source package (#4809)

Author: nichmor

crates/pixi_cli/src/build.rs

@@ -177,6 +177,7 @@ pub async fn execute(args: Args) -> miette::Result<()> {
                 enabled_protocols: Default::default(),
                 work_directory: None,
                 clean: args.clean,
+                force: false,
                 build_profile: BuildProfile::Release,
             })
             .await?;

crates/pixi_command_dispatcher/src/build/build_cache.rs

@@ -209,14 +209,14 @@ impl BuildCache {
 }
 
 /// Cached result of calling `conda/getMetadata` on a build backend.
-#[derive(Debug, Serialize, Deserialize)]
+#[derive(Clone, Debug, Serialize, Deserialize)]
 pub struct CachedBuild {
     #[serde(default, skip_serializing_if = "Option::is_none")]
     pub source: Option<CachedBuildSourceInfo>,
     pub record: RepoDataRecord,
 }
 
-#[derive(Debug, Serialize, Deserialize)]
+#[derive(Clone, Debug, Serialize, Deserialize)]
 pub struct CachedBuildSourceInfo {
     /// The files that were used during the build process. If any of these
     /// change, the build should be considered stale.
@@ -235,14 +235,14 @@ pub struct CachedBuildSourceInfo {
 }
 
 #[serde_as]
-#[derive(Default, Debug, Serialize, Deserialize)]
+#[derive(Clone, Default, Debug, Serialize, Deserialize)]
 pub struct BuildHostEnvironment {
     /// Describes the packages that were installed in the host environment.
     pub packages: Vec<BuildHostPackage>,
 }
 
 #[serde_as]
-#[derive(Debug, Serialize, Deserialize)]
+#[derive(Clone, Debug, Serialize, Deserialize)]
 pub struct BuildHostPackage {
     /// The repodata record of the package.
     #[serde(flatten)]

crates/pixi_command_dispatcher/src/install_pixi/mod.rs

@@ -206,6 +206,10 @@ impl InstallPixiEnvironmentSpec {
         source_record: &SourceRecord,
     ) -> Result<RepoDataRecord, CommandDispatcherError<SourceBuildError>> {
         // Build the source package.
+        // Verify if we need to force the build even if the cache is up to date.
+        let force = self
+            .force_reinstall
+            .contains(&source_record.package_record.name);
         let built_source = command_dispatcher
             .source_build(SourceBuildSpec {
                 source: source_record.source.clone(),
@@ -219,6 +223,8 @@ impl InstallPixiEnvironmentSpec {
                 output_directory: None,
                 work_directory: None,
                 clean: false,
+                // Should we force the build even if the cache is up to date?
+                force,
                 // When we install a pixi environment we always build in development mode.
                 build_profile: BuildProfile::Development,
             })

crates/pixi_command_dispatcher/src/source_build/mod.rs

@@ -90,6 +90,9 @@ pub struct SourceBuildSpec {
     /// The protocols that are enabled for this source
     #[serde(skip_serializing_if = "crate::is_default")]
     pub enabled_protocols: EnabledProtocols,
+
+    /// Force rebuild even if the build cache is up to date.
+    pub force: bool,
 }
 
 #[derive(Debug, Clone)]
@@ -127,58 +130,87 @@ impl SourceBuildSpec {
     ) -> Result<SourceBuildResult, CommandDispatcherError<SourceBuildError>> {
         // If the output directory is not set, we want to use the build cache. Read the
         // build cache in that case.
-        let (output_directory, build_cache) =
-            if let Some(output_directory) = self.output_directory.clone() {
-                (output_directory, None)
-            } else {
-                // Query the source build cache.
-                let build_cache = command_dispatcher
-                    .source_build_cache_status(SourceBuildCacheStatusSpec {
-                        package: self.package.clone(),
-                        build_environment: self.build_environment.clone(),
-                        source: self.source.clone(),
-                        channels: self.channels.clone(),
-                        channel_config: self.channel_config.clone(),
-                        enabled_protocols: self.enabled_protocols.clone(),
-                    })
-                    .await
-                    .map_err_with(SourceBuildError::from)?;
-
-                if let CachedBuildStatus::UpToDate(cached_build) = &build_cache.cached_build {
+        let (output_directory, build_cache) = if let Some(output_directory) =
+            self.output_directory.clone()
+        {
+            (output_directory, None)
+        } else {
+            // Query the source build cache.
+            let build_cache = command_dispatcher
+                .source_build_cache_status(SourceBuildCacheStatusSpec {
+                    package: self.package.clone(),
+                    build_environment: self.build_environment.clone(),
+                    source: self.source.clone(),
+                    channels: self.channels.clone(),
+                    channel_config: self.channel_config.clone(),
+                    enabled_protocols: self.enabled_protocols.clone(),
+                })
+                .await
+                .map_err_with(SourceBuildError::from)?;
+
+            // If the build is up to date and we are not forcing a rebuild, return the cached build.
+            // but if we force a rebuild, and we already have an new cached build, we can reuse the cache entry.
+            if let CachedBuildStatus::UpToDate(cached_build) =
+                &*build_cache.cached_build.lock().await
+            {
+                if !self.force {
                     // If the build is up to date, we can return the cached build.
                     tracing::debug!(
                         source = %self.source,
                         package = ?cached_build.record.package_record.name,
                         build = %cached_build.record.package_record.build,
                         output = %cached_build.record.file_name,
-                        "using cached source build",
+                        "using cached up-to-date source build",
                     );
                     return Ok(SourceBuildResult {
                         output_file: build_cache.cache_dir.join(&cached_build.record.file_name),
                         record: cached_build.record.clone(),
                     });
                 }
+                tracing::debug!(
+                    "source build for {} is up to date, but force rebuild is set, rebuilding anyway",
+                    self.package.name.as_normalized()
+                );
+            }
+            if let CachedBuildStatus::New(cached_build) = &*build_cache.cached_build.lock().await {
+                tracing::debug!(
+                    "source build for {} is already built and marked as new, reusing the cache entry",
+                    self.package.name.as_normalized()
+                );
+                tracing::debug!(
+                    source = %self.source,
+                    package = ?cached_build.record.package_record.name,
+                    build = %cached_build.record.package_record.build,
+                    output = %cached_build.record.file_name,
+                    "using cached new source build",
+                );
+                // dont matter if we forceit , we can reuse the cache entry
+                return Ok(SourceBuildResult {
+                    output_file: build_cache.cache_dir.join(&cached_build.record.file_name),
+                    record: cached_build.record.clone(),
+                });
+            }
 
-                match &build_cache.cached_build {
-                    CachedBuildStatus::Stale(existing) => {
-                        tracing::debug!(
-                            source = %self.source,
-                            package = ?existing.record.package_record.name,
-                            build = %existing.record.package_record.build,
-                            "rebuilding stale source build",
-                        );
-                    }
-                    CachedBuildStatus::Missing => {
-                        tracing::debug!(
-                            source = %self.source,
-                            "no cached source build; starting fresh build",
-                        );
-                    }
-                    CachedBuildStatus::UpToDate(_) => {}
+            match &*build_cache.cached_build.lock().await {
+                CachedBuildStatus::Stale(existing) => {
+                    tracing::debug!(
+                        source = %self.source,
+                        package = ?existing.record.package_record.name,
+                        build = %existing.record.package_record.build,
+                        "rebuilding stale source build",
+                    );
+                }
+                CachedBuildStatus::Missing => {
+                    tracing::debug!(
+                        source = %self.source,
+                        "no cached source build; starting fresh build",
+                    );
                 }
+                CachedBuildStatus::UpToDate(_) | CachedBuildStatus::New(_) => {}
+            }
 
-                (build_cache.cache_dir.clone(), Some(build_cache))
-            };
+            (build_cache.cache_dir.clone(), Some(build_cache))
+        };
 
         // Check out the source code.
         let source_checkout = command_dispatcher
@@ -335,14 +367,21 @@ impl SourceBuildSpec {
         // Update the cache entry if we have one.
         if let Some(build_cache) = build_cache {
             let mut entry = build_cache.entry.lock().await;
+            // set the status that its a new cache
+            // so on the next run we can distinguish between up to date ( was already saved from previous session)
+            // and new that was just build now
+            let cached_build = CachedBuild {
+                source: source_checkout
+                    .pinned
+                    .is_mutable()
+                    .then_some(built_source.metadata.clone()),
+                record: record.clone(),
+            };
+
+            let mut cached_entry = build_cache.cached_build.lock().await;
+            *cached_entry = CachedBuildStatus::New(cached_build.clone());
             entry
-                .insert(CachedBuild {
-                    source: source_checkout
-                        .pinned
-                        .is_mutable()
-                        .then_some(built_source.metadata),
-                    record: record.clone(),
-                })
+                .insert(cached_build)
                 .await
                 .map_err(SourceBuildError::BuildCache)
                 .map_err(CommandDispatcherError::Failed)?;

crates/pixi_command_dispatcher/src/source_build_cache_status/mod.rs

@@ -50,21 +50,25 @@ pub struct SourceBuildCacheStatusSpec {
     pub enabled_protocols: EnabledProtocols,
 }
 
+#[derive(Debug)]
 pub enum CachedBuildStatus {
     /// The build was found in the cache but is stale.
     Stale(CachedBuild),
 
-    /// The build was found in the cache and is up to date.
+    /// The build was found in the cache from previous session and is up to date.
     UpToDate(CachedBuild),
 
+    /// The build was build during the running session.
+    New(CachedBuild),
+
     /// The build was not found in the cache.
     Missing,
 }
 
 pub struct SourceBuildCacheEntry {
     /// The information stored in the build cache. Or `None` if the build did
     /// not exist in the cache.
-    pub cached_build: CachedBuildStatus,
+    pub cached_build: Mutex<CachedBuildStatus>,
 
     /// A reference to the build entry in the cache. Not that as long as this
     /// entry exists a lock is retained on the cache entry.
@@ -100,6 +104,10 @@ impl SourceBuildCacheStatusSpec {
             .map_err(CommandDispatcherError::Failed)?;
 
         // Check the staleness of the cached entry
+        tracing::debug!(
+            "determining cache status for package '{}' from source build cache",
+            self.package,
+        );
         let cached_build = match cached_build {
             Some(cached_build) => {
                 self.determine_cache_status(&command_dispatcher, cached_build)
@@ -108,8 +116,14 @@ impl SourceBuildCacheStatusSpec {
             None => CachedBuildStatus::Missing,
         };
 
+        tracing::debug!(
+            "status of cached build for package '{}' is '{:?}'",
+            self.package,
+            &cached_build
+        );
+
         Ok(SourceBuildCacheEntry {
-            cached_build,
+            cached_build: Mutex::new(cached_build),
             cache_dir: build_cache_entry.cache_dir().to_path_buf(),
             entry: Mutex::new(build_cache_entry),
         })
@@ -134,7 +148,9 @@ impl SourceBuildCacheStatusSpec {
             .check_package_configuration_changed(command_dispatcher, cached_build, source)
             .await?
         {
-            CachedBuildStatus::UpToDate(cached_build) => cached_build,
+            CachedBuildStatus::UpToDate(cached_build) | CachedBuildStatus::New(cached_build) => {
+                cached_build
+            }
             CachedBuildStatus::Stale(cached_build) => {
                 return Ok(CachedBuildStatus::Stale(cached_build));
             }
@@ -146,7 +162,9 @@ impl SourceBuildCacheStatusSpec {
             .check_source_out_of_date(command_dispatcher, cached_build, source)
             .await?
         {
-            CachedBuildStatus::UpToDate(cached_build) => cached_build,
+            CachedBuildStatus::UpToDate(cached_build) | CachedBuildStatus::New(cached_build) => {
+                cached_build
+            }
             CachedBuildStatus::Stale(cached_build) => {
                 return Ok(CachedBuildStatus::Stale(cached_build));
             }
@@ -205,14 +223,15 @@ impl SourceBuildCacheStatusSpec {
                     return Err(CommandDispatcherError::Failed(err));
                 }
                 Ok(entry) => {
-                    match &entry.cached_build {
+                    match &*entry.cached_build.lock().await {
                         CachedBuildStatus::Missing | CachedBuildStatus::Stale(_) => {
                             tracing::debug!(
                                 "package is stale because its build dependency '{identifier}' is missing or stale",
                             );
                             return Ok(CachedBuildStatus::Stale(cached_build));
                         }
-                        CachedBuildStatus::UpToDate(dependency_cached_build) => {
+                        CachedBuildStatus::UpToDate(dependency_cached_build)
+                        | CachedBuildStatus::New(dependency_cached_build) => {
                             // Is this version of the package also what we expect?
                             //
                             // Maybe the package that we previously used was actually updated