Curing fixups (#1298)

* /etc/nix.conf nit * link to dnixd * ConfigureNix: don't use try_join! macro We can run them sequentially, no problem. * Allow skipping placing the nix.conf * fixup: use remove_* wrappers, allow ignoring nonexistent files --------- Co-authored-by: Graham Christensen <[email protected]>
DeterminateSystems · Nov 18, 2024 · 873db07 · 873db07
1 parent ef23eb4
commit 873db07
Show file tree

Hide file tree

Showing 20 changed files with 206 additions and 179 deletions.
diff --git a/flake.nix b/flake.nix
@@ -233,6 +233,10 @@
           default = pkgs.nix-installer-static;
         } // nixpkgs.lib.optionalAttrs (pkgs.stdenv.isDarwin) {
           default = pkgs.nix-installer;
+
+          determinate-nixd = pkgs.runCommand "determinate-nixd-link" { } ''
+            ln -s ${optionalPathToDeterminateNixd system} $out
+          '';
         });
 
       hydraJobs = {

diff --git a/src/action/base/create_directory.rs b/src/action/base/create_directory.rs
@@ -4,13 +4,13 @@ use std::path::{Path, PathBuf};
 use nix::unistd::{chown, Group, User};
 
 use target_lexicon::OperatingSystem;
-use tokio::fs::{create_dir_all, remove_dir_all, remove_file};
 use tokio::process::Command;
 use tracing::{span, Span};
 
 use crate::action::{Action, ActionDescription, ActionErrorKind, ActionState};
 use crate::action::{ActionError, StatefulAction};
 use crate::execute_command;
+use crate::util::OnMissing;
 
 /** Create a directory at the given location, optionally with an owning user, group, and mode.
 
@@ -184,7 +184,7 @@ impl Action for CreateDirectory {
             None
         };
 
-        create_dir_all(&path)
+        tokio::fs::create_dir_all(&path)
             .await
             .map_err(|e| ActionErrorKind::CreateDirectory(path.clone(), e))
             .map_err(Self::error)?;
@@ -263,12 +263,12 @@ impl Action for CreateDirectory {
                         .map_err(|e| ActionErrorKind::GettingMetadata(child_path_path.clone(), e))
                         .map_err(Self::error)?;
                     if child_path_type.is_dir() {
-                        remove_dir_all(child_path_path.clone())
+                        crate::util::remove_dir_all(&child_path_path, OnMissing::Error)
                             .await
                             .map_err(|e| ActionErrorKind::Remove(path.clone(), e))
                             .map_err(Self::error)?
                     } else {
-                        remove_file(child_path_path)
+                        crate::util::remove_file(&child_path_path, OnMissing::Error)
                             .await
                             .map_err(|e| ActionErrorKind::Remove(path.clone(), e))
                             .map_err(Self::error)?
@@ -278,10 +278,12 @@ impl Action for CreateDirectory {
             (true, _, false) => {
                 tracing::debug!("Not cleaning mountpoint `{}`", path.display());
             },
-            (false, true, _) | (false, false, true) => remove_dir_all(path.clone())
-                .await
-                .map_err(|e| ActionErrorKind::Remove(path.clone(), e))
-                .map_err(Self::error)?,
+            (false, true, _) | (false, false, true) => {
+                crate::util::remove_dir_all(&path, OnMissing::Error)
+                    .await
+                    .map_err(|e| ActionErrorKind::Remove(path.clone(), e))
+                    .map_err(Self::error)?
+            },
             (false, false, false) => {
                 tracing::debug!("Not removing `{}`, the folder is not empty", path.display());
             },

diff --git a/src/action/base/create_file.rs b/src/action/base/create_file.rs
@@ -6,12 +6,13 @@ use std::{
     path::{Path, PathBuf},
 };
 use tokio::{
-    fs::{remove_file, File, OpenOptions},
+    fs::{File, OpenOptions},
     io::{AsyncReadExt, AsyncWriteExt},
 };
 
-use crate::action::{
-    Action, ActionDescription, ActionError, ActionErrorKind, ActionTag, StatefulAction,
+use crate::{
+    action::{Action, ActionDescription, ActionError, ActionErrorKind, ActionTag, StatefulAction},
+    util::OnMissing,
 };
 
 /** Create a file at the given location with the provided `buf`,
@@ -258,12 +259,8 @@ impl Action for CreateFile {
             buf: _,
             force: _,
         } = self;
-        // The user already deleted it
-        if !path.exists() {
-            return Ok(());
-        }
 
-        remove_file(&path)
+        crate::util::remove_file(&path, OnMissing::Ignore)
             .await
             .map_err(|e| ActionErrorKind::Remove(path.to_owned(), e))
             .map_err(Self::error)?;

diff --git a/src/action/base/move_unpacked_nix.rs b/src/action/base/move_unpacked_nix.rs
@@ -6,8 +6,9 @@ use std::{
 use tracing::{span, Span};
 use walkdir::WalkDir;
 
-use crate::action::{
-    Action, ActionDescription, ActionError, ActionErrorKind, ActionTag, StatefulAction,
+use crate::{
+    action::{Action, ActionDescription, ActionError, ActionErrorKind, ActionTag, StatefulAction},
+    util::OnMissing,
 };
 
 pub(crate) const DEST: &str = "/nix/";
@@ -99,7 +100,7 @@ impl Action for MoveUnpackedNix {
             let entry_dest = dest_store.join(entry.file_name());
             if entry_dest.exists() {
                 tracing::trace!(src = %entry.path().display(), dest = %entry_dest.display(), "Removing already existing package");
-                tokio::fs::remove_dir_all(&entry_dest)
+                crate::util::remove_dir_all(&entry_dest, OnMissing::Ignore)
                     .await
                     .map_err(|e| ActionErrorKind::Remove(entry_dest.clone(), e))
                     .map_err(Self::error)?;

diff --git a/src/action/base/remove_directory.rs b/src/action/base/remove_directory.rs
@@ -1,10 +1,10 @@
 use std::path::{Path, PathBuf};
 
-use tokio::fs::remove_dir_all;
 use tracing::{span, Span};
 
 use crate::action::{Action, ActionDescription, ActionErrorKind, ActionState};
 use crate::action::{ActionError, StatefulAction};
+use crate::util::OnMissing;
 
 /** Remove a directory, does nothing on revert.
 */
@@ -56,7 +56,10 @@ impl Action for RemoveDirectory {
                     self.path.clone(),
                 )));
             }
-            remove_dir_all(&self.path)
+
+            // At this point, we know the path exists, but just in case it was deleted between then
+            // and now, we still ignore the case where it no longer exists.
+            crate::util::remove_dir_all(&self.path, OnMissing::Ignore)
                 .await
                 .map_err(|e| Self::error(ActionErrorKind::Remove(self.path.clone(), e)))?;
         } else {

diff --git a/src/action/common/configure_determinate_nixd_init_service/mod.rs b/src/action/common/configure_determinate_nixd_init_service/mod.rs
@@ -1,5 +1,5 @@
 use std::collections::HashMap;
-use std::path::PathBuf;
+use std::path::{Path, PathBuf};
 
 use serde::{Deserialize, Serialize};
 use tokio::io::AsyncWriteExt;
@@ -9,6 +9,7 @@ use crate::action::common::configure_init_service::{SocketFile, UnitSrc};
 use crate::action::{common::ConfigureInitService, Action, ActionDescription};
 use crate::action::{ActionError, ActionErrorKind, ActionTag, StatefulAction};
 use crate::settings::InitSystem;
+use crate::util::OnMissing;
 
 // Linux
 const LINUX_NIXD_DAEMON_DEST: &str = "/etc/systemd/system/nix-daemon.service";
@@ -44,22 +45,17 @@ impl ConfigureDeterminateNixdInitService {
                 // these service files wouldn't get removed, so we can't rely on them not being
                 // there after phase 1 of the uninstall
                 // [1]: https://github.com/DeterminateSystems/nix-installer/pull/1266
-                if std::path::Path::new(
-                    super::configure_upstream_init_service::DARWIN_NIX_DAEMON_DEST,
+                crate::util::remove_file(
+                    Path::new(super::configure_upstream_init_service::DARWIN_NIX_DAEMON_DEST),
+                    OnMissing::Ignore,
                 )
-                .exists()
-                {
-                    tokio::fs::remove_file(
-                        super::configure_upstream_init_service::DARWIN_NIX_DAEMON_DEST,
-                    )
-                    .await
-                    .map_err(|e| {
-                        Self::error(ActionErrorKind::Remove(
-                            super::configure_upstream_init_service::DARWIN_NIX_DAEMON_DEST.into(),
-                            e,
-                        ))
-                    })?;
-                }
+                .await
+                .map_err(|e| {
+                    Self::error(ActionErrorKind::Remove(
+                        super::configure_upstream_init_service::DARWIN_NIX_DAEMON_DEST.into(),
+                        e,
+                    ))
+                })?;
 
                 Some(DARWIN_NIXD_DAEMON_DEST.into())
             },

diff --git a/src/action/common/configure_init_service.rs b/src/action/common/configure_init_service.rs
@@ -10,6 +10,7 @@ use crate::execute_command;
 
 use crate::action::{Action, ActionDescription};
 use crate::settings::InitSystem;
+use crate::util::OnMissing;
 
 const TMPFILES_SRC: &str = "/nix/var/nix/profiles/default/lib/tmpfiles.d/nix-daemon.conf";
 const TMPFILES_DEST: &str = "/etc/tmpfiles.d/nix-daemon.conf";
@@ -360,13 +361,12 @@ impl Action for ConfigureInitService {
                     )
                     .await
                     .map_err(Self::error)?;
-                    if Path::new(service_dest).exists() {
-                        tracing::trace!(path = %service_dest.display(), "Removing");
-                        tokio::fs::remove_file(service_dest)
-                            .await
-                            .map_err(|e| ActionErrorKind::Remove(service_dest.into(), e))
-                            .map_err(Self::error)?;
-                    }
+
+                    crate::util::remove_file(service_dest, OnMissing::Ignore)
+                        .await
+                        .map_err(|e| ActionErrorKind::Remove(service_dest.into(), e))
+                        .map_err(Self::error)?;
+
                     tracing::trace!(src = %service_src.display(), dest = %service_dest.display(), "Symlinking");
                     tokio::fs::symlink(service_src, service_dest)
                         .await
@@ -384,13 +384,10 @@ impl Action for ConfigureInitService {
                     Self::check_if_systemd_unit_exists(src, dest)
                         .await
                         .map_err(Self::error)?;
-                    if Path::new(dest).exists() {
-                        tracing::trace!(path = %dest.display(), "Removing");
-                        tokio::fs::remove_file(dest)
-                            .await
-                            .map_err(|e| ActionErrorKind::Remove(dest.into(), e))
-                            .map_err(Self::error)?;
-                    }
+                    crate::util::remove_file(dest, OnMissing::Ignore)
+                        .await
+                        .map_err(|e| ActionErrorKind::Remove(dest.into(), e))
+                        .map_err(Self::error)?;
 
                     match src {
                         UnitSrc::Path(src) => {
@@ -610,13 +607,12 @@ impl Action for ConfigureInitService {
                     errors.push(err);
                 }
 
-                if Path::new(TMPFILES_DEST).exists() {
-                    if let Err(err) = tokio::fs::remove_file(TMPFILES_DEST)
+                if let Err(err) =
+                    crate::util::remove_file(Path::new(TMPFILES_DEST), OnMissing::Ignore)
                         .await
                         .map_err(|e| ActionErrorKind::Remove(PathBuf::from(TMPFILES_DEST), e))
-                    {
-                        errors.push(err);
-                    }
+                {
+                    errors.push(err);
                 }
 
                 if let Err(err) = execute_command(
@@ -636,26 +632,20 @@ impl Action for ConfigureInitService {
         };
 
         if let Some(dest) = &self.service_dest {
-            if dest.exists() {
-                tracing::trace!(path = %dest.display(), "Removing");
-                if let Err(err) = tokio::fs::remove_file(dest)
-                    .await
-                    .map_err(|e| ActionErrorKind::Remove(PathBuf::from(dest), e))
-                {
-                    errors.push(err);
-                }
+            if let Err(err) = crate::util::remove_file(dest, OnMissing::Ignore)
+                .await
+                .map_err(|e| ActionErrorKind::Remove(PathBuf::from(dest), e))
+            {
+                errors.push(err);
             }
         }
 
         for socket in self.socket_files.iter() {
-            if socket.dest.exists() {
-                tracing::trace!(path = %socket.dest.display(), "Removing");
-                if let Err(err) = tokio::fs::remove_file(&socket.dest)
-                    .await
-                    .map_err(|e| ActionErrorKind::Remove(socket.dest.to_path_buf(), e))
-                {
-                    errors.push(err);
-                }
+            if let Err(err) = crate::util::remove_file(&socket.dest, OnMissing::Ignore)
+                .await
+                .map_err(|e| ActionErrorKind::Remove(socket.dest.to_path_buf(), e))
+            {
+                errors.push(err);
             }
         }