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

enable cargo workspace and path dependencies to work seamlessly #684

Merged
merged 5 commits into from
Jun 7, 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 @@ -16,6 +16,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
- #543 - Added environment variables to control the UID and GID in the container
- #524 - docker: Add Nix Store volume support
- Added support for mounting volumes.
- #684 - Enable cargo workspaces to work from any path in the workspace, and make path dependencies mount seamlessly.

### Changed

Expand Down Expand Up @@ -45,6 +46,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
- Re-enabled `sparc64-unknown-linux-gnu` image
- #582 - Added `libprocstat.so` to FreeBSD images
- #665 - when not using [env.volumes](https://github.com/cross-rs/cross#mounting-volumes-into-the-build-environment), mount project in /project
- #494 - Parse Cargo's --manifest-path option to determine mounted docker root


### Removed
Expand Down
6 changes: 0 additions & 6 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -362,12 +362,6 @@ $ QEMU_STRACE=1 cross run --target aarch64-unknown-linux-gnu
9 exit_group(0)
```

## Caveats

- path dependencies (in Cargo.toml) that point outside the Cargo project won't
work because `cross` use docker containers only mounts the Cargo project so
the container doesn't have access to the rest of the filesystem.

## Minimum Supported Rust Version (MSRV)

This crate is guaranteed to compile on stable Rust 1.58.1 and up. It *might*
Expand Down
23 changes: 23 additions & 0 deletions ci/test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,14 @@ function retry {
return ${exit_code}
}

workspace_test() {
# "${@}" is an unbound variable for bash 3.2, which is the installed version on macOS
cross build --target "${TARGET}" --workspace "$@"
cross run --target "${TARGET}" -p binary "$@"
cross run --target "${TARGET}" --bin dependencies \
--features=dependencies "$@"
}

main() {
local td=

Expand Down Expand Up @@ -135,6 +143,21 @@ EOF
popd

rm -rf "${td}"
td=$(mktemp -d)
git clone \
--depth 1 \
--recursive \
https://github.com/cross-rs/test-workspace "${td}"

pushd "${td}"
TARGET="${TARGET}" workspace_test --manifest-path="./workspace/Cargo.toml"
pushd "workspace"
TARGET="${TARGET}" workspace_test
pushd "binary"
cross run --target "${TARGET}"
Alexhuszagh marked this conversation as resolved.
Show resolved Hide resolved
popd
popd
popd
;;
esac

Expand Down
99 changes: 76 additions & 23 deletions src/cargo.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
use serde::Deserialize;
use std::path::{Path, PathBuf};
use std::process::{Command, ExitStatus};
use std::{env, fs};

use crate::cli::Args;
use crate::errors::*;
use crate::extensions::CommandExt;

Expand Down Expand Up @@ -52,38 +53,90 @@ impl<'a> From<&'a str> for Subcommand {
}
}

#[derive(Debug)]
pub struct Root {
path: PathBuf,
#[derive(Debug, Deserialize)]
pub struct CargoMetadata {
pub workspace_root: PathBuf,
pub target_directory: PathBuf,
pub packages: Vec<Package>,
pub workspace_members: Vec<String>,
}

impl Root {
pub fn path(&self) -> &Path {
&self.path
impl CargoMetadata {
fn non_workspace_members(&self) -> impl Iterator<Item = &Package> {
self.packages
.iter()
.filter(|p| !self.workspace_members.iter().any(|m| m == &p.id))
}
}

/// Cargo project root
pub fn root() -> Result<Option<Root>> {
let cd = env::current_dir().wrap_err("couldn't get current directory")?;
pub fn path_dependencies(&self) -> impl Iterator<Item = &Path> {
// TODO: Also filter out things that are in workspace, but not a workspace member
self.non_workspace_members().filter_map(|p| p.crate_path())
}
Comment on lines +72 to +74
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this todo is not super important, but probably good to have since it minifies the mounts needed

}

let mut dir = &*cd;
loop {
let toml = dir.join("Cargo.toml");
#[derive(Debug, Deserialize)]
pub struct Package {
id: String,
manifest_path: PathBuf,
source: Option<String>,
}

if fs::metadata(&toml).is_ok() {
return Ok(Some(Root {
path: dir.to_owned(),
}));
impl Package {
/// Returns the absolute path to the packages manifest "folder"
fn crate_path(&self) -> Option<&Path> {
// when source is none, this package is a path dependency or a workspace member
if self.source.is_none() {
self.manifest_path.parent()
} else {
None
}
}
}

match dir.parent() {
Some(p) => dir = p,
None => break,
/// Cargo metadata with specific invocation
pub fn cargo_metadata_with_args(
cd: Option<&Path>,
args: Option<&Args>,
verbose: bool,
) -> Result<Option<CargoMetadata>> {
let mut command = std::process::Command::new(
std::env::var("CARGO")
.ok()
.unwrap_or_else(|| "cargo".to_string()),
);
command.arg("metadata").arg("--format-version=1");
if let Some(cd) = cd {
command.current_dir(cd);
}
if let Some(config) = args {
if let Some(ref manifest_path) = config.manifest_path {
command.args(["--manifest-path".as_ref(), manifest_path.as_os_str()]);
}
} else {
command.arg("--no-deps");
}

Ok(None)
if let Some(target) = args.and_then(|a| a.target.as_ref()) {
command.args(["--filter-platform", target.triple()]);
}
Emilgardis marked this conversation as resolved.
Show resolved Hide resolved
if let Some(features) = args.map(|a| &a.features).filter(|v| !v.is_empty()) {
command.args([String::from("--features"), features.join(",")]);
}
let output = command.run_and_get_output(verbose)?;
if !output.status.success() {
// TODO: logging
return Ok(None);
}
let manifest: Option<CargoMetadata> = serde_json::from_slice(&output.stdout)?;
manifest
.map(|m| -> Result<_> {
Ok(CargoMetadata {
target_directory: args
.and_then(|a| a.target_dir.clone())
.unwrap_or(m.target_directory),
..m
})
})
.transpose()
}

/// Pass-through mode
Expand Down
31 changes: 30 additions & 1 deletion src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,11 @@ pub struct Args {
pub subcommand: Option<Subcommand>,
pub channel: Option<String>,
pub target: Option<Target>,
pub features: Vec<String>,
pub target_dir: Option<PathBuf>,
pub docker_in_docker: bool,
pub enable_doctests: bool,
pub manifest_path: Option<PathBuf>,
}

// Fix for issue #581. target_dir must be absolute.
Expand Down Expand Up @@ -72,6 +74,8 @@ pub fn fmt_subcommands(stdout: &str) {
pub fn parse(target_list: &TargetList) -> Result<Args> {
let mut channel = None;
let mut target = None;
let mut features = Vec::new();
let mut manifest_path: Option<PathBuf> = None;
let mut target_dir = None;
let mut sc = None;
let mut all: Vec<String> = Vec::new();
Expand All @@ -82,7 +86,21 @@ pub fn parse(target_list: &TargetList) -> Result<Args> {
if arg.is_empty() {
continue;
}
if let ("+", ch) = arg.split_at(1) {
if arg == "--manifest-path" {
all.push(arg);
if let Some(m) = args.next() {
let p = PathBuf::from(&m);
all.push(m);
manifest_path = env::current_dir().ok().map(|cwd| cwd.join(p));
}
} else if arg.starts_with("--manifest-path=") {
manifest_path = arg
.split_once('=')
.map(|x| x.1)
.map(PathBuf::from)
.and_then(|p| env::current_dir().ok().map(|cwd| cwd.join(p)));
all.push(arg);
} else if let ("+", ch) = arg.split_at(1) {
Comment on lines +89 to +103
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should add a utility to do this operation easier and in a standardized way

channel = Some(ch.to_string());
} else if arg == "--target" {
all.push(arg);
Expand All @@ -95,6 +113,15 @@ pub fn parse(target_list: &TargetList) -> Result<Args> {
.split_once('=')
.map(|(_, t)| Target::from(t, target_list));
all.push(arg);
} else if arg == "--features" {
all.push(arg);
if let Some(t) = args.next() {
features.push(t.clone());
all.push(t);
}
} else if arg.starts_with("--features=") {
features.extend(arg.split_once('=').map(|(_, t)| t.to_owned()));
all.push(arg);
} else if arg == "--target-dir" {
all.push(arg);
if let Some(td) = args.next() {
Expand Down Expand Up @@ -128,8 +155,10 @@ pub fn parse(target_list: &TargetList) -> Result<Args> {
subcommand: sc,
channel,
target,
features,
target_dir,
docker_in_docker,
enable_doctests,
manifest_path,
})
}
Loading