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

Initial implementation #1

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Initial implementation #1

wants to merge 6 commits into from

Conversation

vadorovsky
Copy link
Member

@vadorovsky vadorovsky commented Oct 23, 2024

cross-llvm is a tool for easy cross (but also native) builds using LLVM (clang, Rust).

It consists of:

  • Container images which provide the toolchains for each target.
  • A CLI tool, which is a wrapper about a container engine (Docker, podman) and allows easy execution of the toolchain.

Provided container images can be used:

@vadorovsky vadorovsky force-pushed the initial branch 6 times, most recently from 4dee329 to 019ff72 Compare October 24, 2024 17:22
cross-llvm is a tool for easy cross (but also native) builds using LLVM
(clang, Rust).

It consists of:

- Container images which provide the toolchains for each target.
- A CLI tool, which is a wrapper about a container engine (Docker,
  podman) and allows easy execution of the toolchain.

Provided container images can be used with https://github.com/cross-rs/cross
Cross wrappers for musl containers are already provided by crossdev[0].
To achieve the save effect on Debian containers, create similar
wrappers.
@vadorovsky vadorovsky marked this pull request as ready for review October 27, 2024 21:53
@vadorovsky vadorovsky marked this pull request as draft October 27, 2024 21:53
@vadorovsky vadorovsky marked this pull request as ready for review October 28, 2024 06:18
uuid = { version = "1.10", features = ["v4"] }
which = "6.0"

cross-llvm = { path = "cross-llvm" }
Copy link

Choose a reason for hiding this comment

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

why list this here?

uuid = { workspace = true }
which = { workspace = true }

cross-llvm = { workspace = true }
Copy link

Choose a reason for hiding this comment

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

use the path?

#[error("containerized builds are not supported for target {0}")]
UnsupportedTarget(String),
#[error("failed to build a container image")]
ContainerImageBuild,
Copy link

Choose a reason for hiding this comment

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

shouldn't this include a source error?

ditto below

container_engine: Option<ContainerEngine>,

/// Do not use existing cached images for the container build. Build from
/// the start with a new set of cached layers.
Copy link

Choose a reason for hiding this comment

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

inconsistent punctuation

#[arg(short, long = "tag", name = "tag")]
tags: Vec<OsString>,

/// Target triple (optional)
Copy link

Choose a reason for hiding this comment

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

why do you need to say optional? doesn't clap do the right thing based on the type?


pub fn clang(args: RunArgs) -> anyhow::Result<()> {
let triple: Triple = match args.target {
Some(ref target) => target.clone().into(),
Copy link

Choose a reason for hiding this comment

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

why do you need to clone it? you have it by value. looks like it's because run_container wants the same argument; split the struct and use #[arg(flatten)]

let mut container = Command::new(container_engine.to_string());
container
.args([
OsStr::new("run"),
Copy link

Choose a reason for hiding this comment

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

same thing here, overusing OsStr::new imo. you can mostly use &str literals

])
.args(cmd_args)
.args(cmd)
.stdin(Stdio::inherit())
Copy link

Choose a reason for hiding this comment

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

why inherit stdin? again i think we should capture stdio here for a semblance of discipline


#[derive(Debug, Error)]
pub enum CrossLlvmError {
#[error("no supported container engine (docker, podman) was found")]
Copy link

Choose a reason for hiding this comment

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

odd to enumerate the options at a distance


#[derive(Parser)]
pub struct RunArgs {
/// Container engine (if not provided, is going to be autodetected)
Copy link

Choose a reason for hiding this comment

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

getting dejavu here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants