Cure APFS/Fstabs on Mac

src/action/common/place_nix_configuration.rs

@@ -41,7 +41,7 @@ impl PlaceNixConfiguration {
         );
         let create_directory =
             CreateDirectory::plan(NIX_CONF_FOLDER, None, None, 0o0755, force).await?;
-        let create_file = CreateFile::plan(NIX_CONF, None, None, 0o0664, buf, force).await?;
+        let create_file = CreateFile::plan(NIX_CONF, None, None, 0o100664, buf, force).await?;
         Ok(Self {
             create_directory,
             create_file,

src/action/macos/create_fstab_entry.rs

@@ -1,7 +1,8 @@
 use uuid::Uuid;
 
+use super::CreateApfsVolume;
 use crate::{
-    action::{Action, ActionDescription, ActionError, StatefulAction},
+    action::{Action, ActionDescription, ActionError, ActionState, StatefulAction},
     execute_command,
 };
 use serde::Deserialize;
@@ -15,8 +16,16 @@ use tracing::{span, Span};
 
 const FSTAB_PATH: &str = "/etc/fstab";
 
-/** Create an `/etc/fstab` entry for the given volume
+#[derive(Debug, serde::Deserialize, serde::Serialize, Clone, Copy)]
+enum ExistingFstabEntry {
+    /// Need to update the existing `nix-installer` made entry
+    NixInstallerEntry,
+    /// Need to remove old entry and add new entry
+    Foreign,
+    None,
+}
 
+/** Create an `/etc/fstab` entry for the given volume
 
 This action queries `diskutil info` on the volume to fetch it's UUID and
 add the relevant information to `/etc/fstab`.
@@ -26,52 +35,77 @@ add the relevant information to `/etc/fstab`.
 #[derive(Debug, serde::Deserialize, serde::Serialize, Clone)]
 pub struct CreateFstabEntry {
     apfs_volume_label: String,
+    existing_entry: ExistingFstabEntry,
 }
 
 impl CreateFstabEntry {
     #[tracing::instrument(level = "debug", skip_all)]
-    pub async fn plan(apfs_volume_label: String) -> Result<StatefulAction<Self>, ActionError> {
+    pub async fn plan(
+        apfs_volume_label: String,
+        planned_create_apfs_volume: &StatefulAction<CreateApfsVolume>,
+    ) -> Result<StatefulAction<Self>, ActionError> {
         let fstab_path = Path::new(FSTAB_PATH);
+
         if fstab_path.exists() {
             let fstab_buf = tokio::fs::read_to_string(&fstab_path)
                 .await
                 .map_err(|e| ActionError::Read(fstab_path.to_path_buf(), e))?;
             let prelude_comment = fstab_prelude_comment(&apfs_volume_label);
 
-            // See if the user already has a `/nix` related entry, if so, invite them to remove it.
-            if fstab_buf.split(&[' ', '\t']).any(|chunk| chunk == "/nix") {
-                return Err(ActionError::Custom(Box::new(
-                    CreateFstabEntryError::NixEntryExists,
-                )));
-            }
-
             // See if a previous install from this crate exists, if so, invite the user to remove it (we may need to change it)
             if fstab_buf.contains(&prelude_comment) {
-                return Err(ActionError::Custom(Box::new(
-                    CreateFstabEntryError::VolumeEntryExists(apfs_volume_label.clone()),
-                )));
+                if planned_create_apfs_volume.state != ActionState::Completed {
+                    return Ok(StatefulAction::completed(Self {
+                        apfs_volume_label,
+                        existing_entry: ExistingFstabEntry::NixInstallerEntry,
+                    }));
+                }
+
+                return Ok(StatefulAction::uncompleted(Self {
+                    apfs_volume_label,
+                    existing_entry: ExistingFstabEntry::NixInstallerEntry,
+                }));
+            } else if fstab_buf
+                .lines()
+                .any(|line| line.split(&[' ', '\t']).nth(2) == Some("/nix"))
+            {
+                // See if the user already has a `/nix` related entry, if so, invite them to remove it.
+                return Ok(StatefulAction::uncompleted(Self {
+                    apfs_volume_label,
+                    existing_entry: ExistingFstabEntry::Foreign,
+                }));
             }
         }
 
-        Ok(Self { apfs_volume_label }.into())
+        Ok(StatefulAction::uncompleted(Self {
+            apfs_volume_label,
+            existing_entry: ExistingFstabEntry::None,
+        }))
     }
 }
 
 #[async_trait::async_trait]
 #[typetag::serde(name = "create_fstab_entry")]
 impl Action for CreateFstabEntry {
     fn tracing_synopsis(&self) -> String {
-        format!(
-            "Add a UUID based entry for the APFS volume `{}` to `/etc/fstab`",
-            self.apfs_volume_label
-        )
+        match self.existing_entry {
+            ExistingFstabEntry::NixInstallerEntry | ExistingFstabEntry::Foreign => format!(
+                "Update existing entry for the APFS volume `{}` to `/etc/fstab`",
+                self.apfs_volume_label
+            ),
+            ExistingFstabEntry::None => format!(
+                "Add a UUID based entry for the APFS volume `{}` to `/etc/fstab`",
+                self.apfs_volume_label
+            ),
+        }
     }
 
     fn tracing_span(&self) -> Span {
         let span = span!(
             tracing::Level::DEBUG,
             "create_fstab_entry",
             apfs_volume_label = self.apfs_volume_label,
+            existing_entry = ?self.existing_entry,
         );
 
         span
@@ -83,10 +117,12 @@ impl Action for CreateFstabEntry {
 
     #[tracing::instrument(level = "debug", skip_all)]
     async fn execute(&mut self) -> Result<(), ActionError> {
-        let Self { apfs_volume_label } = self;
+        let Self {
+            apfs_volume_label,
+            existing_entry,
+        } = self;
         let fstab_path = Path::new(FSTAB_PATH);
         let uuid = get_uuid_for_label(&apfs_volume_label).await?;
-        let fstab_entry = fstab_entry(&uuid, apfs_volume_label);
 
         let mut fstab = tokio::fs::OpenOptions::new()
             .create(true)
@@ -103,20 +139,79 @@ impl Action for CreateFstabEntry {
             .await
             .map_err(|e| ActionError::Read(fstab_path.to_owned(), e))?;
 
-        if fstab_buf.contains(&fstab_entry) {
-            tracing::debug!("Skipped writing to `/etc/fstab` as the content already existed")
-        } else {
-            fstab
-                .write_all(fstab_entry.as_bytes())
-                .await
-                .map_err(|e| ActionError::Write(fstab_path.to_owned(), e))?;
-        }
+        let updated_buf = match existing_entry {
+            ExistingFstabEntry::NixInstallerEntry => {
+                // Update the entry
+                let mut current_fstab_lines = fstab_buf
+                    .lines()
+                    .map(|v| v.to_owned())
+                    .collect::<Vec<String>>();
+                let mut updated_line = false;
+                let mut saw_prelude = false;
+                let prelude = fstab_prelude_comment(&apfs_volume_label);
+                for line in current_fstab_lines.iter_mut() {
+                    if line == &prelude {
+                        saw_prelude = true;
+                        continue;
+                    }
+                    if saw_prelude && line.split(&[' ', '\t']).nth(2) == Some("/nix") {
+                        *line = fstab_entry(&uuid);
+                        updated_line = true;
+                        break;
+                    }
+                }
+                if !(updated_line && updated_line) {
+                    return Err(ActionError::Custom(Box::new(
+                        CreateFstabEntryError::ExistingNixInstallerEntryDisappeared,
+                    )));
+                }
+                current_fstab_lines.join("\n")
+            },
+            ExistingFstabEntry::Foreign => {
+                // Overwrite the existing entry with our own
+                let mut current_fstab_lines = fstab_buf
+                    .lines()
+                    .map(|v| v.to_owned())
+                    .collect::<Vec<String>>();
+                let mut updated_line = false;
+                for line in current_fstab_lines.iter_mut() {
+                    if line.split(&[' ', '\t']).nth(2) == Some("/nix") {
+                        *line = fstab_lines(&uuid, apfs_volume_label);
+                        updated_line = true;
+                        break;
+                    }
+                }
+                if !updated_line {
+                    return Err(ActionError::Custom(Box::new(
+                        CreateFstabEntryError::ExistingForeignEntryDisappeared,
+                    )));
+                }
+                current_fstab_lines.join("\n")
+            },
+            ExistingFstabEntry::None => fstab_buf + "\n" + &fstab_lines(&uuid, apfs_volume_label),
+        };
+
+        fstab
+            .seek(SeekFrom::Start(0))
+            .await
+            .map_err(|e| ActionError::Seek(fstab_path.to_owned(), e))?;
+        fstab
+            .set_len(0)
+            .await
+            .map_err(|e| ActionError::Truncate(fstab_path.to_owned(), e))?;
+        fstab
+            .write_all(updated_buf.as_bytes())
+            .await
+            .map_err(|e| ActionError::Write(fstab_path.to_owned(), e))?;
 
         Ok(())
     }
 
     fn revert_description(&self) -> Vec<ActionDescription> {
-        let Self { apfs_volume_label } = &self;
+        let Self {
+            apfs_volume_label,
+            existing_entry: _,
+        } = &self;
         vec![ActionDescription::new(
             format!(
                 "Remove the UUID based entry for the APFS volume `{}` in `/etc/fstab`",
@@ -128,10 +223,13 @@ impl Action for CreateFstabEntry {
 
     #[tracing::instrument(level = "debug", skip_all)]
     async fn revert(&mut self) -> Result<(), ActionError> {
-        let Self { apfs_volume_label } = self;
+        let Self {
+            apfs_volume_label,
+            existing_entry: _,
+        } = self;
         let fstab_path = Path::new(FSTAB_PATH);
         let uuid = get_uuid_for_label(&apfs_volume_label).await?;
-        let fstab_entry = fstab_entry(&uuid, apfs_volume_label);
+        let fstab_entry = fstab_lines(&uuid, apfs_volume_label);
 
         let mut file = OpenOptions::new()
             .create(false)
@@ -186,26 +284,26 @@ async fn get_uuid_for_label(apfs_volume_label: &str) -> Result<Uuid, ActionError
     Ok(parsed.volume_uuid)
 }
 
+fn fstab_lines(uuid: &Uuid, apfs_volume_label: &str) -> String {
+    let prelude_comment = fstab_prelude_comment(apfs_volume_label);
+    let fstab_entry = fstab_entry(uuid);
+    prelude_comment + "\n" + &fstab_entry
+}
+
 fn fstab_prelude_comment(apfs_volume_label: &str) -> String {
     format!("# nix-installer created volume labelled `{apfs_volume_label}`")
 }
 
-fn fstab_entry(uuid: &Uuid, apfs_volume_label: &str) -> String {
-    let prelude_comment = fstab_prelude_comment(apfs_volume_label);
-    format!(
-        "\
-        {prelude_comment}\n\
-        UUID={uuid} /nix apfs rw,noauto,nobrowse,suid,owners\n\
-        "
-    )
+fn fstab_entry(uuid: &Uuid) -> String {
+    format!("UUID={uuid} /nix apfs rw,noauto,nobrowse,suid,owners")
 }
 
 #[derive(thiserror::Error, Debug)]
 pub enum CreateFstabEntryError {
-    #[error("An `/etc/fstab` entry for the `/nix` path already exists, consider removing the entry for `/nix`d from `/etc/fstab`")]
-    NixEntryExists,
-    #[error("An `/etc/fstab` entry created by `nix-installer` already exists. If a volume named `{0}` already exists, it may need to be deleted with `diskutil apfs deleteVolume \"{0}\" and the entry for `/nix` should be removed from `/etc/fstab`")]
-    VolumeEntryExists(String),
+    #[error("The `/etc/fstab` entry (previously created by a previous `nix-installer` install) detected during planning disappeared between planning and executing. Cannot update `/etc/fstab` as planned")]
+    ExistingNixInstallerEntryDisappeared,
+    #[error("The `/etc/fstab` entry (previously created by the official install scripts) detected during planning disappeared between planning and executing. Cannot update `/etc/fstab` as planned")]
+    ExistingForeignEntryDisappeared,
 }
 
 #[derive(Deserialize, Clone, Debug)]

src/action/macos/create_nix_volume.rs

@@ -48,7 +48,7 @@ impl CreateNixVolume {
             "/etc/synthetic.conf",
             None,
             None,
-            0o0655,
+            0o100644,
             "nix\n".into(), /* The newline is required otherwise it segfaults */
             create_or_insert_into_file::Position::End,
         )
@@ -61,7 +61,7 @@ impl CreateNixVolume {
 
         let create_volume = CreateApfsVolume::plan(disk, name.clone(), case_sensitive).await?;
 
-        let create_fstab_entry = CreateFstabEntry::plan(name.clone())
+        let create_fstab_entry = CreateFstabEntry::plan(name.clone(), &create_volume)
             .await
             .map_err(|e| ActionError::Child(Box::new(e)))?;
 
@@ -135,7 +135,7 @@ impl CreateNixVolume {
 impl Action for CreateNixVolume {
     fn tracing_synopsis(&self) -> String {
         format!(
-            "Create an APFS volume `{}` for Nix on `{}`",
+            "Create an APFS volume `{}` for Nix on `{}` and add it to `/etc/fstab` mounting on `/nix`",
             self.name,
             self.disk.display()
         )

tests/fixtures/macos/macos.json

@@ -40,7 +40,8 @@
         },
         "create_fstab_entry": {
           "action": {
-            "apfs_volume_label": "Nix Store"
+            "apfs_volume_label": "Nix Store",
+            "existing_entry": "None"
           },
           "state": "Uncompleted"
         },