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

Supporting the nightly flag -Zmultitarget #19

Merged
merged 24 commits into from
Mar 24, 2022
Merged

Conversation

TheOnlyArtz
Copy link
Contributor

First of all I want to say thank you for this incredible tool, it made our work so much easier.

As a part of our pipe line we need to build our program to multiple targets, recently a change has been made to the cargo cli allowing to pass a flag called multitarget which makes it possible to compile to multiple targets all at once.

The proposed changes follow the way the cargo build CLI works, see:
https://github.dev/rust-lang/cargo/blob/06b9d31743210b788b130c8a484c2838afa6fc27/src/bin/cargo/commands/build.rs

    fn arg_target_triple(self, target: &'static str) -> Self {
        self._arg(multi_opt("target", "TRIPLE", target))
    }

pub fn multi_opt(name: &'static str, value_name: &'static str, help: &'static str) -> Arg<'static> {
    opt(name, help)
        .value_name(value_name)
        .multiple_occurrences(true)
}

suggesting that target should infact be a flag which may occur N >= 1 amount of times.

This pull request makes it possible to run the next:
cargo-zigbuild.exe build -Zmultitarget --target x86_64-pc-windows-msvc --target x86_64-unknown-linux-musl --release

src/build.rs Outdated Show resolved Hide resolved
@TheOnlyArtz
Copy link
Contributor Author

TheOnlyArtz commented Mar 23, 2022

I see where the issue arises, the loop in the method which constructs the process command is pretty much useless as it just overrides the previous iteration.

Although, the dynamic environment variables are related to the linker, and they are being bound to a specific target which seems ok.

I can't have an idea why the CI would fail that much really...

What would be a sensible approach for multi target builds? Multi threading? What do you think? @messense

src/build.rs Outdated Show resolved Hide resolved
@messense
Copy link
Member

error: linking with cc failed: exit status: 1

Looks like it's not using our linker wrapper script.

@messense
Copy link
Member

CC_aarch64_unknown_linux_gnu = None

It's not working, maybe you can add some println!s to debug it.

@TheOnlyArtz
Copy link
Contributor Author

It just compiled fine on my end, I don't know why the CI would fail, it also looks for CC_{lower_cased_target} ? It's case sensitive ain't it?

PS C:\Users\amitkatz\Desktop\cargo-zigbuild> cargo r zigbuild --target aarch64-unknown-linux-gnu --manifest-path tests/hello-rustls/Cargo.toml
   Compiling cargo-zigbuild v0.6.7 (C:\Users\amitkatz\Desktop\cargo-zigbuild)
    Finished dev [unoptimized + debuginfo] target(s) in 34.12s
     Running `target\debug\cargo-zigbuild.exe zigbuild --target aarch64-unknown-linux-gnu --manifest-path tests/hello-rustls/Cargo.toml`
CommandEnvs { iter: [(EnvKey { os_string: "CARGO_TARGET_AARCH64_UNKNOWN_LINUX_GNU_LINKER", utf16: [67, 65, 82, 71, 79, 95, 84, 65, 82, 71, 69, 84, 95, 65, 65, 82, 67, 72, 54, 52, 95, 85, 78, 75, 78, 79, 87, 78, 95, 76, 73, 78, 85, 88, 95, 71, 78, 85, 95, 76, 73, 78, 75, 69, 82] }, Some("C:\\Users\\amitkatz\\AppData\\Local\\cargo-zigbuild\\0.6.7\\zigcc-aarch64-unknown-linux-gnu.bat")), (EnvKey { os_string: "CC_AARCH64_UNKNOWN_LINUX_GNU", utf16: [67, 67, 95, 65, 65, 82, 67, 72, 54, 52, 95, 85, 78, 75, 78, 79, 87, 78, 95, 76, 73, 78, 85, 88, 95, 71, 78, 85] }, Some("C:\\Users\\amitkatz\\AppData\\Local\\cargo-zigbuild\\0.6.7\\zigcc-aarch64-unknown-linux-gnu.bat")), (EnvKey { os_string: "CXX_AARCH64_UNKNOWN_LINUX_GNU", utf16: [67, 88, 88, 95, 65, 65, 82, 67, 72, 54, 52, 95, 85, 78, 75, 78, 79, 87, 78, 95,
76, 73, 78, 85, 88, 95, 71, 78, 85] }, Some("C:\\Users\\amitkatz\\AppData\\Local\\cargo-zigbuild\\0.6.7\\zigcxx-aarch64-unknown-linux-gnu.bat"))] }

(DISCARDED THE VERBOSE PRINTING)

   Compiling hello-rustls v0.1.0 (C:\Users\amitkatz\Desktop\cargo-zigbuild\tests\hello-rustls)
    Finished dev [unoptimized + debuginfo] target(s) in 7m 18s

@messense
Copy link
Member

it also looks for CC_{lower_cased_target} ? It's case sensitive ain't it?

Yes, it should be lower case.

src/build.rs Outdated Show resolved Hide resolved
@messense
Copy link
Member

The nightly test failure is unrelated to this change: briansmith/ring#1469

@TheOnlyArtz
Copy link
Contributor Author

The nightly test failure is unrelated to this change: briansmith/ring#1469

What can it be then?

@messense
Copy link
Member

The nightly test failure is unrelated to this change: briansmith/ring#1469

What can it be then?

I don't know, opened rust-lang/rust#95267.

src/build.rs Outdated Show resolved Hide resolved
src/build.rs Show resolved Hide resolved
src/build.rs Outdated Show resolved Hide resolved
src/build.rs Outdated Show resolved Hide resolved
Copy link
Member

@messense messense left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@messense messense merged commit 079f10c into rust-cross:main Mar 24, 2022
@messense
Copy link
Member

Released in v0.7.0: https://crates.io/crates/cargo-zigbuild/0.7.0

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