Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Various bug fixes for cross. #904

Merged
merged 1 commit into from
Jul 3, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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());
Alexhuszagh marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

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()
}