Skip to content

Commit

Permalink
Various bug fixes for cross.
Browse files Browse the repository at this point in the history
Fixed `cargo metadata` failing with unstable arguments in rustflags if the default toolchain was not nightly. We also log any warnings that occurred if we cannot get metadata. Also fixed a lack of formatting in messages with remote docker support. The mount paths are also correctly processed if using docker-in-docker for mounted volumes.
  • Loading branch information
Alexhuszagh committed Jul 3, 2022
1 parent 00c1de2 commit 2c13e20
Show file tree
Hide file tree
Showing 7 changed files with 81 additions and 34 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ This project adheres to [Semantic Versioning](http://semver.org/).

### Fixed

- #904 - ensure `cargo metadata` works by using the same channel.
- #904 - fixed the path for workspace volumes and passthrough volumes with docker-in-docker.
- #898 - fix the path to the mount root with docker-in-docker if mounting volumes.
- #897 - ensure `target.$(...)` config options override `build` ones when parsing strings and vecs.
- #895 - convert filenames in docker tags to ASCII lowercase and ignore invalid characters
Expand Down
9 changes: 7 additions & 2 deletions src/cargo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use std::process::{Command, ExitStatus};
use crate::cli::Args;
use crate::errors::*;
use crate::extensions::CommandExt;
use crate::shell::MessageInfo;
use crate::shell::{self, MessageInfo};

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum Subcommand {
Expand Down Expand Up @@ -123,6 +123,9 @@ pub fn cargo_metadata_with_args(
msg_info: MessageInfo,
) -> Result<Option<CargoMetadata>> {
let mut command = cargo_command();
if let Some(channel) = args.and_then(|x| x.channel.as_deref()) {
command.arg(format!("+{channel}"));
}
command.arg("metadata").args(&["--format-version", "1"]);
if let Some(cd) = cd {
command.current_dir(cd);
Expand All @@ -142,7 +145,9 @@ pub fn cargo_metadata_with_args(
}
let output = command.run_and_get_output(msg_info)?;
if !output.status.success() {
// TODO: logging
shell::warn("unable to get metadata for package", msg_info)?;
let indented = shell::indent(&String::from_utf8(output.stderr)?, shell::default_ident());
shell::debug(indented, msg_info)?;
return Ok(None);
}
let manifest: Option<CargoMetadata> = serde_json::from_slice(&output.stdout)?;
Expand Down
4 changes: 3 additions & 1 deletion src/docker/local.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ pub(crate) fn run(
docker_in_docker: bool,
cwd: &Path,
) -> Result<ExitStatus> {
let dirs = Directories::create(engine, metadata, cwd, sysroot, docker_in_docker)?;
let mount_finder = MountFinder::create(engine, docker_in_docker)?;
let dirs = Directories::create(&mount_finder, metadata, cwd, sysroot)?;

let mut cmd = cargo_safe_command(uses_xargo);
cmd.args(args);
Expand All @@ -37,6 +38,7 @@ pub(crate) fn run(
let mount_volumes = docker_mount(
&mut docker,
metadata,
&mount_finder,
config,
target,
cwd,
Expand Down
9 changes: 8 additions & 1 deletion src/docker/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use std::process::ExitStatus;

use crate::cargo::CargoMetadata;
use crate::errors::*;
use crate::shell::MessageInfo;
use crate::shell::{self, MessageInfo};
use crate::{Config, Target};

#[allow(clippy::too_many_arguments)] // TODO: refactor
Expand All @@ -28,6 +28,13 @@ pub fn run(
docker_in_docker: bool,
cwd: &Path,
) -> Result<ExitStatus> {
if cfg!(target_os = "windows") && docker_in_docker {
shell::fatal(
"running cross insider a container running windows is currently unsupported",
msg_info,
1,
);
}
if engine.is_remote {
remote::run(
engine,
Expand Down
10 changes: 6 additions & 4 deletions src/docker/remote.rs
Original file line number Diff line number Diff line change
Expand Up @@ -764,7 +764,8 @@ pub(crate) fn run(
docker_in_docker: bool,
cwd: &Path,
) -> Result<ExitStatus> {
let dirs = Directories::create(engine, metadata, cwd, sysroot, docker_in_docker)?;
let mount_finder = MountFinder::create(engine, docker_in_docker)?;
let dirs = Directories::create(&mount_finder, metadata, cwd, sysroot)?;

let mount_prefix = MOUNT_PREFIX;

Expand Down Expand Up @@ -793,16 +794,16 @@ pub(crate) fn run(
let volume = VolumeId::create(engine, &toolchain_id, &container, msg_info)?;
let state = container_state(engine, &container, msg_info)?;
if !state.is_stopped() {
shell::warn("container {container} was running.", msg_info)?;
shell::warn(format_args!("container {container} was running."), msg_info)?;
container_stop(engine, &container, msg_info)?;
}
if state.exists() {
shell::warn("container {container} was exited.", msg_info)?;
shell::warn(format_args!("container {container} was exited."), msg_info)?;
container_rm(engine, &container, msg_info)?;
}
if let VolumeId::Discard(ref id) = volume {
if volume_exists(engine, id, msg_info)? {
shell::warn("temporary volume {container} existed.", msg_info)?;
shell::warn(format_args!("temporary volume {id} existed."), msg_info)?;
volume_rm(engine, id, msg_info)?;
}
}
Expand All @@ -824,6 +825,7 @@ pub(crate) fn run(
let mount_volumes = docker_mount(
&mut docker,
metadata,
&mount_finder,
config,
target,
cwd,
Expand Down
70 changes: 44 additions & 26 deletions src/docker/shared.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,17 +43,11 @@ pub struct Directories {

impl Directories {
pub fn create(
engine: &Engine,
mount_finder: &MountFinder,
metadata: &CargoMetadata,
cwd: &Path,
sysroot: &Path,
docker_in_docker: bool,
) -> Result<Self> {
let mount_finder = if docker_in_docker {
MountFinder::new(docker_read_mount_paths(engine)?)
} else {
MountFinder::default()
};
let home_dir =
home::home_dir().ok_or_else(|| eyre::eyre!("could not find home directory"))?;
let cargo = home::cargo_home()?;
Expand Down Expand Up @@ -89,18 +83,10 @@ impl Directories {
}
#[cfg(not(target_os = "windows"))]
{
// NOTE: host root has already found the mount path
mount_root = host_root.to_utf8()?.to_string();
}
let mount_cwd: String;
#[cfg(target_os = "windows")]
{
// On Windows, we can not mount the directory name directly. Instead, we use wslpath to convert the path to a linux compatible path.
mount_cwd = cwd.as_wslpath()?;
}
#[cfg(not(target_os = "windows"))]
{
mount_cwd = mount_finder.find_mount_path(cwd).to_utf8()?.to_string();
}
let mount_cwd = mount_finder.find_path(cwd, false)?;
let sysroot = mount_finder.find_mount_path(sysroot);

Ok(Directories {
Expand Down Expand Up @@ -163,8 +149,10 @@ pub fn get_package_info(
let cwd = std::env::current_dir()?;
let host_meta = rustc::version_meta()?;
let host = host_meta.host();

let sysroot = rustc::get_sysroot(&host, &target, channel, msg_info)?.1;
let dirs = Directories::create(engine, &metadata, &cwd, &sysroot, docker_in_docker)?;
let mount_finder = MountFinder::create(engine, docker_in_docker)?;
let dirs = Directories::create(&mount_finder, &metadata, &cwd, &sysroot)?;

Ok((target, metadata, dirs))
}
Expand Down Expand Up @@ -251,9 +239,9 @@ fn add_cargo_configuration_envvars(docker: &mut Command) {
}
}

pub(crate) fn mount(docker: &mut Command, val: &Path, prefix: &str) -> Result<String> {
let host_path = file::canonicalize(val)?;
let mount_path = canonicalize_mount_path(&host_path)?;
// NOTE: host path must be canonical
pub(crate) fn mount(docker: &mut Command, host_path: &Path, prefix: &str) -> Result<String> {
let mount_path = canonicalize_mount_path(host_path)?;
docker.args(&[
"-v",
&format!("{}:{prefix}{}", host_path.to_utf8()?, mount_path),
Expand Down Expand Up @@ -338,6 +326,7 @@ pub(crate) fn docker_cwd(
pub(crate) fn docker_mount(
docker: &mut Command,
metadata: &CargoMetadata,
mount_finder: &MountFinder,
config: &Config,
target: &Target,
cwd: &Path,
Expand All @@ -359,15 +348,19 @@ pub(crate) fn docker_mount(
};

if let Ok(val) = value {
let mount_path = mount_cb(docker, val.as_ref())?;
docker.args(&["-e", &format!("{}={}", var, mount_path)]);
let canonical_val = file::canonicalize(&val)?;
let host_path = mount_finder.find_path(&canonical_val, true)?;
let mount_path = mount_cb(docker, host_path.as_ref())?;
docker.args(&["-e", &format!("{}={}", host_path, mount_path)]);
store_cb((val, mount_path));
mount_volumes = true;
}
}

for path in metadata.path_dependencies() {
let mount_path = mount_cb(docker, path)?;
let canonical_path = file::canonicalize(path)?;
let host_path = mount_finder.find_path(&canonical_path, true)?;
let mount_path = mount_cb(docker, host_path.as_ref())?;
store_cb((path.to_utf8()?.to_string(), mount_path));
mount_volumes = true;
}
Expand Down Expand Up @@ -614,7 +607,7 @@ fn dockerinfo_parse_user_mounts(info: &serde_json::Value) -> Vec<MountDetail> {
}

#[derive(Debug, Default)]
struct MountFinder {
pub struct MountFinder {
mounts: Vec<MountDetail>,
}

Expand All @@ -636,7 +629,15 @@ impl MountFinder {
MountFinder { mounts }
}

fn find_mount_path(&self, path: impl AsRef<Path>) -> PathBuf {
pub fn create(engine: &Engine, docker_in_docker: bool) -> Result<MountFinder> {
Ok(if docker_in_docker {
MountFinder::new(docker_read_mount_paths(engine)?)
} else {
MountFinder::default()
})
}

pub fn find_mount_path(&self, path: impl AsRef<Path>) -> PathBuf {
let path = path.as_ref();

for info in &self.mounts {
Expand All @@ -647,6 +648,23 @@ impl MountFinder {

path.to_path_buf()
}

#[allow(unused_variables, clippy::needless_return)]
fn find_path(&self, path: &Path, host: bool) -> Result<String> {
#[cfg(target_os = "windows")]
{
// On Windows, we can not mount the directory name directly. Instead, we use wslpath to convert the path to a linux compatible path.
if host {
return Ok(path.to_utf8()?.to_string());
} else {
return path.as_wslpath();
}
}
#[cfg(not(target_os = "windows"))]
{
return Ok(self.find_mount_path(path).to_utf8()?.to_string());
}
}
}

fn path_digest(path: &Path) -> Result<const_sha1::Digest> {
Expand Down
11 changes: 11 additions & 0 deletions src/shell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -308,3 +308,14 @@ impl Stream for io::Stderr {
const TTY: atty::Stream = atty::Stream::Stderr;
const OWO: owo_colors::Stream = owo_colors::Stream::Stderr;
}

pub fn default_ident() -> usize {
cross_prefix!("").len()
}

pub fn indent(message: &str, spaces: usize) -> String {
message
.lines()
.map(|s| format!("{:spaces$}{s}", ""))
.collect()
}

0 comments on commit 2c13e20

Please sign in to comment.