Merge #1072

1072: Fix persistent data volumes with remote cross. r=Emilgardis a=Alexhuszagh

This fixes copying files into a directory inside a docker data volume, where previously we copied the entire temp directory into the volume itself. This meant that `tmp.tmpLkX8M3` would be copied to `"${volume}/tmp.tmpLkX8M3"`, rather than its contents being copied into `"${volume}"`. This also contains numerous other bugs fixes and changes internally, which have been submitted each as separate commits.

This fixes a regression in #1054 to re-enable fingerprinting to work, since previously the fingerprint file name was based off the container name. Since #1054 changes the container names to rely on the system time, this ensures the fingerprints are specific for only the toolchain and the path of directory.

This also simplifies the internals of the docker module substantially. Most free functions have been modified to become methods of structs to simplify their function signatures. Some structs have also been renamed to make it clearer what their purpose is.

Closes #1016.

Co-authored-by: Alex Huszagh <ahuszagh@gmail.com>

Author: bors[bot]

src/bin/commands/containers.rs

@@ -320,7 +320,8 @@ fn get_cross_volumes(
     msg_info: &mut MessageInfo,
 ) -> cross::Result<Vec<String>> {
     use cross::docker::VOLUME_PREFIX;
-    let stdout = docker::subcommand(engine, "volume")
+    let stdout = engine
+        .subcommand("volume")
         .arg("list")
         .args(["--format", "{{.Name}}"])
         // handles simple regex: ^ for start of line.
@@ -348,7 +349,7 @@ pub fn remove_all_volumes(
 ) -> cross::Result<()> {
     let volumes = get_cross_volumes(engine, msg_info)?;
 
-    let mut command = docker::subcommand(engine, "volume");
+    let mut command = engine.subcommand("volume");
     command.arg("rm");
     if force {
         command.arg("--force");
@@ -370,7 +371,7 @@ pub fn prune_volumes(
     engine: &docker::Engine,
     msg_info: &mut MessageInfo,
 ) -> cross::Result<()> {
-    let mut command = docker::subcommand(engine, "volume");
+    let mut command = engine.subcommand("volume");
     command.args(["prune", "--force"]);
     if execute {
         command.run(msg_info, false).map_err(Into::into)
@@ -397,34 +398,34 @@ pub fn create_persistent_volume(
     };
     let mount_finder = docker::MountFinder::create(engine)?;
     let dirs = docker::ToolchainDirectories::assemble(&mount_finder, toolchain.clone())?;
-    let container = dirs.unique_container_identifier(&toolchain.host().target)?;
-    let volume = dirs.unique_toolchain_identifier()?;
+    let container_id = dirs.unique_container_identifier(&toolchain.host().target)?;
+    let volume_id = dirs.unique_toolchain_identifier()?;
+    let volume = docker::DockerVolume::new(engine, &volume_id);
 
-    if docker::volume_exists(engine, &volume, msg_info)? {
-        eyre::bail!("Error: volume {volume} already exists.");
+    if volume.exists(msg_info)? {
+        eyre::bail!("Error: volume {volume_id} already exists.");
     }
 
-    docker::subcommand(engine, "volume")
-        .args(["create", &volume])
-        .run_and_get_status(msg_info, false)?;
+    volume.create(msg_info)?;
 
     // stop the container if it's already running
-    let state = docker::container_state(engine, &container, msg_info)?;
+    let container = docker::DockerContainer::new(engine, &container_id);
+    let state = container.state(msg_info)?;
     if !state.is_stopped() {
-        msg_info.warn(format_args!("container {container} was running."))?;
-        docker::container_stop_default(engine, &container, msg_info)?;
+        msg_info.warn(format_args!("container {container_id} was running."))?;
+        container.stop_default(msg_info)?;
     }
     if state.exists() {
-        msg_info.warn(format_args!("container {container} was exited."))?;
-        docker::container_rm(engine, &container, msg_info)?;
+        msg_info.warn(format_args!("container {container_id} was exited."))?;
+        container.remove(msg_info)?;
     }
 
     // create a dummy running container to copy data over
     let mount_prefix = docker::MOUNT_PREFIX;
-    let mut docker = docker::subcommand(engine, "run");
-    docker.args(["--name", &container]);
+    let mut docker = engine.subcommand("run");
+    docker.args(["--name", &container_id]);
     docker.arg("--rm");
-    docker.args(["-v", &format!("{}:{}", volume, mount_prefix)]);
+    docker.args(["-v", &format!("{}:{}", volume_id, mount_prefix)]);
     docker.arg("-d");
     let is_tty = io::Stdin::is_atty() && io::Stdout::is_atty() && io::Stderr::is_atty();
     if is_tty {
@@ -440,34 +441,15 @@ pub fn create_persistent_volume(
         docker.args(["sh", "-c", "sleep infinity"]);
     }
     // store first, since failing to non-existing container is fine
-    docker::Container::create(engine.clone(), container.clone())?;
-    docker.run_and_get_status(msg_info, false)?;
-
-    docker::remote::copy_volume_container_xargo(
-        engine,
-        &container,
-        &dirs,
-        mount_prefix.as_ref(),
-        msg_info,
-    )?;
-    docker::remote::copy_volume_container_cargo(
-        engine,
-        &container,
-        &dirs,
-        mount_prefix.as_ref(),
-        copy_registry,
-        msg_info,
-    )?;
-    docker::remote::copy_volume_container_rust(
-        engine,
-        &container,
-        &dirs,
-        None,
-        mount_prefix.as_ref(),
-        msg_info,
-    )?;
-
-    docker::Container::finish_static(is_tty, msg_info);
+    docker::ChildContainer::create(engine.clone(), container_id.clone())?;
+    docker.run_and_get_status(msg_info, true)?;
+
+    let data_volume = docker::ContainerDataVolume::new(engine, &container_id, &dirs);
+    data_volume.copy_xargo(mount_prefix.as_ref(), msg_info)?;
+    data_volume.copy_cargo(mount_prefix.as_ref(), copy_registry, msg_info)?;
+    data_volume.copy_rust(None, mount_prefix.as_ref(), msg_info)?;
+
+    docker::ChildContainer::finish_static(is_tty, msg_info);
 
     Ok(())
 }
@@ -484,13 +466,14 @@ pub fn remove_persistent_volume(
     };
     let mount_finder = docker::MountFinder::create(engine)?;
     let dirs = docker::ToolchainDirectories::assemble(&mount_finder, toolchain)?;
-    let volume = dirs.unique_toolchain_identifier()?;
+    let volume_id = dirs.unique_toolchain_identifier()?;
+    let volume = docker::DockerVolume::new(engine, &volume_id);
 
-    if !docker::volume_exists(engine, &volume, msg_info)? {
-        eyre::bail!("Error: volume {volume} does not exist.");
+    if !volume.exists(msg_info)? {
+        eyre::bail!("Error: volume {volume_id} does not exist.");
     }
 
-    docker::volume_rm(engine, &volume, msg_info)?;
+    volume.remove(msg_info)?;
 
     Ok(())
 }
@@ -500,7 +483,8 @@ fn get_cross_containers(
     msg_info: &mut MessageInfo,
 ) -> cross::Result<Vec<String>> {
     use cross::docker::VOLUME_PREFIX;
-    let stdout = docker::subcommand(engine, "ps")
+    let stdout = engine
+        .subcommand("ps")
         .arg("-a")
         .args(["--format", "{{.Names}}: {{.State}}"])
         // handles simple regex: ^ for start of line.
@@ -543,13 +527,13 @@ pub fn remove_all_containers(
 
     let mut commands = vec![];
     if !running.is_empty() {
-        let mut stop = docker::subcommand(engine, "stop");
+        let mut stop = engine.subcommand("stop");
         stop.args(&running);
         commands.push(stop);
     }
 
     if !(stopped.is_empty() && running.is_empty()) {
-        let mut rm = docker::subcommand(engine, "rm");
+        let mut rm = engine.subcommand("rm");
         if force {
             rm.arg("--force");
         }

src/bin/commands/images.rs

@@ -166,7 +166,8 @@ fn get_cross_images(
     msg_info: &mut MessageInfo,
     local: bool,
 ) -> cross::Result<Vec<Image>> {
-    let mut images: BTreeSet<_> = cross::docker::subcommand(engine, "images")
+    let mut images: BTreeSet<_> = engine
+        .subcommand("images")
         .args(["--format", "{{.Repository}}:{{.Tag}} {{.ID}}"])
         .args([
             "--filter",
@@ -177,7 +178,8 @@ fn get_cross_images(
         .map(parse_image)
         .collect();
 
-    let stdout = cross::docker::subcommand(engine, "images")
+    let stdout = engine
+        .subcommand("images")
         .args(["--format", "{{.Repository}}:{{.Tag}} {{.ID}}"])
         .run_and_get_stdout(msg_info)?;
     let ids: Vec<_> = images.iter().map(|i| i.id.to_string()).collect();
@@ -238,7 +240,7 @@ fn get_image_target(
             return Ok(target);
         }
     }
-    let mut command = cross::docker::subcommand(engine, "inspect");
+    let mut command = engine.subcommand("inspect");
     command.args([
         "--format",
         &format!(
@@ -332,7 +334,7 @@ fn remove_images(
     force: bool,
     execute: bool,
 ) -> cross::Result<()> {
-    let mut command = docker::subcommand(engine, "rmi");
+    let mut command = engine.subcommand("rmi");
     if force {
         command.arg("--force");
     }

src/docker/custom.rs

@@ -7,7 +7,7 @@ use crate::shell::MessageInfo;
 use crate::{errors::*, file, CommandExt, ToUtf8};
 use crate::{CargoMetadata, TargetTriple};
 
-use super::{get_image_name, parse_docker_opts, path_hash, ImagePlatform};
+use super::{get_image_name, path_hash, Engine, ImagePlatform};
 
 pub const CROSS_CUSTOM_DOCKERFILE_IMAGE_PREFIX: &str = "localhost/cross-rs/cross-custom-";
 
@@ -71,7 +71,7 @@ impl<'a> Dockerfile<'a> {
         msg_info: &mut MessageInfo,
     ) -> Result<String> {
         let uses_zig = options.cargo_variant.uses_zig();
-        let mut docker_build = docker::command(&options.engine);
+        let mut docker_build = options.engine.command();
         match docker::Engine::has_buildkit() {
             true => docker_build.args(["buildx", "build"]),
             false => docker_build.arg("build"),
@@ -147,7 +147,7 @@ impl<'a> Dockerfile<'a> {
         docker_build.args(["--file".into(), path]);
 
         if let Some(build_opts) = options.config.build_opts() {
-            docker_build.args(parse_docker_opts(&build_opts)?);
+            docker_build.args(Engine::parse_opts(&build_opts)?);
         }
 
         let has_output = options.config.build_opts().map_or(false, |opts| {
@@ -205,7 +205,7 @@ impl<'a> Dockerfile<'a> {
                 "{}{package_name}:{target_triple}-{path_hash}{custom}",
                 CROSS_CUSTOM_DOCKERFILE_IMAGE_PREFIX,
                 package_name = docker_package_name(metadata),
-                path_hash = path_hash(&metadata.workspace_root)?,
+                path_hash = path_hash(&metadata.workspace_root, docker::PATH_HASH_SHORT)?,
                 custom = if matches!(self, Self::File { .. }) {
                     ""
                 } else {

src/docker/local.rs

@@ -30,34 +30,34 @@ pub(crate) fn run(
     let toolchain_dirs = paths.directories.toolchain_directories();
     let package_dirs = paths.directories.package_directories();
 
-    let mut cmd = cargo_safe_command(options.cargo_variant);
+    let mut cmd = options.cargo_variant.safe_command();
     cmd.args(args);
 
-    let mut docker = subcommand(engine, "run");
-    docker_userns(&mut docker);
+    let mut docker = engine.subcommand("run");
+    docker.add_userns();
 
     options
         .image
         .platform
         .specify_platform(&options.engine, &mut docker);
-    docker_envvars(&mut docker, &options, toolchain_dirs, msg_info)?;
+    docker.add_envvars(&options, toolchain_dirs, msg_info)?;
 
-    docker_mount(
-        &mut docker,
+    docker.add_mounts(
         &options,
         &paths,
         |docker, host, absolute| mount(docker, host, absolute, ""),
         |_| {},
         msg_info,
     )?;
 
-    let container = toolchain_dirs.unique_container_identifier(options.target.target())?;
-    docker.args(["--name", &container]);
+    let container_id = toolchain_dirs.unique_container_identifier(options.target.target())?;
+    docker.args(["--name", &container_id]);
     docker.arg("--rm");
 
-    docker_seccomp(&mut docker, engine.kind, &options.target, &paths.metadata)
+    docker
+        .add_seccomp(engine.kind, &options.target, &paths.metadata)
         .wrap_err("when copying seccomp profile")?;
-    docker_user_id(&mut docker, engine.kind);
+    docker.add_user_id(engine.kind);
 
     docker
         .args([
@@ -99,7 +99,7 @@ pub(crate) fn run(
             "-v",
             &format!("{}:/target:z", package_dirs.target().to_utf8()?),
         ]);
-    docker_cwd(&mut docker, &paths)?;
+    docker.add_cwd(&paths)?;
 
     // When running inside NixOS or using Nix packaging we need to add the Nix
     // Store to the running container so it can load the needed binaries.
@@ -124,10 +124,10 @@ pub(crate) fn run(
             .wrap_err("when building custom image")?;
     }
 
-    Container::create(engine.clone(), container)?;
+    ChildContainer::create(engine.clone(), container_id)?;
     let status = docker
         .arg(&image_name)
-        .args(["sh", "-c", &build_command(toolchain_dirs, &cmd)])
+        .add_build_command(toolchain_dirs, &cmd)
         .run_and_get_status(msg_info, false)
         .map_err(Into::into);
 
@@ -138,7 +138,7 @@ pub(crate) fn run(
     // SAFETY: an atomic load.
     let is_terminated = unsafe { crate::errors::TERMINATED.load(Ordering::SeqCst) };
     if !is_terminated {
-        Container::exit_static();
+        ChildContainer::exit_static();
     }
 
     status