-
Notifications
You must be signed in to change notification settings - Fork 382
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 usage of other toolchains in images #817
Conversation
68e91a1
to
d303111
Compare
b44de01
to
1efbfba
Compare
179149f
to
21e933a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've mostly review it but there's a few pressing changes I think need to be resolved, so I'll do a full review once those are addressed.
cbbe5be
to
bcac46d
Compare
need to add documentation and basic test, ill work on that integration can go in #873 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, let me know if you need a final review or want to check specific cases.
Went over the changes since the last commit, everything looks good. I tested it and it works on:
It does not, however, work with This has one more issue: in some cases, the following will partially succeed: $ podman version -f '{{ .Server.Os }},,,{{ .Server.Arch }}'
linux,,,Error: template: version:1:15: executing "version" at <.Arch>: can't evaluate field Arch in type *define.Version
$ echo $?
125 This means we need to check the status result as well. |
Seems podman build only supports --output since 2022-05-04 I think we could just remove |
I've added the following diff which handles Podman returning partial data with a status error, better error handling on the fallback case, and not using diff --git a/src/docker/custom.rs b/src/docker/custom.rs
index 89eb189..5e82910 100644
--- a/src/docker/custom.rs
+++ b/src/docker/custom.rs
@@ -2,7 +2,7 @@ use std::io::Write;
use std::path::PathBuf;
use std::str::FromStr;
-use crate::docker::{DockerOptions, DockerPaths};
+use crate::docker::{DockerOptions, DockerPaths, EngineType};
use crate::shell::MessageInfo;
use crate::{docker, CargoMetadata, TargetTriple};
use crate::{errors::*, file, CommandExt, ToUtf8};
@@ -147,7 +147,7 @@ impl<'a> Dockerfile<'a> {
let has_output = options.config.build_opts().map_or(false, |opts| {
opts.contains("--load") || opts.contains("--output")
});
- if !has_output {
+ if options.engine.kind == EngineType::Docker && !has_output {
docker_build.args(&["--output", "type=docker"]);
};
diff --git a/src/docker/engine.rs b/src/docker/engine.rs
index 9bc37c4..d7095dd 100644
--- a/src/docker/engine.rs
+++ b/src/docker/engine.rs
@@ -3,9 +3,9 @@ use std::path::{Path, PathBuf};
use std::process::Command;
use crate::config::bool_from_envvar;
+use crate::errors::*;
use crate::extensions::CommandExt;
use crate::shell::MessageInfo;
-use crate::{errors::*, OutputExt};
use super::{Architecture, ContainerOs};
@@ -20,6 +20,13 @@ pub enum EngineType {
Other,
}
+impl EngineType {
+ #[must_use]
+ pub fn is_podman(&self) -> bool {
+ matches!(self, EngineType::Podman | EngineType::PodmanRemote)
+ }
+}
+
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct Engine {
pub kind: EngineType,
@@ -88,6 +95,11 @@ impl Engine {
)
}
+ #[must_use]
+ pub fn is_podman(&self) -> bool {
+ self.kind.is_podman()
+ }
+
#[must_use]
pub fn is_remote() -> bool {
env::var("CROSS_REMOTE")
@@ -117,82 +129,71 @@ fn get_engine_info(
EngineType::Other
};
- let mut cmd = Command::new(ce);
- cmd.args(&["version", "-f", "{{ .Server.Os }},,,{{ .Server.Arch }}"]);
-
- let out = cmd.run_and_get_output(msg_info)?;
-
- let stdout = out.stdout()?.to_lowercase();
-
- let osarch = stdout
- .trim()
- .split_once(",,,")
- .map(|(os, arch)| -> Result<_> { Ok((ContainerOs::new(os)?, Architecture::new(arch)?)) })
- .transpose();
-
- let osarch = match (kind, osarch) {
- (_, Ok(Some(osarch))) => Some(osarch),
- (EngineType::PodmanRemote | EngineType::Podman, Ok(None)) => get_podman_info(ce, msg_info)?,
- (_, Err(e)) => {
- return Err(e.wrap_err(format!(
- "command `{}` returned unexpected data",
- cmd.command_pretty(msg_info, |_| false)
- )));
- }
- (EngineType::Docker | EngineType::Other, Ok(None)) => None,
- };
-
- let osarch = if osarch.is_some() {
- osarch
- } else if !out.status.success() {
- get_custom_info(ce, msg_info).with_error(|| {
- cmd.status_result(msg_info, out.status, Some(&out))
- .expect_err("status_result should error")
- })?
- } else {
- get_custom_info(ce, msg_info)?
+ // this can fail: podman can give partial output
+ // linux,,,Error: template: version:1:15: executing "version" at <.Arch>:
+ // can't evaluate field Arch in type *define.Version
+ let osarch = engine_info(
+ ce,
+ &["version", "-f", "{{ .Server.Os }},,,{{ .Server.Arch }}"],
+ ",,,",
+ msg_info,
+ )
+ .unwrap_or(None);
+
+ let osarch = match osarch {
+ Some(osarch) => Some(osarch),
+ None if kind.is_podman() => get_podman_info(ce, msg_info)?,
+ None => get_custom_info(ce, msg_info)?,
};
let (os, arch) = osarch.map_or(<_>::default(), |(os, arch)| (Some(os), Some(arch)));
Ok((kind, arch, os))
}
-fn get_podman_info(
+fn engine_info(
ce: &Path,
+ args: &[&str],
+ sep: &str,
msg_info: &mut MessageInfo,
) -> Result<Option<(ContainerOs, Architecture)>> {
let mut cmd = Command::new(ce);
- cmd.args(&["info", "-f", "{{ .Version.OsArch }}"]);
+ cmd.args(args);
cmd.run_and_get_stdout(msg_info)
.map(|s| {
s.to_lowercase()
.trim()
- .split_once('/')
+ .split_once(sep)
.map(|(os, arch)| -> Result<_> {
Ok((ContainerOs::new(os)?, Architecture::new(arch)?))
})
})
+ .wrap_err_with(|| {
+ format!(
+ "command `{}` returned unexpected data",
+ cmd.command_pretty(msg_info, |_| false)
+ )
+ })
.wrap_err("could not determine os and architecture of vm")?
.transpose()
}
+fn get_podman_info(
+ ce: &Path,
+ msg_info: &mut MessageInfo,
+) -> Result<Option<(ContainerOs, Architecture)>> {
+ engine_info(ce, &["info", "-f", "{{ .Version.OsArch }}"], "/", msg_info)
+}
+
fn get_custom_info(
ce: &Path,
msg_info: &mut MessageInfo,
) -> Result<Option<(ContainerOs, Architecture)>> {
- let mut cmd = Command::new(ce);
- cmd.args(&["info", "-f", "{{ .Client.Os }},,,{{ .Client.Arch }}"]);
- cmd.run_and_get_stdout(msg_info)
- .map(|s| {
- s.to_lowercase()
- .trim()
- .split_once(",,,")
- .map(|(os, arch)| -> Result<_> {
- Ok((ContainerOs::new(os)?, Architecture::new(arch)?))
- })
- })
- .unwrap_or_default()
- .transpose()
+ engine_info(
+ ce,
+ &["info", "-f", "{{ .Client.Os }},,,{{ .Client.Arch }}"],
+ ",,,",
+ msg_info,
+ )
}
pub fn get_container_engine() -> Result<PathBuf, which::Error> {
diff --git a/xtask/src/build_docker_image.rs b/xtask/src/build_docker_image.rs
index 220dcc0..034ccde 100644
--- a/xtask/src/build_docker_image.rs
+++ b/xtask/src/build_docker_image.rs
@@ -183,7 +183,7 @@ pub fn build_docker_image(
if push {
docker_build.arg("--push");
- } else if no_output {
+ } else if engine.kind == docker::EngineType::Docker && no_output {
docker_build.args(&["--output", "type=tar,dest=/dev/null"]);
} else {
docker_build.arg("--load");
|
@Alexhuszagh I've applied the patch but with more error handling. Hopefully I didn't break any logic that was implied by the patch. now returns on error (with artificial flag
|
b2a192c
to
86a550d
Compare
lgtm, going to test briefly and then merge |
bors r+ |
817: Enable usage of other toolchains in images r=Alexhuszagh a=Emilgardis Makes the following work ```toml # Cross.toml [build] default-target = "x86_64-unknown-linux-musl" [target."x86_64-unknown-linux-musl"] image.name = "alpine:edge" image.toolchain = ["x86_64-unknown-linux-musl"] pre-build = ["apk add --no-cache gcc musl-dev"] ``` ```rust // src/main.rs fn main() { println!("Hello, world!"); } ``` ``` > cross run [..snip] Finished dev [unoptimized + debuginfo] target(s) in 0.24s Running `/target/x86_64-unknown-linux-musl/debug/cross-minimal` Hello, world! ``` and also ```toml [target.x86_64-unknown-linux-gnu] pre-build = [ "apt-get update && apt-get install -y libc6 g++-x86-64-linux-gnu libc6-dev-amd64-cross", ] [target.x86_64-unknown-linux-gnu.env] passthrough = [ "CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_LINKER=x86_64-linux-gnu-gcc", "CC_x86_64_unknown_linux_gnu=x86_64-linux-gnu-gcc", "CXX_x86_64_unknown_linux_gnu=x86_64-linux-gnu-g++", ] [target.x86_64-unknown-linux-gnu.image] name = "ubuntu:20.04" toolchain = ["aarch64-unknown-linux-gnu"] ``` ``` cross build --target x86_64-unknown-linux-gnu -v + cargo metadata --format-version 1 --filter-platform x86_64-unknown-linux-gnu + rustc --print sysroot + /usr/local/bin/docker + /usr/local/bin/docker version '-f "{{ .Server.Os }},{{ .Server.Arch }}"' + rustup --verbose toolchain list + rustup --verbose target list --toolchain stable-aarch64-unknown-linux-gnu + rustup --verbose component list --toolchain stable-aarch64-unknown-linux-gnu + "/Users/emil/workspace/cross-tests/basic" /usr/local/bin/docker build --platform linux/arm64 --label 'org.cross-rs.for-cross-target=x86_64-unknown-linux-gnu' --label 'org.cross-rs.runs-with=aarch64-unknown-linux-gnu' --label 'org.cross-rs.workspace_root=/Users/emil/workspace/cross-tests/basic' --tag cross-custom-basic:x86_64-unknown-linux-gnu-50dfa-pre-build --build-arg 'CROSS_CMD=apt-get update && apt-get install -y libc6 g++-x86-64-linux-gnu libc6-dev-amd64-cross' --build-arg 'CROSS_DEB_ARCH=amd64' --file /Users/emil/workspace/cross-tests/basic/target/x86_64-unknown-linux-gnu/Dockerfile.x86_64-unknown-linux-gnu-custom . [+] Building 10.0s (6/6) FINISHED => [internal] load build definition from Dockerfile.x86_64-unknown-linux-gnu-custom 0.0s => => transferring dockerfile: 166B 0.0s => [internal] load .dockerignore 0.0s => => transferring context: 2B 0.0s => [internal] load metadata for docker.io/library/ubuntu:20.04 0.6s => CACHED [1/2] FROM docker.io/library/ubuntu:20.04@sha256:fd92c36d3cb9b1d027c4d2a72c6bf0125da82425fc2ca 0.0s => [2/2] RUN eval "apt-get update && apt-get install -y libc6 g++-x86-64-linux-gnu libc6-dev-amd64-cross 8.8s => exporting to image 0.5s => => exporting layers 0.5s => => writing image sha256:92f3cf51cc16c7b176c9bc09ce28ef83c029881e9c5017589eb8372bf6fff8d3 0.0s => => naming to docker.io/library/cross-custom-basic:x86_64-unknown-linux-gnu-50dfa-pre-build 0.0s + /usr/local/bin/docker run --userns host --platform linux/arm64 -e 'CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_LINKER=x86_64-linux-gnu-gcc' -e 'CC_x86_64_unknown_linux_gnu=x86_64-linux-gnu-gcc' -e 'CXX_x86_64_unknown_linux_gnu=x86_64-linux-gnu-g++' -e 'PKG_CONFIG_ALLOW_CROSS=1' -e 'XARGO_HOME=/xargo' -e 'CARGO_HOME=/cargo' -e 'CARGO_TARGET_DIR=/target' -e 'CROSS_RUNNER=' -e TERM -e 'USER=emil' --rm --user 501:20 -v /Users/emil/.xargo:/xargo:Z -v /Users/emil/.cargo:/cargo:Z -v /cargo/bin -v /Users/emil/workspace/cross-tests/basic:/project:Z -v /Users/emil/.rustup/toolchains/stable-aarch64-unknown-linux-gnu:/rust:Z,ro -v /Users/emil/workspace/cross-tests/basic/target:/target:Z -w /project -i -t cross-custom-basic:x86_64-unknown-linux-gnu-50dfa-pre-build sh -c 'PATH=$PATH:/rust/bin cargo build --target x86_64-unknown-linux-gnu -v' Fresh basic v0.1.0 (/project) Finished dev [unoptimized + debuginfo] target(s) in 0.47s basic on master [?] is 📦 v0.1.0 via 🦀 v1.61.0 took 12s ❯ file target/x86_64-unknown-linux-gnu/debug/basic target/x86_64-unknown-linux-gnu/debug/basic: ELF 64-bit LSB pie executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, BuildID[sha1]=5716ca6c6514e255e9a29eeeeac3d9cb4fb43c0a, for GNU/Linux 3.2.0, with debug_info, not stripped basic on master [?] is 📦 v0.1.0 via 🦀 v1.61.0 ❯ uname -a Darwin Emils-MacBook-Pro.local 21.5.0 Darwin Kernel Version 21.5.0: Tue Apr 26 21:08:37 PDT 2022; root:xnu-8020.121.3~4/RELEASE_ARM64_T6000 arm64 ``` also makes `cross +nightly-2022-01-01` possible. this is a requirement for #751 Co-authored-by: Emil Gardström <[email protected]>
Build failed: |
also don't block integration tests on shellcheck
bumps minimal versions
86a550d
to
583f997
Compare
bors r=@Alexhuszagh |
Build succeeded: |
Makes the following work
and also
also makes
cross +nightly-2022-01-01
possible.this is a requirement for #751