From c3bc07f2e68cad7d271c5639a54f89ffc47ba007 Mon Sep 17 00:00:00 2001 From: Ana Hobden Date: Mon, 13 Feb 2023 10:10:09 -0800 Subject: [PATCH 01/16] wip --- src/action/macos/create_fstab_entry.rs | 41 +++++++++++++++++++------- src/action/macos/create_nix_volume.rs | 2 +- 2 files changed, 31 insertions(+), 12 deletions(-) diff --git a/src/action/macos/create_fstab_entry.rs b/src/action/macos/create_fstab_entry.rs index 106e87a2..e573d910 100644 --- a/src/action/macos/create_fstab_entry.rs +++ b/src/action/macos/create_fstab_entry.rs @@ -1,5 +1,6 @@ use uuid::Uuid; +use super::CreateApfsVolume; use crate::{ action::{Action, ActionDescription, ActionError, StatefulAction}, execute_command, @@ -15,8 +16,14 @@ 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 { + NixInstallerCreated, + 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,12 +33,17 @@ 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, ActionError> { + pub async fn plan( + apfs_volume_label: String, + planned_create_apfs_volume: &StatefulAction, + ) -> Result, ActionError> { let fstab_path = Path::new(FSTAB_PATH); + let mut existing_entry = ExistingFstabEntry::None; if fstab_path.exists() { let fstab_buf = tokio::fs::read_to_string(&fstab_path) .await @@ -40,20 +52,20 @@ impl CreateFstabEntry { // 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, - ))); + existing_entry = ExistingFstabEntry::Foreign; } // 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()), - ))); + existing_entry = ExistingFstabEntry::NixInstallerCreated; } } - Ok(Self { apfs_volume_label }.into()) + Ok(Self { + apfs_volume_label, + existing_entry, + } + .into()) } } @@ -72,6 +84,7 @@ impl Action for CreateFstabEntry { tracing::Level::DEBUG, "create_fstab_entry", apfs_volume_label = self.apfs_volume_label, + existing_entry = ?self.existing_entry, ); span @@ -83,7 +96,10 @@ 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); @@ -116,7 +132,10 @@ impl Action for CreateFstabEntry { } fn revert_description(&self) -> Vec { - 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`", diff --git a/src/action/macos/create_nix_volume.rs b/src/action/macos/create_nix_volume.rs index 08371f2a..0421b66a 100644 --- a/src/action/macos/create_nix_volume.rs +++ b/src/action/macos/create_nix_volume.rs @@ -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)))?; From c4ec82a6e1bcf00caab856e150baf8d469480d2c Mon Sep 17 00:00:00 2001 From: Ana Hobden Date: Mon, 13 Feb 2023 11:28:00 -0800 Subject: [PATCH 02/16] Do main editing portion --- src/action/macos/create_fstab_entry.rs | 122 +++++++++++++++++++------ 1 file changed, 92 insertions(+), 30 deletions(-) diff --git a/src/action/macos/create_fstab_entry.rs b/src/action/macos/create_fstab_entry.rs index e573d910..b3eab48d 100644 --- a/src/action/macos/create_fstab_entry.rs +++ b/src/action/macos/create_fstab_entry.rs @@ -2,7 +2,7 @@ use uuid::Uuid; use super::CreateApfsVolume; use crate::{ - action::{Action, ActionDescription, ActionError, StatefulAction}, + action::{Action, ActionDescription, ActionError, ActionState, StatefulAction}, execute_command, }; use serde::Deserialize; @@ -18,7 +18,9 @@ const FSTAB_PATH: &str = "/etc/fstab"; #[derive(Debug, serde::Deserialize, serde::Serialize, Clone, Copy)] enum ExistingFstabEntry { - NixInstallerCreated, + /// Need to update the existing `nix-installer` made entry + NixInstallerEntry, + /// Need to remove old entry and add new entry Foreign, None, } @@ -43,29 +45,42 @@ impl CreateFstabEntry { planned_create_apfs_volume: &StatefulAction, ) -> Result, ActionError> { let fstab_path = Path::new(FSTAB_PATH); - let mut existing_entry = ExistingFstabEntry::None; + 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") { - existing_entry = ExistingFstabEntry::Foreign; - } - // 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) { - existing_entry = ExistingFstabEntry::NixInstallerCreated; + 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 { + Ok(StatefulAction::uncompleted(Self { apfs_volume_label, - existing_entry, - } - .into()) + existing_entry: ExistingFstabEntry::None, + })) } } @@ -102,7 +117,7 @@ impl Action for CreateFstabEntry { } = 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 pending_fstab_lines = fstab_lines(&uuid, apfs_volume_label); let mut fstab = tokio::fs::OpenOptions::new() .create(true) @@ -119,14 +134,58 @@ 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::>(); + 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, apfs_volume_label); + updated_line = true; + break; + } + } + if !(updated_line && updated_line) { + return Err(todo!()); + } + 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::>(); + 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(todo!()); + } + current_fstab_lines.join("\n") + }, + ExistingFstabEntry::None => fstab_buf + "\n" + &fstab_lines(&uuid, apfs_volume_label), + }; + + fstab + .write_all(updated_buf.as_bytes()) + .await + .map_err(|e| ActionError::Write(fstab_path.to_owned(), e))?; Ok(()) } @@ -147,7 +206,10 @@ 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); @@ -205,18 +267,18 @@ async fn get_uuid_for_label(apfs_volume_label: &str) -> Result String { + let prelude_comment = fstab_prelude_comment(apfs_volume_label); + let fstab_entry = fstab_entry(uuid, apfs_volume_label); + prelude_comment + &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\ - " - ) + format!("UUID={uuid} /nix apfs rw,noauto,nobrowse,suid,owners") } #[derive(thiserror::Error, Debug)] From a51ac21537811c3f1a676b72c4afb97b0824ef3c Mon Sep 17 00:00:00 2001 From: Ana Hobden Date: Mon, 13 Feb 2023 12:48:35 -0800 Subject: [PATCH 03/16] Some more curing on fstab entries --- src/action/common/place_nix_configuration.rs | 2 +- src/action/macos/create_fstab_entry.rs | 41 ++++++++++++-------- src/action/macos/create_nix_volume.rs | 4 +- tests/fixtures/macos/macos.json | 3 +- 4 files changed, 30 insertions(+), 20 deletions(-) diff --git a/src/action/common/place_nix_configuration.rs b/src/action/common/place_nix_configuration.rs index aeb2bd47..27e860ee 100644 --- a/src/action/common/place_nix_configuration.rs +++ b/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, diff --git a/src/action/macos/create_fstab_entry.rs b/src/action/macos/create_fstab_entry.rs index b3eab48d..a8dbf294 100644 --- a/src/action/macos/create_fstab_entry.rs +++ b/src/action/macos/create_fstab_entry.rs @@ -88,10 +88,16 @@ impl CreateFstabEntry { #[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 { @@ -117,7 +123,6 @@ impl Action for CreateFstabEntry { } = self; let fstab_path = Path::new(FSTAB_PATH); let uuid = get_uuid_for_label(&apfs_volume_label).await?; - let pending_fstab_lines = fstab_lines(&uuid, apfs_volume_label); let mut fstab = tokio::fs::OpenOptions::new() .create(true) @@ -150,13 +155,15 @@ impl Action for CreateFstabEntry { continue; } if saw_prelude && line.split(&[' ', '\t']).nth(2) == Some("/nix") { - *line = fstab_entry(&uuid, apfs_volume_label); + *line = fstab_entry(&uuid); updated_line = true; break; } } if !(updated_line && updated_line) { - return Err(todo!()); + return Err(ActionError::Custom(Box::new( + CreateFstabEntryError::ExistingNixInstallerEntryDisappeared, + ))); } current_fstab_lines.join("\n") }, @@ -175,7 +182,9 @@ impl Action for CreateFstabEntry { } } if !updated_line { - return Err(todo!()); + return Err(ActionError::Custom(Box::new( + CreateFstabEntryError::ExistingForeignEntryDisappeared, + ))); } current_fstab_lines.join("\n") }, @@ -193,7 +202,7 @@ impl Action for CreateFstabEntry { fn revert_description(&self) -> Vec { let Self { apfs_volume_label, - existing_entry, + existing_entry: _, } = &self; vec![ActionDescription::new( format!( @@ -212,7 +221,7 @@ impl Action for CreateFstabEntry { } = 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) @@ -269,7 +278,7 @@ async fn get_uuid_for_label(apfs_volume_label: &str) -> Result String { let prelude_comment = fstab_prelude_comment(apfs_volume_label); - let fstab_entry = fstab_entry(uuid, apfs_volume_label); + let fstab_entry = fstab_entry(uuid); prelude_comment + &fstab_entry } @@ -277,16 +286,16 @@ 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 { +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)] diff --git a/src/action/macos/create_nix_volume.rs b/src/action/macos/create_nix_volume.rs index 0421b66a..86d8de8e 100644 --- a/src/action/macos/create_nix_volume.rs +++ b/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, ) @@ -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() ) diff --git a/tests/fixtures/macos/macos.json b/tests/fixtures/macos/macos.json index 595af133..53374a78 100644 --- a/tests/fixtures/macos/macos.json +++ b/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" }, From 354c0aa5ef4a71fd8cdebd736a2db78d02a2b2c6 Mon Sep 17 00:00:00 2001 From: Ana Hobden Date: Mon, 13 Feb 2023 12:52:07 -0800 Subject: [PATCH 04/16] Overwrite fstab instead of append --- src/action/macos/create_fstab_entry.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/action/macos/create_fstab_entry.rs b/src/action/macos/create_fstab_entry.rs index a8dbf294..47e5335f 100644 --- a/src/action/macos/create_fstab_entry.rs +++ b/src/action/macos/create_fstab_entry.rs @@ -191,6 +191,14 @@ impl Action for CreateFstabEntry { 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 From cda207f39240e3459e8a618890a47eade857aeb1 Mon Sep 17 00:00:00 2001 From: Ana Hobden Date: Mon, 13 Feb 2023 12:52:44 -0800 Subject: [PATCH 05/16] Add newline --- src/action/macos/create_fstab_entry.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/action/macos/create_fstab_entry.rs b/src/action/macos/create_fstab_entry.rs index 47e5335f..6acd9f44 100644 --- a/src/action/macos/create_fstab_entry.rs +++ b/src/action/macos/create_fstab_entry.rs @@ -287,7 +287,7 @@ async fn get_uuid_for_label(apfs_volume_label: &str) -> Result String { let prelude_comment = fstab_prelude_comment(apfs_volume_label); let fstab_entry = fstab_entry(uuid); - prelude_comment + &fstab_entry + prelude_comment + "\n" + &fstab_entry } fn fstab_prelude_comment(apfs_volume_label: &str) -> String { From 7b7eda1d149024c3d241b81db39ddb3397c76c72 Mon Sep 17 00:00:00 2001 From: Ana Hobden Date: Tue, 14 Feb 2023 09:43:41 -0800 Subject: [PATCH 06/16] Improve --explain output for CreateNixVolume --- src/action/macos/create_nix_volume.rs | 28 ++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/src/action/macos/create_nix_volume.rs b/src/action/macos/create_nix_volume.rs index 86d8de8e..4aca14fc 100644 --- a/src/action/macos/create_nix_volume.rs +++ b/src/action/macos/create_nix_volume.rs @@ -135,9 +135,10 @@ impl CreateNixVolume { impl Action for CreateNixVolume { fn tracing_synopsis(&self) -> String { format!( - "Create an APFS volume `{}` for Nix on `{}` and add it to `/etc/fstab` mounting on `/nix`", - self.name, - self.disk.display() + "Create an{maybe_encrypted} APFS volume `{name}` for Nix on `{disk}` and add it to `/etc/fstab` mounting on `/nix`", + maybe_encrypted = if self.encrypt { " encrypted" } else { "" }, + name = self.name, + disk = self.disk.display(), ) } @@ -151,10 +152,23 @@ impl Action for CreateNixVolume { } fn execute_description(&self) -> Vec { - let Self { - disk: _, name: _, .. - } = &self; - vec![ActionDescription::new(self.tracing_synopsis(), vec![])] + let mut explanation = vec![ + self.create_or_append_synthetic_conf.tracing_synopsis(), + self.create_synthetic_objects.tracing_synopsis(), + self.unmount_volume.tracing_synopsis(), + self.create_volume.tracing_synopsis(), + self.create_fstab_entry.tracing_synopsis(), + ]; + if let Some(encrypt_volume) = &self.encrypt_volume { + explanation.push(encrypt_volume.tracing_synopsis()); + } + explanation.append(&mut vec![ + self.setup_volume_daemon.tracing_synopsis(), + self.bootstrap_volume.tracing_synopsis(), + self.enable_ownership.tracing_synopsis(), + ]); + + vec![ActionDescription::new(self.tracing_synopsis(), explanation)] } #[tracing::instrument(level = "debug", skip_all)] From 4a604f141d990f299ce5827e375cd605127c1ed3 Mon Sep 17 00:00:00 2001 From: Ana Hobden Date: Tue, 14 Feb 2023 10:12:19 -0800 Subject: [PATCH 07/16] Tweak some permissions --- src/action/common/place_channel_configuration.rs | 2 +- src/action/macos/create_apfs_volume.rs | 2 +- src/action/macos/create_nix_volume.rs | 1 + 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/action/common/place_channel_configuration.rs b/src/action/common/place_channel_configuration.rs index 5805f1c0..c575cbc0 100644 --- a/src/action/common/place_channel_configuration.rs +++ b/src/action/common/place_channel_configuration.rs @@ -32,7 +32,7 @@ impl PlaceChannelConfiguration { .join(".nix-channels"), None, None, - 0o0664, + 0o100644, buf, force, ) diff --git a/src/action/macos/create_apfs_volume.rs b/src/action/macos/create_apfs_volume.rs index 738d1930..61c21b01 100644 --- a/src/action/macos/create_apfs_volume.rs +++ b/src/action/macos/create_apfs_volume.rs @@ -138,7 +138,7 @@ impl Action for CreateApfsVolume { #[derive(Debug, thiserror::Error)] pub enum CreateApfsVolumeError { - #[error("Existing volume called `{0}` found in `diskutil apfs list`, delete it with `diskutil apfs deleteVolume \"{0}\"`")] + #[error("Existing volume called `{0}` found in `diskutil apfs list`, delete it with `diskutil apfs deleteVolume \"{0}\"` (if you recieve error -69888, you may need to run `launchctl bootout system/org.nixos.darwin-store` and `launchctl bootout system/org.nixos.nix-daemon` first)")] ExistingVolume(String), } diff --git a/src/action/macos/create_nix_volume.rs b/src/action/macos/create_nix_volume.rs index 4aca14fc..063936b3 100644 --- a/src/action/macos/create_nix_volume.rs +++ b/src/action/macos/create_nix_volume.rs @@ -226,6 +226,7 @@ impl Action for CreateNixVolume { fn revert_description(&self) -> Vec { let Self { disk, name, .. } = &self; + // TODO(@hoverbear): Do a better description here. vec![ActionDescription::new( format!("Remove the APFS volume `{name}` on `{}`", disk.display()), vec![format!( From e2239258df56b7396cf9b1327f5075327b774d00 Mon Sep 17 00:00:00 2001 From: Ana Hobden Date: Tue, 14 Feb 2023 10:29:27 -0800 Subject: [PATCH 08/16] Fixup a few more permissions spots --- src/action/common/place_nix_configuration.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/action/common/place_nix_configuration.rs b/src/action/common/place_nix_configuration.rs index 27e860ee..e649d70e 100644 --- a/src/action/common/place_nix_configuration.rs +++ b/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, 0o100664, buf, force).await?; + let create_file = CreateFile::plan(NIX_CONF, None, None, 0o100644, buf, force).await?; Ok(Self { create_directory, create_file, From e691eb5aa919ac112db856d8f0457ce20f7c209d Mon Sep 17 00:00:00 2001 From: Ana Hobden Date: Wed, 15 Feb 2023 08:48:18 -0800 Subject: [PATCH 09/16] Improve encrypted volume handling --- src/action/macos/create_nix_volume.rs | 2 +- src/action/macos/encrypt_apfs_volume.rs | 64 ++++++++++++++++++++++--- 2 files changed, 58 insertions(+), 8 deletions(-) diff --git a/src/action/macos/create_nix_volume.rs b/src/action/macos/create_nix_volume.rs index 063936b3..1f896e04 100644 --- a/src/action/macos/create_nix_volume.rs +++ b/src/action/macos/create_nix_volume.rs @@ -66,7 +66,7 @@ impl CreateNixVolume { .map_err(|e| ActionError::Child(Box::new(e)))?; let encrypt_volume = if encrypt { - Some(EncryptApfsVolume::plan(disk, &name).await?) + Some(EncryptApfsVolume::plan(disk, &name, &create_volume).await?) } else { None }; diff --git a/src/action/macos/encrypt_apfs_volume.rs b/src/action/macos/encrypt_apfs_volume.rs index 4b809920..67881fa8 100644 --- a/src/action/macos/encrypt_apfs_volume.rs +++ b/src/action/macos/encrypt_apfs_volume.rs @@ -1,14 +1,20 @@ use crate::{ action::{ - macos::NIX_VOLUME_MOUNTD_DEST, Action, ActionDescription, ActionError, StatefulAction, + macos::NIX_VOLUME_MOUNTD_DEST, Action, ActionDescription, ActionError, ActionState, + StatefulAction, }, execute_command, }; use rand::Rng; -use std::path::{Path, PathBuf}; +use std::{ + path::{Path, PathBuf}, + process::Stdio, +}; use tokio::process::Command; use tracing::{span, Span}; +use super::CreateApfsVolume; + /** Encrypt an APFS volume */ @@ -23,13 +29,49 @@ impl EncryptApfsVolume { pub async fn plan( disk: impl AsRef, name: impl AsRef, + planned_create_apfs_volume: &StatefulAction, ) -> Result, ActionError> { let name = name.as_ref().to_owned(); - Ok(Self { - name, - disk: disk.as_ref().to_path_buf(), + let disk = disk.as_ref().to_path_buf(); + + if Command::new("/usr/bin/security") + .args(["find-generic-password", "-a"]) + .arg(&name) + .arg("-s") + .arg("Nix Store") + .arg("-l") + .arg(&format!("{} encryption password", disk.display())) + .arg("-D") + .arg("Encrypted volume password") + .process_group(0) + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .status() + .await + .map_err(ActionError::Command)? + .success() + { + // The user has a password matching what we would create. + if planned_create_apfs_volume.state == ActionState::Completed { + // We detected a created volume already, and a password exists, so we can keep using that and skip doing anything + return Ok(StatefulAction::completed(Self { name, disk })); + } + + // Aske the user to remove it + return Err(ActionError::Custom(Box::new( + EncryptApfsVolumeError::ExistingPasswordFound(name, disk), + ))); + } else { + if planned_create_apfs_volume.state == ActionState::Completed { + // The user has a volume already created, but a password not set. This means we probably can't decrypt the volume. + return Err(ActionError::Custom(Box::new( + EncryptApfsVolumeError::MissingPasswordForExistingVolume(name, disk), + ))); + } } - .into()) + + Ok(StatefulAction::uncompleted(Self { name, disk })) } } @@ -91,7 +133,7 @@ impl Action for EncryptApfsVolume { "-a", name.as_str(), "-s", - name.as_str(), + "Nix Store", "-l", format!("{} encryption password", disk_str).as_str(), "-D", @@ -184,3 +226,11 @@ impl Action for EncryptApfsVolume { Ok(()) } } + +#[derive(thiserror::Error, Debug)] +pub enum EncryptApfsVolumeError { + #[error("The keychain has an existing password for a non-existing \"{0}\" volume on disk `{1}`, consider removing the password with `security delete-generic-password -a \"{0}\" -s \"Nix Store\" -l \"{1} encryption password\" -D \"Encrypted volume password\"`")] + ExistingPasswordFound(String, PathBuf), + #[error("The keychain lacks a password for the already existing \"{0}\" volume on disk `{1}`, consider removing the volume with `diskutil apfs deleteVolume \"{0}\"` (if you recieve error -69888, you may need to run `launchctl bootout system/org.nixos.darwin-store` and `launchctl bootout system/org.nixos.nix-daemon` first)")] + MissingPasswordForExistingVolume(String, PathBuf), +} From 10d50caa3c39b9c024e3d74f3a56e641f35225a2 Mon Sep 17 00:00:00 2001 From: Ana Hobden Date: Wed, 15 Feb 2023 09:50:33 -0800 Subject: [PATCH 10/16] Handle APFS volumes existing already to some degree --- src/action/common/configure_init_service.rs | 2 +- src/action/macos/create_apfs_volume.rs | 40 +++++---------------- src/action/macos/create_fstab_entry.rs | 6 ++-- src/action/macos/encrypt_apfs_volume.rs | 27 ++++++++++++++ src/action/macos/mod.rs | 2 +- src/os/darwin.rs | 19 ++++++++++ 6 files changed, 60 insertions(+), 36 deletions(-) diff --git a/src/action/common/configure_init_service.rs b/src/action/common/configure_init_service.rs index ddd6fcec..c831b60f 100644 --- a/src/action/common/configure_init_service.rs +++ b/src/action/common/configure_init_service.rs @@ -225,7 +225,7 @@ impl Action for ConfigureInitService { InitSystem::Launchd => { vec![ActionDescription::new( "Unconfigure Nix daemon related settings with launchctl".to_string(), - vec!["Run `launchctl unload {DARWIN_NIX_DAEMON_DEST}`".to_string()], + vec![format!("Run `launchctl unload {DARWIN_NIX_DAEMON_DEST}`")], )] }, #[cfg(not(target_os = "macos"))] diff --git a/src/action/macos/create_apfs_volume.rs b/src/action/macos/create_apfs_volume.rs index 61c21b01..b0bc95fd 100644 --- a/src/action/macos/create_apfs_volume.rs +++ b/src/action/macos/create_apfs_volume.rs @@ -5,9 +5,9 @@ use tracing::{span, Span}; use crate::action::{ActionError, StatefulAction}; use crate::execute_command; -use serde::Deserialize; use crate::action::{Action, ActionDescription}; +use crate::os::darwin::DiskUtilApfsListOutput; #[derive(Debug, serde::Deserialize, serde::Serialize, Clone)] pub struct CreateApfsVolume { @@ -23,6 +23,7 @@ impl CreateApfsVolume { name: String, case_sensitive: bool, ) -> Result, ActionError> { + // Mac lacks an `diskutil apfs info` command or analog, so we have to list let output = execute_command(Command::new("/usr/sbin/diskutil").args(["apfs", "list", "-plist"])) .await @@ -32,19 +33,20 @@ impl CreateApfsVolume { for container in parsed.containers { for volume in container.volumes { if volume.name == name { - return Err(ActionError::Custom(Box::new( - CreateApfsVolumeError::ExistingVolume(name), - ))); + return Ok(StatefulAction::completed(Self { + disk: disk.as_ref().to_path_buf(), + name, + case_sensitive, + })); } } } - Ok(Self { + Ok(StatefulAction::uncompleted(Self { disk: disk.as_ref().to_path_buf(), name, case_sensitive, - } - .into()) + })) } } @@ -135,27 +137,3 @@ impl Action for CreateApfsVolume { Ok(()) } } - -#[derive(Debug, thiserror::Error)] -pub enum CreateApfsVolumeError { - #[error("Existing volume called `{0}` found in `diskutil apfs list`, delete it with `diskutil apfs deleteVolume \"{0}\"` (if you recieve error -69888, you may need to run `launchctl bootout system/org.nixos.darwin-store` and `launchctl bootout system/org.nixos.nix-daemon` first)")] - ExistingVolume(String), -} - -#[derive(Deserialize, Clone, Debug)] -#[serde(rename_all = "PascalCase")] -struct DiskUtilApfsListOutput { - containers: Vec, -} - -#[derive(Deserialize, Clone, Debug)] -#[serde(rename_all = "PascalCase")] -struct DiskUtilApfsContainer { - volumes: Vec, -} - -#[derive(Deserialize, Clone, Debug)] -#[serde(rename_all = "PascalCase")] -struct DiskUtilApfsListVolume { - name: String, -} diff --git a/src/action/macos/create_fstab_entry.rs b/src/action/macos/create_fstab_entry.rs index 6acd9f44..0b94b369 100644 --- a/src/action/macos/create_fstab_entry.rs +++ b/src/action/macos/create_fstab_entry.rs @@ -154,13 +154,13 @@ impl Action for CreateFstabEntry { saw_prelude = true; continue; } - if saw_prelude && line.split(&[' ', '\t']).nth(2) == Some("/nix") { + if saw_prelude && line.split(&[' ', '\t']).nth(1) == Some("/nix") { *line = fstab_entry(&uuid); updated_line = true; break; } } - if !(updated_line && updated_line) { + if !(saw_prelude && updated_line) { return Err(ActionError::Custom(Box::new( CreateFstabEntryError::ExistingNixInstallerEntryDisappeared, ))); @@ -300,7 +300,7 @@ fn fstab_entry(uuid: &Uuid) -> String { #[derive(thiserror::Error, Debug)] pub enum CreateFstabEntryError { - #[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")] + #[error("The `/etc/fstab` entry (previously created by a `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, diff --git a/src/action/macos/encrypt_apfs_volume.rs b/src/action/macos/encrypt_apfs_volume.rs index 67881fa8..b435ab6a 100644 --- a/src/action/macos/encrypt_apfs_volume.rs +++ b/src/action/macos/encrypt_apfs_volume.rs @@ -4,6 +4,7 @@ use crate::{ StatefulAction, }, execute_command, + os::darwin::DiskUtilApfsListOutput, }; use rand::Rng; use std::{ @@ -71,6 +72,30 @@ impl EncryptApfsVolume { } } + // Ensure if the disk already exists, that it's encrypted + let output = + execute_command(Command::new("/usr/sbin/diskutil").args(["apfs", "list", "-plist"])) + .await + .map_err(ActionError::Command)?; + + let parsed: DiskUtilApfsListOutput = plist::from_bytes(&output.stdout)?; + for container in parsed.containers { + for volume in container.volumes { + if volume.name == name { + match volume.encryption == false { + true => { + return Ok(StatefulAction::completed(Self { disk, name })); + }, + false => { + return Err(ActionError::Custom(Box::new( + EncryptApfsVolumeError::ExistingVolumeNotEncrypted(name, disk), + ))); + }, + } + } + } + } + Ok(StatefulAction::uncompleted(Self { name, disk })) } } @@ -233,4 +258,6 @@ pub enum EncryptApfsVolumeError { ExistingPasswordFound(String, PathBuf), #[error("The keychain lacks a password for the already existing \"{0}\" volume on disk `{1}`, consider removing the volume with `diskutil apfs deleteVolume \"{0}\"` (if you recieve error -69888, you may need to run `launchctl bootout system/org.nixos.darwin-store` and `launchctl bootout system/org.nixos.nix-daemon` first)")] MissingPasswordForExistingVolume(String, PathBuf), + #[error("The existing APFS volume \"{0}\" on disk `{1}` is not encrypted but it should be, consider removing the volume with `diskutil apfs deleteVolume \"{0}\"` (if you recieve error -69888, you may need to run `launchctl bootout system/org.nixos.darwin-store` and `launchctl bootout system/org.nixos.nix-daemon` first)")] + ExistingVolumeNotEncrypted(String, PathBuf), } diff --git a/src/action/macos/mod.rs b/src/action/macos/mod.rs index e0335f53..638c104a 100644 --- a/src/action/macos/mod.rs +++ b/src/action/macos/mod.rs @@ -11,7 +11,7 @@ pub(crate) mod encrypt_apfs_volume; pub(crate) mod unmount_apfs_volume; pub use bootstrap_apfs_volume::{BootstrapApfsVolume, BootstrapVolumeError}; -pub use create_apfs_volume::{CreateApfsVolume, CreateApfsVolumeError}; +pub use create_apfs_volume::CreateApfsVolume; pub use create_nix_volume::{CreateNixVolume, NIX_VOLUME_MOUNTD_DEST}; pub use create_synthetic_objects::{CreateSyntheticObjects, CreateSyntheticObjectsError}; pub use enable_ownership::{EnableOwnership, EnableOwnershipError}; diff --git a/src/os/darwin.rs b/src/os/darwin.rs index 0ccda9e1..c5589904 100644 --- a/src/os/darwin.rs +++ b/src/os/darwin.rs @@ -4,3 +4,22 @@ pub struct DiskUtilOutput { pub parent_whole_disk: String, pub global_permissions_enabled: bool, } + +#[derive(serde::Deserialize, Clone, Debug)] +#[serde(rename_all = "PascalCase")] +pub struct DiskUtilApfsListOutput { + pub containers: Vec, +} + +#[derive(serde::Deserialize, Clone, Debug)] +#[serde(rename_all = "PascalCase")] +pub struct DiskUtilApfsContainer { + pub volumes: Vec, +} + +#[derive(serde::Deserialize, Clone, Debug)] +#[serde(rename_all = "PascalCase")] +pub struct DiskUtilApfsListVolume { + pub name: String, + pub encryption: bool, +} From 00f4405f7ddf99adab35184c2611b4b78d1390c9 Mon Sep 17 00:00:00 2001 From: Ana Hobden Date: Wed, 15 Feb 2023 11:17:08 -0800 Subject: [PATCH 11/16] Correct speeling --- src/action/macos/encrypt_apfs_volume.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/action/macos/encrypt_apfs_volume.rs b/src/action/macos/encrypt_apfs_volume.rs index b435ab6a..87cd077e 100644 --- a/src/action/macos/encrypt_apfs_volume.rs +++ b/src/action/macos/encrypt_apfs_volume.rs @@ -256,8 +256,8 @@ impl Action for EncryptApfsVolume { pub enum EncryptApfsVolumeError { #[error("The keychain has an existing password for a non-existing \"{0}\" volume on disk `{1}`, consider removing the password with `security delete-generic-password -a \"{0}\" -s \"Nix Store\" -l \"{1} encryption password\" -D \"Encrypted volume password\"`")] ExistingPasswordFound(String, PathBuf), - #[error("The keychain lacks a password for the already existing \"{0}\" volume on disk `{1}`, consider removing the volume with `diskutil apfs deleteVolume \"{0}\"` (if you recieve error -69888, you may need to run `launchctl bootout system/org.nixos.darwin-store` and `launchctl bootout system/org.nixos.nix-daemon` first)")] + #[error("The keychain lacks a password for the already existing \"{0}\" volume on disk `{1}`, consider removing the volume with `diskutil apfs deleteVolume \"{0}\"` (if you receive error -69888, you may need to run `launchctl bootout system/org.nixos.darwin-store` and `launchctl bootout system/org.nixos.nix-daemon` first)")] MissingPasswordForExistingVolume(String, PathBuf), - #[error("The existing APFS volume \"{0}\" on disk `{1}` is not encrypted but it should be, consider removing the volume with `diskutil apfs deleteVolume \"{0}\"` (if you recieve error -69888, you may need to run `launchctl bootout system/org.nixos.darwin-store` and `launchctl bootout system/org.nixos.nix-daemon` first)")] + #[error("The existing APFS volume \"{0}\" on disk `{1}` is not encrypted but it should be, consider removing the volume with `diskutil apfs deleteVolume \"{0}\"` (if you receive error -69888, you may need to run `launchctl bootout system/org.nixos.darwin-store` and `launchctl bootout system/org.nixos.nix-daemon` first)")] ExistingVolumeNotEncrypted(String, PathBuf), } From 8851b66c7029c2c63e259f53c0abb0354650daba Mon Sep 17 00:00:00 2001 From: Ana Hobden Date: Wed, 15 Feb 2023 13:15:41 -0800 Subject: [PATCH 12/16] More tweaking preparing for bootstrap/kickstart work --- src/action/macos/bootstrap_apfs_volume.rs | 1 + src/action/macos/enable_ownership.rs | 4 +-- src/action/macos/unmount_apfs_volume.rs | 40 ++++++++++++++++++----- src/os/darwin.rs | 5 ++- src/planner/macos.rs | 6 ++-- 5 files changed, 41 insertions(+), 15 deletions(-) diff --git a/src/action/macos/bootstrap_apfs_volume.rs b/src/action/macos/bootstrap_apfs_volume.rs index 958b4574..3313fbe8 100644 --- a/src/action/macos/bootstrap_apfs_volume.rs +++ b/src/action/macos/bootstrap_apfs_volume.rs @@ -58,6 +58,7 @@ impl Action for BootstrapApfsVolume { ) .await .map_err(|e| ActionError::Command(e))?; + execute_command( Command::new("launchctl") .process_group(0) diff --git a/src/action/macos/enable_ownership.rs b/src/action/macos/enable_ownership.rs index 9c0f700e..e35bb445 100644 --- a/src/action/macos/enable_ownership.rs +++ b/src/action/macos/enable_ownership.rs @@ -8,7 +8,7 @@ use crate::action::{ActionError, StatefulAction}; use crate::execute_command; use crate::action::{Action, ActionDescription}; -use crate::os::darwin::DiskUtilOutput; +use crate::os::darwin::DiskUtilInfoOutput; /** Enable ownership on a volume @@ -62,7 +62,7 @@ impl Action for EnableOwnership { .await .map_err(ActionError::Command)? .stdout; - let the_plist: DiskUtilOutput = plist::from_reader(Cursor::new(buf))?; + let the_plist: DiskUtilInfoOutput = plist::from_reader(Cursor::new(buf))?; the_plist.global_permissions_enabled }; diff --git a/src/action/macos/unmount_apfs_volume.rs b/src/action/macos/unmount_apfs_volume.rs index e8ba716a..3c8e8cac 100644 --- a/src/action/macos/unmount_apfs_volume.rs +++ b/src/action/macos/unmount_apfs_volume.rs @@ -1,3 +1,4 @@ +use std::io::Cursor; use std::path::{Path, PathBuf}; use tokio::process::Command; @@ -7,6 +8,7 @@ use crate::action::{ActionError, StatefulAction}; use crate::execute_command; use crate::action::{Action, ActionDescription}; +use crate::os::darwin::DiskUtilInfoOutput; /** Unmount an APFS volume @@ -52,15 +54,35 @@ impl Action for UnmountApfsVolume { async fn execute(&mut self) -> Result<(), ActionError> { let Self { disk: _, name } = self; - execute_command( - Command::new("/usr/sbin/diskutil") - .process_group(0) - .args(["unmount", "force"]) - .arg(name) - .stdin(std::process::Stdio::null()), - ) - .await - .map_err(|e| ActionError::Command(e))?; + let currently_mounted = { + let buf = execute_command( + Command::new("/usr/sbin/diskutil") + .process_group(0) + .args(["info", "-plist"]) + .arg(&name) + .stdin(std::process::Stdio::null()), + ) + .await + .map_err(ActionError::Command)? + .stdout; + let the_plist: DiskUtilInfoOutput = plist::from_reader(Cursor::new(buf))?; + + the_plist.mount_point.is_some() + }; + + if !currently_mounted { + execute_command( + Command::new("/usr/sbin/diskutil") + .process_group(0) + .args(["unmount", "force"]) + .arg(name) + .stdin(std::process::Stdio::null()), + ) + .await + .map_err(|e| ActionError::Command(e))?; + } else { + tracing::debug!("Volume was already unmounted, can skip unmounting") + } Ok(()) } diff --git a/src/os/darwin.rs b/src/os/darwin.rs index c5589904..56120841 100644 --- a/src/os/darwin.rs +++ b/src/os/darwin.rs @@ -1,8 +1,11 @@ +use std::path::PathBuf; + #[derive(serde::Deserialize)] #[serde(rename_all = "PascalCase")] -pub struct DiskUtilOutput { +pub struct DiskUtilInfoOutput { pub parent_whole_disk: String, pub global_permissions_enabled: bool, + pub mount_point: Option, } #[derive(serde::Deserialize, Clone, Debug)] diff --git a/src/planner/macos.rs b/src/planner/macos.rs index 6a038067..798bcb18 100644 --- a/src/planner/macos.rs +++ b/src/planner/macos.rs @@ -11,7 +11,7 @@ use crate::{ StatefulAction, }, execute_command, - os::darwin::DiskUtilOutput, + os::darwin::DiskUtilInfoOutput, planner::{Planner, PlannerError}, settings::InstallSettingsError, settings::{CommonSettings, InitSystem}, @@ -67,7 +67,7 @@ async fn default_root_disk() -> Result { .await .unwrap() .stdout; - let the_plist: DiskUtilOutput = plist::from_reader(Cursor::new(buf))?; + let the_plist: DiskUtilInfoOutput = plist::from_reader(Cursor::new(buf))?; Ok(the_plist.parent_whole_disk) } @@ -97,7 +97,7 @@ impl Planner for Macos { .await .unwrap() .stdout; - let the_plist: DiskUtilOutput = plist::from_reader(Cursor::new(buf)).unwrap(); + let the_plist: DiskUtilInfoOutput = plist::from_reader(Cursor::new(buf)).unwrap(); Some(the_plist.parent_whole_disk) }, From 9640f1df87b8d7a56a0945deab2594e755923a1a Mon Sep 17 00:00:00 2001 From: Ana Hobden Date: Fri, 17 Feb 2023 09:11:59 -0800 Subject: [PATCH 13/16] Most of volume curing works --- src/action/macos/bootstrap_apfs_volume.rs | 103 ------------- .../macos/bootstrap_launchctl_service.rs | 139 +++++++++++++++++ src/action/macos/create_apfs_volume.rs | 13 +- src/action/macos/create_nix_volume.rs | 21 ++- src/action/macos/enable_ownership.rs | 2 +- src/action/macos/encrypt_apfs_volume.rs | 2 +- .../macos/kickstart_launchctl_service.rs | 142 ++++++++++++++++++ src/action/macos/mod.rs | 6 +- src/lib.rs | 7 +- tests/plan.rs | 48 +++--- 10 files changed, 343 insertions(+), 140 deletions(-) delete mode 100644 src/action/macos/bootstrap_apfs_volume.rs create mode 100644 src/action/macos/bootstrap_launchctl_service.rs create mode 100644 src/action/macos/kickstart_launchctl_service.rs diff --git a/src/action/macos/bootstrap_apfs_volume.rs b/src/action/macos/bootstrap_apfs_volume.rs deleted file mode 100644 index 3313fbe8..00000000 --- a/src/action/macos/bootstrap_apfs_volume.rs +++ /dev/null @@ -1,103 +0,0 @@ -use std::path::{Path, PathBuf}; - -use tokio::process::Command; -use tracing::{span, Span}; - -use crate::action::{ActionError, StatefulAction}; -use crate::execute_command; - -use crate::action::{Action, ActionDescription}; - -/** -Bootstrap and kickstart an APFS volume -*/ -#[derive(Debug, serde::Deserialize, serde::Serialize, Clone)] -pub struct BootstrapApfsVolume { - path: PathBuf, -} - -impl BootstrapApfsVolume { - #[tracing::instrument(level = "debug", skip_all)] - pub async fn plan(path: impl AsRef) -> Result, ActionError> { - Ok(Self { - path: path.as_ref().to_path_buf(), - } - .into()) - } -} - -#[async_trait::async_trait] -#[typetag::serde(name = "bootstrap_volume")] -impl Action for BootstrapApfsVolume { - fn tracing_synopsis(&self) -> String { - format!("Bootstrap and kickstart `{}`", self.path.display()) - } - - fn tracing_span(&self) -> Span { - span!( - tracing::Level::DEBUG, - "bootstrap_volume", - path = %self.path.display(), - ) - } - - fn execute_description(&self) -> Vec { - vec![ActionDescription::new(self.tracing_synopsis(), vec![])] - } - - #[tracing::instrument(level = "debug", skip_all)] - async fn execute(&mut self) -> Result<(), ActionError> { - let Self { path } = self; - - execute_command( - Command::new("launchctl") - .process_group(0) - .args(["bootstrap", "system"]) - .arg(path) - .stdin(std::process::Stdio::null()), - ) - .await - .map_err(|e| ActionError::Command(e))?; - - execute_command( - Command::new("launchctl") - .process_group(0) - .args(["kickstart", "-k", "system/org.nixos.darwin-store"]) - .stdin(std::process::Stdio::null()), - ) - .await - .map_err(|e| ActionError::Command(e))?; - - Ok(()) - } - - fn revert_description(&self) -> Vec { - vec![ActionDescription::new( - format!("Stop `{}`", self.path.display()), - vec![], - )] - } - - #[tracing::instrument(level = "debug", skip_all)] - async fn revert(&mut self) -> Result<(), ActionError> { - let Self { path } = self; - - execute_command( - Command::new("launchctl") - .process_group(0) - .args(["bootout", "system"]) - .arg(path) - .stdin(std::process::Stdio::null()), - ) - .await - .map_err(|e| ActionError::Command(e))?; - - Ok(()) - } -} - -#[derive(Debug, thiserror::Error)] -pub enum BootstrapVolumeError { - #[error("Failed to execute command")] - Command(#[source] std::io::Error), -} diff --git a/src/action/macos/bootstrap_launchctl_service.rs b/src/action/macos/bootstrap_launchctl_service.rs new file mode 100644 index 00000000..6ed272a4 --- /dev/null +++ b/src/action/macos/bootstrap_launchctl_service.rs @@ -0,0 +1,139 @@ +use std::path::{Path, PathBuf}; + +use tokio::process::Command; +use tracing::{span, Span}; + +use crate::action::{ActionError, StatefulAction}; +use crate::execute_command; + +use crate::action::{Action, ActionDescription}; + +/** +Bootstrap and kickstart an APFS volume +*/ +#[derive(Debug, serde::Deserialize, serde::Serialize, Clone)] +pub struct BootstrapLaunchctlService { + domain: String, + service: String, + path: PathBuf, +} + +impl BootstrapLaunchctlService { + #[tracing::instrument(level = "debug", skip_all)] + pub async fn plan( + domain: impl AsRef, + service: impl AsRef, + path: impl AsRef, + ) -> Result, ActionError> { + let domain = domain.as_ref().to_string(); + let service = service.as_ref().to_string(); + let path = path.as_ref().to_path_buf(); + + let output = Command::new("launchctl") + .process_group(0) + .arg("print") + .arg(format!("{domain}/{service}")) + .arg("-plist") + .stdin(std::process::Stdio::null()) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .output() + .await + .map_err(|e| ActionError::Command(e))?; + if output.status.success() || output.status.code() == Some(37) { + // We presume that success means it's found + return Ok(StatefulAction::completed(Self { + service, + domain, + path, + })); + } + + Ok(StatefulAction::uncompleted(Self { + domain, + service, + path, + })) + } +} + +#[async_trait::async_trait] +#[typetag::serde(name = "bootstrap_launchctl_service")] +impl Action for BootstrapLaunchctlService { + fn tracing_synopsis(&self) -> String { + format!( + "Bootstrap the `{}` service via `launchctl bootstrap {} {}`", + self.service, + self.domain, + self.path.display() + ) + } + + fn tracing_span(&self) -> Span { + span!( + tracing::Level::DEBUG, + "bootstrap_launchctl_service", + domain = self.domain, + path = %self.path.display(), + ) + } + + fn execute_description(&self) -> Vec { + vec![ActionDescription::new(self.tracing_synopsis(), vec![])] + } + + #[tracing::instrument(level = "debug", skip_all)] + async fn execute(&mut self) -> Result<(), ActionError> { + let Self { + domain, + service: _, + path, + } = self; + + execute_command( + Command::new("launchctl") + .process_group(0) + .arg("bootstrap") + .arg(domain) + .arg(path) + .stdin(std::process::Stdio::null()), + ) + .await + .map_err(|e| ActionError::Command(e))?; + + Ok(()) + } + + fn revert_description(&self) -> Vec { + vec![ActionDescription::new( + format!( + "Run `launchctl bootout {} {}`", + self.domain, + self.path.display() + ), + vec![], + )] + } + + #[tracing::instrument(level = "debug", skip_all)] + async fn revert(&mut self) -> Result<(), ActionError> { + let Self { + path, + service: _, + domain, + } = self; + + execute_command( + Command::new("launchctl") + .process_group(0) + .arg("bootout") + .arg(domain) + .arg(path) + .stdin(std::process::Stdio::null()), + ) + .await + .map_err(|e| ActionError::Command(e))?; + + Ok(()) + } +} diff --git a/src/action/macos/create_apfs_volume.rs b/src/action/macos/create_apfs_volume.rs index b0bc95fd..3d91ac01 100644 --- a/src/action/macos/create_apfs_volume.rs +++ b/src/action/macos/create_apfs_volume.rs @@ -24,10 +24,15 @@ impl CreateApfsVolume { case_sensitive: bool, ) -> Result, ActionError> { // Mac lacks an `diskutil apfs info` command or analog, so we have to list - let output = - execute_command(Command::new("/usr/sbin/diskutil").args(["apfs", "list", "-plist"])) - .await - .map_err(ActionError::Command)?; + let output = execute_command( + Command::new("/usr/sbin/diskutil") + .args(["apfs", "list", "-plist"]) + .stdin(std::process::Stdio::null()) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()), + ) + .await + .map_err(ActionError::Command)?; let parsed: DiskUtilApfsListOutput = plist::from_bytes(&output.stdout)?; for container in parsed.containers { diff --git a/src/action/macos/create_nix_volume.rs b/src/action/macos/create_nix_volume.rs index 1f896e04..d27ce22a 100644 --- a/src/action/macos/create_nix_volume.rs +++ b/src/action/macos/create_nix_volume.rs @@ -1,7 +1,7 @@ use crate::action::{ base::{create_or_insert_into_file, CreateFile, CreateOrInsertIntoFile}, macos::{ - BootstrapApfsVolume, CreateApfsVolume, CreateSyntheticObjects, EnableOwnership, + BootstrapLaunchctlService, CreateApfsVolume, CreateSyntheticObjects, EnableOwnership, EncryptApfsVolume, UnmountApfsVolume, }, Action, ActionDescription, ActionError, StatefulAction, @@ -13,7 +13,7 @@ use std::{ use tokio::process::Command; use tracing::{span, Span}; -use super::create_fstab_entry::CreateFstabEntry; +use super::{create_fstab_entry::CreateFstabEntry, KickstartLaunchctlService}; pub const NIX_VOLUME_MOUNTD_DEST: &str = "/Library/LaunchDaemons/org.nixos.darwin-store.plist"; @@ -31,7 +31,8 @@ pub struct CreateNixVolume { create_fstab_entry: StatefulAction, encrypt_volume: Option>, setup_volume_daemon: StatefulAction, - bootstrap_volume: StatefulAction, + bootstrap_volume: StatefulAction, + kickstart_launchctl_service: StatefulAction, enable_ownership: StatefulAction, } @@ -108,7 +109,14 @@ impl CreateNixVolume { let setup_volume_daemon = CreateFile::plan(NIX_VOLUME_MOUNTD_DEST, None, None, None, mount_plist, false).await?; - let bootstrap_volume = BootstrapApfsVolume::plan(NIX_VOLUME_MOUNTD_DEST).await?; + let bootstrap_volume = BootstrapLaunchctlService::plan( + "system", + "org.nixos.darwin-store", + NIX_VOLUME_MOUNTD_DEST, + ) + .await?; + let kickstart_launchctl_service = + KickstartLaunchctlService::plan("system/org.nixos.darwin-store").await?; let enable_ownership = EnableOwnership::plan("/nix").await?; Ok(Self { @@ -124,6 +132,7 @@ impl CreateNixVolume { encrypt_volume, setup_volume_daemon, bootstrap_volume, + kickstart_launchctl_service, enable_ownership, } .into()) @@ -186,6 +195,7 @@ impl Action for CreateNixVolume { encrypt_volume, setup_volume_daemon, bootstrap_volume, + kickstart_launchctl_service, enable_ownership, } = self; @@ -200,6 +210,7 @@ impl Action for CreateNixVolume { setup_volume_daemon.try_execute().await?; bootstrap_volume.try_execute().await?; + kickstart_launchctl_service.try_execute().await?; let mut retry_tokens: usize = 50; loop { @@ -250,10 +261,12 @@ impl Action for CreateNixVolume { encrypt_volume, setup_volume_daemon, bootstrap_volume, + kickstart_launchctl_service, enable_ownership, } = self; enable_ownership.try_revert().await?; + kickstart_launchctl_service.try_revert().await?; bootstrap_volume.try_revert().await?; setup_volume_daemon.try_revert().await?; if let Some(encrypt_volume) = encrypt_volume { diff --git a/src/action/macos/enable_ownership.rs b/src/action/macos/enable_ownership.rs index e35bb445..f7f9b640 100644 --- a/src/action/macos/enable_ownership.rs +++ b/src/action/macos/enable_ownership.rs @@ -32,7 +32,7 @@ impl EnableOwnership { #[typetag::serde(name = "enable_ownership")] impl Action for EnableOwnership { fn tracing_synopsis(&self) -> String { - format!("Enable ownership on {}", self.path.display()) + format!("Enable ownership on `{}`", self.path.display()) } fn tracing_span(&self) -> Span { diff --git a/src/action/macos/encrypt_apfs_volume.rs b/src/action/macos/encrypt_apfs_volume.rs index 87cd077e..b23a1959 100644 --- a/src/action/macos/encrypt_apfs_volume.rs +++ b/src/action/macos/encrypt_apfs_volume.rs @@ -59,7 +59,7 @@ impl EncryptApfsVolume { return Ok(StatefulAction::completed(Self { name, disk })); } - // Aske the user to remove it + // Ask the user to remove it return Err(ActionError::Custom(Box::new( EncryptApfsVolumeError::ExistingPasswordFound(name, disk), ))); diff --git a/src/action/macos/kickstart_launchctl_service.rs b/src/action/macos/kickstart_launchctl_service.rs new file mode 100644 index 00000000..0404e7ee --- /dev/null +++ b/src/action/macos/kickstart_launchctl_service.rs @@ -0,0 +1,142 @@ +use std::process::Output; + +use tokio::process::Command; +use tracing::{span, Span}; + +use crate::action::{ActionError, StatefulAction}; +use crate::execute_command; + +use crate::action::{Action, ActionDescription}; + +/** +Bootstrap and kickstart an APFS volume +*/ +#[derive(Debug, serde::Deserialize, serde::Serialize, Clone)] +pub struct KickstartLaunchctlService { + service: String, +} + +impl KickstartLaunchctlService { + #[tracing::instrument(level = "debug", skip_all)] + pub async fn plan(service: impl AsRef) -> Result, ActionError> { + let service = service.as_ref().to_string(); + + let mut service_exists = false; + let mut service_started = false; + let output = Command::new("launchctl") + .process_group(0) + .arg("print") + .arg(&service) + .arg("-plist") + .stdin(std::process::Stdio::null()) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .output() + .await + .map_err(|e| ActionError::Command(e))?; + if output.status.success() { + service_exists = true; + + let output_string = String::from_utf8(output.stdout)?; + // We are looking for a line containing "state = " with some trailing content + // The output is not a JSON or a plist + // MacOS's man pages explicitly tell us not to try to parse this output + // MacOS's man pages explicitly tell us this output is not stable + // Yet, here we are, doing exactly that. + for output_line in output_string.lines() { + let output_line_trimmed = output_line.trim(); + if output_line_trimmed.starts_with("state") { + if output_line_trimmed.contains("running") { + service_started = true; + } + break; + } + } + } + + if service_exists && service_started { + return Ok(StatefulAction::completed(Self { service })); + } + + // It's safe to assume the user does not have the service started + Ok(StatefulAction::uncompleted(Self { service })) + } +} + +#[async_trait::async_trait] +#[typetag::serde(name = "kickstart_launchctl_service")] +impl Action for KickstartLaunchctlService { + fn tracing_synopsis(&self) -> String { + format!("Run `launchctl kickstart {}`", self.service) + } + + fn tracing_span(&self) -> Span { + span!( + tracing::Level::DEBUG, + "kickstart_launchctl_service", + path = %self.service, + ) + } + + fn execute_description(&self) -> Vec { + vec![ActionDescription::new(self.tracing_synopsis(), vec![])] + } + + #[tracing::instrument(level = "debug", skip_all)] + async fn execute(&mut self) -> Result<(), ActionError> { + let Self { service } = self; + + execute_command( + Command::new("launchctl") + .process_group(0) + .args(["kickstart", "-k"]) + .arg(service) + .stdin(std::process::Stdio::null()), + ) + .await + .map_err(|e| ActionError::Command(e))?; + + Ok(()) + } + + fn revert_description(&self) -> Vec { + vec![ActionDescription::new( + format!("Run `launchctl stop {}`", self.service), + vec![], + )] + } + + #[tracing::instrument(level = "debug", skip_all)] + async fn revert(&mut self) -> Result<(), ActionError> { + let Self { service } = self; + + // MacOs doesn't offer an "ensure-stopped" like they do with Kickstart + let mut command = Command::new("launchctl"); + command.process_group(0); + command.arg("stop"); + command.arg(service); + command.stdin(std::process::Stdio::null()); + let command_str = format!("{:?}", command.as_std()); + let output = command + .output() + .await + .map_err(|e| ActionError::Command(e))?; + // On our test Macs, a status code of `3` was reported if the service was stopped while not running. + match output.status.code() { + Some(3) | Some(0) | None => (), + _ => { + return Err(ActionError::Custom(Box::new( + KickstartLaunchctlServiceError::CannotStopService(command_str, output), + ))) + }, + } + + Ok(()) + } +} + +#[derive(Debug, thiserror::Error)] +pub enum KickstartLaunchctlServiceError { + #[error("Command `{0}` failed, stderr: {}", String::from_utf8(.1.stderr.clone()).unwrap_or_else(|_e| String::from("")))] + CannotStopService(String, Output), +} diff --git a/src/action/macos/mod.rs b/src/action/macos/mod.rs index 638c104a..1d5aa1a0 100644 --- a/src/action/macos/mod.rs +++ b/src/action/macos/mod.rs @@ -1,19 +1,21 @@ /*! [`Action`](crate::action::Action)s for Darwin based systems */ -pub(crate) mod bootstrap_apfs_volume; +pub(crate) mod bootstrap_launchctl_service; pub(crate) mod create_apfs_volume; pub(crate) mod create_fstab_entry; pub(crate) mod create_nix_volume; pub(crate) mod create_synthetic_objects; pub(crate) mod enable_ownership; pub(crate) mod encrypt_apfs_volume; +pub(crate) mod kickstart_launchctl_service; pub(crate) mod unmount_apfs_volume; -pub use bootstrap_apfs_volume::{BootstrapApfsVolume, BootstrapVolumeError}; +pub use bootstrap_launchctl_service::BootstrapLaunchctlService; pub use create_apfs_volume::CreateApfsVolume; pub use create_nix_volume::{CreateNixVolume, NIX_VOLUME_MOUNTD_DEST}; pub use create_synthetic_objects::{CreateSyntheticObjects, CreateSyntheticObjectsError}; pub use enable_ownership::{EnableOwnership, EnableOwnershipError}; pub use encrypt_apfs_volume::EncryptApfsVolume; +pub use kickstart_launchctl_service::KickstartLaunchctlService; pub use unmount_apfs_volume::UnmountApfsVolume; diff --git a/src/lib.rs b/src/lib.rs index 36fef2fa..e9ab0172 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -100,7 +100,12 @@ async fn execute_command(command: &mut Command) -> Result Err(std::io::Error::new( std::io::ErrorKind::Other, format!( - "Command `{command_str}` failed status, stderr:\n{}\n", + "Command `{command_str}` failed{}, stderr:\n{}\n", + if let Some(code) = output.status.code() { + format!(" status {code}") + } else { + "".into() + }, String::from_utf8(output.stderr).unwrap_or_else(|_e| String::from("")) ), )), diff --git a/tests/plan.rs b/tests/plan.rs index e34ad0f7..fec314de 100644 --- a/tests/plan.rs +++ b/tests/plan.rs @@ -7,29 +7,29 @@ const STEAM_DECK: &str = include_str!("./fixtures/linux/steam-deck.json"); #[cfg(target_os = "macos")] const MACOS: &str = include_str!("./fixtures/macos/macos.json"); -// Ensure existing plans still parse -// If this breaks and you need to update the fixture, disable these tests, bump `nix_installer` to a new version, and update the plans. -#[cfg(target_os = "linux")] -#[test] -fn plan_compat_linux() -> eyre::Result<()> { - let _: InstallPlan = serde_json::from_str(LINUX)?; - Ok(()) -} +// // Ensure existing plans still parse +// // If this breaks and you need to update the fixture, disable these tests, bump `nix_installer` to a new version, and update the plans. +// #[cfg(target_os = "linux")] +// #[test] +// fn plan_compat_linux() -> eyre::Result<()> { +// let _: InstallPlan = serde_json::from_str(LINUX)?; +// Ok(()) +// } -// Ensure existing plans still parse -// If this breaks and you need to update the fixture, disable these tests, bump `nix_installer` to a new version, and update the plans. -#[cfg(target_os = "linux")] -#[test] -fn plan_compat_steam_deck() -> eyre::Result<()> { - let _: InstallPlan = serde_json::from_str(STEAM_DECK)?; - Ok(()) -} +// // Ensure existing plans still parse +// // If this breaks and you need to update the fixture, disable these tests, bump `nix_installer` to a new version, and update the plans. +// #[cfg(target_os = "linux")] +// #[test] +// fn plan_compat_steam_deck() -> eyre::Result<()> { +// let _: InstallPlan = serde_json::from_str(STEAM_DECK)?; +// Ok(()) +// } -// Ensure existing plans still parse -// If this breaks and you need to update the fixture, disable these tests, bump `nix_installer` to a new version, and update the plans. -#[cfg(target_os = "macos")] -#[test] -fn plan_compat_macos() -> eyre::Result<()> { - let _: InstallPlan = serde_json::from_str(MACOS)?; - Ok(()) -} +// // Ensure existing plans still parse +// // If this breaks and you need to update the fixture, disable these tests, bump `nix_installer` to a new version, and update the plans. +// #[cfg(target_os = "macos")] +// #[test] +// fn plan_compat_macos() -> eyre::Result<()> { +// let _: InstallPlan = serde_json::from_str(MACOS)?; +// Ok(()) +// } From 0672a9e46ce48c0072126d4720c8031f38d4ada4 Mon Sep 17 00:00:00 2001 From: Ana Hobden Date: Mon, 6 Mar 2023 09:45:24 -0800 Subject: [PATCH 14/16] Make kickstart use domain/service too --- src/action/macos/create_nix_volume.rs | 2 +- .../macos/kickstart_launchctl_service.rs | 19 ++++++++++++------- tests/fixtures/macos/macos.json | 11 ++++++++++- 3 files changed, 23 insertions(+), 9 deletions(-) diff --git a/src/action/macos/create_nix_volume.rs b/src/action/macos/create_nix_volume.rs index 52c53bb5..0502027f 100644 --- a/src/action/macos/create_nix_volume.rs +++ b/src/action/macos/create_nix_volume.rs @@ -125,7 +125,7 @@ impl CreateNixVolume { .await .map_err(|e| ActionError::Child(BootstrapLaunchctlService::action_tag(), Box::new(e)))?; let kickstart_launchctl_service = - KickstartLaunchctlService::plan("system/org.nixos.darwin-store") + KickstartLaunchctlService::plan("system", "org.nixos.darwin-store") .await .map_err(|e| { ActionError::Child(KickstartLaunchctlService::action_tag(), Box::new(e)) diff --git a/src/action/macos/kickstart_launchctl_service.rs b/src/action/macos/kickstart_launchctl_service.rs index 2794831c..846ac2b9 100644 --- a/src/action/macos/kickstart_launchctl_service.rs +++ b/src/action/macos/kickstart_launchctl_service.rs @@ -13,12 +13,17 @@ Bootstrap and kickstart an APFS volume */ #[derive(Debug, serde::Deserialize, serde::Serialize, Clone)] pub struct KickstartLaunchctlService { + domain: String, service: String, } impl KickstartLaunchctlService { #[tracing::instrument(level = "debug", skip_all)] - pub async fn plan(service: impl AsRef) -> Result, ActionError> { + pub async fn plan( + domain: impl AsRef, + service: impl AsRef, + ) -> Result, ActionError> { + let domain = domain.as_ref().to_string(); let service = service.as_ref().to_string(); let mut service_exists = false; @@ -56,11 +61,11 @@ impl KickstartLaunchctlService { } if service_exists && service_started { - return Ok(StatefulAction::completed(Self { service })); + return Ok(StatefulAction::completed(Self { domain, service })); } // It's safe to assume the user does not have the service started - Ok(StatefulAction::uncompleted(Self { service })) + Ok(StatefulAction::uncompleted(Self { domain, service })) } } @@ -88,13 +93,13 @@ impl Action for KickstartLaunchctlService { #[tracing::instrument(level = "debug", skip_all)] async fn execute(&mut self) -> Result<(), ActionError> { - let Self { service } = self; + let Self { domain, service } = self; execute_command( Command::new("launchctl") .process_group(0) .args(["kickstart", "-k"]) - .arg(service) + .arg(format!("{domain}/{service}")) .stdin(std::process::Stdio::null()), ) .await?; @@ -111,13 +116,13 @@ impl Action for KickstartLaunchctlService { #[tracing::instrument(level = "debug", skip_all)] async fn revert(&mut self) -> Result<(), ActionError> { - let Self { service } = self; + let Self { domain, service } = self; // MacOs doesn't offer an "ensure-stopped" like they do with Kickstart let mut command = Command::new("launchctl"); command.process_group(0); command.arg("stop"); - command.arg(service); + command.arg(format!("{domain}/{service}")); command.stdin(std::process::Stdio::null()); let command_str = format!("{:?}", command.as_std()); let output = command diff --git a/tests/fixtures/macos/macos.json b/tests/fixtures/macos/macos.json index 9c09e404..c95e2642 100644 --- a/tests/fixtures/macos/macos.json +++ b/tests/fixtures/macos/macos.json @@ -65,10 +65,19 @@ }, "bootstrap_volume": { "action": { + "domain": "system", + "service": "org.nixos.darwin-store", "path": "/Library/LaunchDaemons/org.nixos.darwin-store.plist" }, "state": "Uncompleted" }, + "kickstart_launchctl_service": { + "action": { + "domain": "system", + "service": "org.nixos.darwin-store" + }, + "state": "Uncompleted" + }, "enable_ownership": { "action": { "path": "/nix" @@ -970,4 +979,4 @@ "volume_label": "Nix Store", "root_disk": "disk3" } -} +} \ No newline at end of file From 2669dab852e044ce131aad1d9d30e10e05284a1a Mon Sep 17 00:00:00 2001 From: Ana Hobden Date: Wed, 8 Mar 2023 07:51:32 -0800 Subject: [PATCH 15/16] Fixup nits --- .../macos/bootstrap_launchctl_service.rs | 2 +- src/action/macos/create_nix_volume.rs | 28 +++++++++++++++---- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/src/action/macos/bootstrap_launchctl_service.rs b/src/action/macos/bootstrap_launchctl_service.rs index b49d3013..f6ec940e 100644 --- a/src/action/macos/bootstrap_launchctl_service.rs +++ b/src/action/macos/bootstrap_launchctl_service.rs @@ -32,7 +32,7 @@ impl BootstrapLaunchctlService { let mut command = Command::new("launchctl"); command.process_group(0); command.arg("print"); - command.arg(format!("{domain}/{service}")); + command.arg("{domain}/{service}"); command.arg("-plist"); command.stdin(std::process::Stdio::null()); command.stdout(std::process::Stdio::piped()); diff --git a/src/action/macos/create_nix_volume.rs b/src/action/macos/create_nix_volume.rs index 0502027f..22117621 100644 --- a/src/action/macos/create_nix_volume.rs +++ b/src/action/macos/create_nix_volume.rs @@ -272,13 +272,29 @@ impl Action for CreateNixVolume { } fn revert_description(&self) -> Vec { - let Self { disk, name, .. } = &self; - // TODO(@hoverbear): Do a better description here. + let mut explanation = vec![ + self.create_or_append_synthetic_conf.tracing_synopsis(), + self.create_synthetic_objects.tracing_synopsis(), + self.unmount_volume.tracing_synopsis(), + self.create_volume.tracing_synopsis(), + self.create_fstab_entry.tracing_synopsis(), + ]; + if let Some(encrypt_volume) = &self.encrypt_volume { + explanation.push(encrypt_volume.tracing_synopsis()); + } + explanation.append(&mut vec![ + self.setup_volume_daemon.tracing_synopsis(), + self.bootstrap_volume.tracing_synopsis(), + self.enable_ownership.tracing_synopsis(), + ]); + vec![ActionDescription::new( - format!("Remove the APFS volume `{name}` on `{}`", disk.display()), - vec![format!( - "Create a writable, persistent systemd system extension.", - )], + format!( + "Remove the APFS volume `{}` on `{}`", + self.name, + self.disk.display() + ), + explanation, )] } From f83a308c04d32c8e2be4b7e348711f5005ca7da1 Mon Sep 17 00:00:00 2001 From: Ana Hobden Date: Wed, 8 Mar 2023 11:02:00 -0800 Subject: [PATCH 16/16] Fix a missing format! --- src/action/macos/bootstrap_launchctl_service.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/action/macos/bootstrap_launchctl_service.rs b/src/action/macos/bootstrap_launchctl_service.rs index f6ec940e..b49d3013 100644 --- a/src/action/macos/bootstrap_launchctl_service.rs +++ b/src/action/macos/bootstrap_launchctl_service.rs @@ -32,7 +32,7 @@ impl BootstrapLaunchctlService { let mut command = Command::new("launchctl"); command.process_group(0); command.arg("print"); - command.arg("{domain}/{service}"); + command.arg(format!("{domain}/{service}")); command.arg("-plist"); command.stdin(std::process::Stdio::null()); command.stdout(std::process::Stdio::piped());