Fix handling of the --store argument. (#12)

flake.nix

@@ -53,6 +53,11 @@
         inherit nixpkgs self;
         nixos = "${nixpkgs}/nixos";
       };
+
+      # Only useful for quick tests
+      systemConfigs.default = self.lib.makeSystemConfig {
+        modules = [ ./examples/example.nix ];
+      };
     }
     //
     (flake-utils.lib.eachSystem

src/lib.rs

@@ -22,15 +22,12 @@ pub struct StorePath {
     pub store_path: PathBuf,
 }
 
+// We cannot implement both From and TryFrom, and we need From for Deserialize...
 impl From<String> for StorePath {
     fn from(path: String) -> Self {
-        // TODO: handle this better
+        PathBuf::from(path)
-        if !path.starts_with("/nix/store/") {
+            .try_into()
-            panic!("Error constructing store path, not in store: {path}");
+            .expect("Error constructing store path, path not in store.")
-        }
-        StorePath {
-            store_path: PathBuf::from(path),
-        }
     }
 }
 
@@ -40,6 +37,22 @@ impl From<StorePath> for PathBuf {
     }
 }
 
+impl TryFrom<PathBuf> for StorePath {
+    type Error = anyhow::Error;
+
+    fn try_from(path: PathBuf) -> Result<Self> {
+        let canon = path.canonicalize()?;
+        if !canon.starts_with(PathBuf::from("/").join("nix").join("store")) {
+            anyhow::bail!(
+                "Error constructing store path, not in store: {} (canonicalised: {})",
+                path.display(),
+                canon.display()
+            );
+        }
+        Ok(Self { store_path: canon })
+    }
+}
+
 impl From<StorePath> for String {
     fn from(value: StorePath) -> Self {
         format!("{}", value.store_path.display())

src/main.rs

@@ -1,7 +1,7 @@
 use anyhow::Result;
 use clap::Parser;
 use std::ffi::OsString;
-use std::path::Path;
+use std::path::{Path, PathBuf};
 use std::process::{self, ExitCode};
 
 use system_manager::StorePath;
@@ -53,12 +53,12 @@ struct ActivationArgs {
 }
 
 #[derive(clap::Args, Debug)]
-struct DeactivationArgs {
+struct OptionalStorePathArgs {
-    #[arg(long)]
+    #[arg(long = "store-path", name = "STORE_PATH")]
     /// The store path for the system-manager profile.
     /// You only need to specify this explicitly if it differs from the active
-    /// system-manager profile
+    /// system-manager profile.
-    store_path: Option<StorePath>,
+    maybe_store_path: Option<StorePath>,
 }
 
 #[derive(clap::Subcommand, Debug)]
@@ -71,10 +71,11 @@ enum Action {
         #[command(flatten)]
         activation_args: ActivationArgs,
     },
+    /// Put all files defined by the given generation in place, but do not start
+    /// services. Useful in build scripts.
     PrePopulate {
-        #[arg(long)]
+        #[command(flatten)]
-        /// The store path containing the system-manager profile to activate
+        optional_store_path_args: OptionalStorePathArgs,
-        store_path: StorePath,
         #[command(flatten)]
         activation_args: ActivationArgs,
     },
@@ -86,7 +87,7 @@ enum Action {
     /// Deactivate the active system-manager profile, removing all managed configuration
     Deactivate {
         #[command(flatten)]
-        deactivation_args: DeactivationArgs,
+        optional_store_path_args: OptionalStorePathArgs,
     },
     /// Generate a new system-manager profile and
     /// register is as the active system-manager profile
@@ -125,9 +126,10 @@ fn go(args: Args) -> Result<()> {
             activate(&store_path, ephemeral, &target_host, use_remote_sudo)
         }
         Action::PrePopulate {
-            store_path,
+            optional_store_path_args: OptionalStorePathArgs { maybe_store_path },
             activation_args: ActivationArgs { ephemeral },
         } => {
+            let store_path = store_path_or_active_profile(maybe_store_path).try_into()?;
             copy_closure(&store_path, &target_host)?;
             prepopulate(&store_path, ephemeral, &target_host, use_remote_sudo)
         }
@@ -135,8 +137,8 @@ fn go(args: Args) -> Result<()> {
             build_args: BuildArgs { flake_uri },
         } => build(&flake_uri, &target_host),
         Action::Deactivate {
-            deactivation_args: DeactivationArgs { store_path },
+            optional_store_path_args: OptionalStorePathArgs { maybe_store_path },
-        } => deactivate(store_path, &target_host, use_remote_sudo),
+        } => deactivate(maybe_store_path, &target_host, use_remote_sudo),
         Action::Generate { generate_args } => {
             generate(&generate_args, &target_host, use_remote_sudo)
         }
@@ -260,20 +262,13 @@ fn prepopulate(
 }
 
 fn deactivate(
-    store_path: Option<StorePath>,
+    maybe_store_path: Option<StorePath>,
     target_host: &Option<String>,
     use_remote_sudo: bool,
 ) -> Result<()> {
     if let Some(target_host) = target_host {
-        invoke_remote_script(
+        let store_path = store_path_or_active_profile(maybe_store_path);
-            &store_path.map_or_else(
+        invoke_remote_script(&store_path, "deactivate", target_host, use_remote_sudo)?;
-                || Path::new(system_manager::PROFILE_DIR).join("system-manager"),
-                |store_path| store_path.store_path,
-            ),
-            "deactivate",
-            target_host,
-            use_remote_sudo,
-        )?;
         Ok(())
     } else {
         check_root()?;
@@ -306,7 +301,7 @@ fn do_copy_closure(store_path: &StorePath, target_host: &str) -> Result<()> {
 }
 
 fn invoke_remote_script(
-    store_path: &Path,
+    path: &Path,
     script_name: &str,
     target_host: &str,
     use_remote_sudo: bool,
@@ -318,8 +313,7 @@ fn invoke_remote_script(
     }
     let status = cmd
         .arg(OsString::from(
-            store_path
+            path.join("bin")
-                .join("bin")
                 .join(script_name)
                 .to_string_lossy()
                 .into_owned(),
@@ -337,6 +331,17 @@ fn check_root() -> Result<()> {
     Ok(())
 }
 
+fn store_path_or_active_profile(maybe_store_path: Option<StorePath>) -> PathBuf {
+    maybe_store_path.map_or_else(
+        || {
+            let path = Path::new(system_manager::PROFILE_DIR).join("system-manager");
+            log::info!("No store path provided, using {}", path.display());
+            path
+        },
+        |store_path| store_path.store_path,
+    )
+}
+
 fn handle_toplevel_error<T>(r: Result<T>) -> ExitCode {
     if let Err(e) = r {
         log::error!("{:?}", e);

test/nix/modules/default.nix

@@ -71,7 +71,7 @@ forEachUbuntuImage
 
               node1.wait_for_unit("default.target")
 
-              node1.execute("/system-manager-profile/bin/activate")
+              node1.succeed("/system-manager-profile/bin/activate")
               node1.wait_for_unit("system-manager.target")
 
               node1.wait_for_unit("service-9.service")
@@ -81,7 +81,7 @@ forEachUbuntuImage
               node1.succeed("grep -F 'launch_the_rockets = true' /etc/foo.conf")
               node1.fail("grep -F 'launch_the_rockets = false' /etc/foo.conf")
 
-              node1.execute("${newConfig}/bin/activate")
+              node1.succeed("${newConfig}/bin/activate")
               node1.wait_for_unit("new-service.service")
               node1.wait_until_fails("systemctl status service-9.service")
               node1.wait_until_fails("cat /etc/a/nested/example/foo3")
@@ -104,7 +104,7 @@ forEachUbuntuImage
               node1.wait_until_fails("cat /etc/baz/bar/foo2")
               node1.wait_for_file("/etc/foo_new")
 
-              node1.execute("${newConfig}/bin/deactivate")
+              node1.succeed("${newConfig}/bin/deactivate")
               node1.wait_until_fails("systemctl status new-service.service")
               node1.wait_until_fails("cat /etc/foo_new")
             '';
@@ -145,7 +145,7 @@ forEachUbuntuImage
 
               node1.wait_for_unit("default.target")
 
-              node1.execute("/system-manager-profile/bin/prepopulate")
+              node1.succeed("/system-manager-profile/bin/prepopulate")
               node1.systemctl("daemon-reload")
               node1.systemctl("start default.target")
               node1.wait_for_unit("system-manager.target")
@@ -157,14 +157,14 @@ forEachUbuntuImage
               node1.succeed("grep -F 'launch_the_rockets = true' /etc/foo.conf")
               node1.fail("grep -F 'launch_the_rockets = false' /etc/foo.conf")
 
-              node1.execute("${newConfig}/bin/activate")
+              node1.succeed("${newConfig}/bin/activate")
               node1.wait_for_unit("new-service.service")
               node1.wait_until_fails("systemctl status service-9.service")
               node1.wait_until_fails("cat /etc/a/nested/example/foo3")
               node1.wait_until_fails("cat /etc/baz/bar/foo2")
               node1.wait_for_file("/etc/foo_new")
 
-              node1.execute("${newConfig}/bin/deactivate")
+              node1.succeed("${newConfig}/bin/deactivate")
               node1.wait_until_fails("systemctl status new-service.service")
               node1.wait_until_fails("cat /etc/foo_new")
             '';