Multi target support #212

UebelAndre · 2020-09-04T21:24:23Z

Similar to #54 this pull request adds support for generating Bazel targets for multiple platforms. This is still kinda WIP but I wanted to share. I'll update this description as the feature evolves but for now am looking for any insight on what may be missing for the initial release of this feature.

Output BUILD file:

"""
@generated
cargo-raze crate build file.

DO NOT EDIT! Replaced on runs of cargo-raze
"""

# buildifier: disable=load
load(
    "@io_bazel_rules_rust//rust:rust.bzl",
    "rust_binary",
    "rust_library",
    "rust_test",
)

# buildifier: disable=load
load("@bazel_skylib//lib:selects.bzl", "selects")

package(default_visibility = [
    # Public for visibility by "@raze__crate__version//" targets.
    #
    # Prefer access through "//cbindgen/raze", which limits external
    # visibility to explicit Cargo.toml dependencies.
    "//visibility:public",
])

licenses([
    "notice",  # MIT from expression "MIT"
])

# Generated targets
# Unsupported target "atty" with type "example" omitted

# buildifier: leave-alone
rust_library(
    name = "atty",
    crate_type = "lib",
    deps = [
    ] + selects.with_or({
        # cfg(unix)
        (
            "@io_bazel_rules_rust//rust/platform:aarch64-apple-ios",
            "@io_bazel_rules_rust//rust/platform:aarch64-linux-android",
            "@io_bazel_rules_rust//rust/platform:aarch64-unknown-linux-gnu",
            "@io_bazel_rules_rust//rust/platform:arm-unknown-linux-gnueabi",
            "@io_bazel_rules_rust//rust/platform:i686-apple-darwin",
            "@io_bazel_rules_rust//rust/platform:i686-linux-android",
            "@io_bazel_rules_rust//rust/platform:i686-unknown-freebsd",
            "@io_bazel_rules_rust//rust/platform:i686-unknown-linux-gnu",
            "@io_bazel_rules_rust//rust/platform:powerpc-unknown-linux-gnu",
            "@io_bazel_rules_rust//rust/platform:s390x-unknown-linux-gnu",
            "@io_bazel_rules_rust//rust/platform:x86_64-apple-darwin",
            "@io_bazel_rules_rust//rust/platform:x86_64-apple-ios",
            "@io_bazel_rules_rust//rust/platform:x86_64-linux-android",
            "@io_bazel_rules_rust//rust/platform:x86_64-unknown-freebsd",
            "@io_bazel_rules_rust//rust/platform:x86_64-unknown-linux-gnu",
        ): [
            "@rules_rust_cbindgen__libc__0_2_76//:libc",
        ],
        "//conditions:default": [],
    }) + selects.with_or({
        # cfg(windows)
        (
            "@io_bazel_rules_rust//rust/platform:i686-pc-windows-gnu",
            "@io_bazel_rules_rust//rust/platform:x86_64-pc-windows-gnu",
        ): [
            "@rules_rust_cbindgen__winapi__0_3_9//:winapi",
        ],
        "//conditions:default": [],
    }),
    srcs = glob(["**/*.rs"]),
    crate_root = "src/lib.rs",
    edition = "2015",
    rustc_flags = [
        "--cap-lints=allow",
    ],
    version = "0.2.14",
    tags = [
        "cargo-raze",
        "manual",
    ],
    crate_features = [
    ],
)

Note that the deps section can be filtered down to a whitelist of platform triples using the new targets setting.

targets = [
    "aarch64-unknown-linux-gnu",
    "i686-apple-darwin",
    "i686-pc-windows-gnu",
    "i686-unknown-linux-gnu",
    "powerpc-unknown-linux-gnu",
    "x86_64-apple-darwin",
    "x86_64-pc-windows-gnu",
    "x86_64-unknown-linux-gnu",
]

The list above creates the following diff in the output BUILD file.

diff --git a/cbindgen/raze/remote/atty-0.2.14.BUILD.bazel b/cbindgen/raze/remote/atty-0.2.14.BUILD.bazel
index dfcf387..6002194 100644
--- a/cbindgen/raze/remote/atty-0.2.14.BUILD.bazel
+++ b/cbindgen/raze/remote/atty-0.2.14.BUILD.bazel
@@ -48,21 +48,12 @@ rust_library(
     }) + selects.with_or({
         # cfg(unix)
         (
+            "@io_bazel_rules_rust//rust/platform:aarch64-unknown-linux-gnu",
             "@io_bazel_rules_rust//rust/platform:i686-apple-darwin",
             "@io_bazel_rules_rust//rust/platform:i686-unknown-linux-gnu",
+            "@io_bazel_rules_rust//rust/platform:powerpc-unknown-linux-gnu",
             "@io_bazel_rules_rust//rust/platform:x86_64-apple-darwin",
             "@io_bazel_rules_rust//rust/platform:x86_64-unknown-linux-gnu",
-            "@io_bazel_rules_rust//rust/platform:aarch64-apple-ios",
-            "@io_bazel_rules_rust//rust/platform:aarch64-linux-android",
-            "@io_bazel_rules_rust//rust/platform:aarch64-unknown-linux-gnu",
-            "@io_bazel_rules_rust//rust/platform:powerpc-unknown-linux-gnu",
-            "@io_bazel_rules_rust//rust/platform:arm-unknown-linux-gnueabi",
-            "@io_bazel_rules_rust//rust/platform:s390x-unknown-linux-gnu",
-            "@io_bazel_rules_rust//rust/platform:i686-linux-android",
-            "@io_bazel_rules_rust//rust/platform:i686-unknown-freebsd",
-            "@io_bazel_rules_rust//rust/platform:x86_64-apple-ios",
-            "@io_bazel_rules_rust//rust/platform:x86_64-linux-android",
-            "@io_bazel_rules_rust//rust/platform:x86_64-unknown-freebsd",
         ): [
             "@rules_rust_cbindgen__libc__0_2_76//:libc",
         ],

UebelAndre · 2020-09-13T15:25:50Z

@damienmg Hey, I think this is ready for review. I've been using the changes in this branch to work on bazelbuild/rules_rust#392 and was able to easily get generate targets for cbindgen that successfully built on the Linux, MacOS, and Windows CI builds.

I tried a couple of ways to try and come up with some logic to transcribe the cfgs into a matching select statement but wasn't really able to come up with a clean solution. I think this should be easier if bazelbuild/bazel_skylib#272 ever gets addressed. With a match_none option for selects.config_setting_group I think it would be a more realistic option to generate config settings for each section of a cfg block and appropriate distill that into one select statement.

Would love your thoughts and to know if there's any functionality missing here.

Also, I wanted to extend a special thank you for always taking the time to review my pull requests 😄 You're the best! @damienmg

impl/src/bazel.rs

damienmg · 2020-09-14T10:01:15Z

@damienmg Hey, I think this is ready for review. I've been using the changes in this branch to work on bazelbuild/rules_rust#392 and was able to easily get generate targets for cbindgen that successfully built on the Linux, MacOS, and Windows CI builds.

Sweet!

I tried a couple of ways to try and come up with some logic to transcribe the cfgs into a matching select statement but wasn't really able to come up with a clean solution. I think this should be easier if bazelbuild/bazel_skylib#272 ever gets addressed. With a match_none option for selects.config_setting_group I think it would be a more realistic option to generate config settings for each section of a cfg block and appropriate distill that into one select statement.

I agree. Anyway for now I think this PR is fine as it is and can be improved on in further iteration. So I would refrain to include that last change now.

Would love your thoughts and to know if there's any functionality missing here.

I see nothing missing just a little code readability.

Also, I wanted to extend a special thank you for always taking the time to review my pull requests 😄 You're the best! @damienmg

Thanks to you for making all those improvements. Unfortunately, I am not as available as I would like to (a lot is going on both profesionnally and personally, so not a lot of time for 20%).

UebelAndre · 2020-09-14T18:45:35Z

@damienmg

I agree. Anyway for now I think this PR is fine as it is and can be improved on in further iteration. So I would refrain to include that last change now.

Great! Yeah, I also think it'd be good to get general user feedback on the implementation as well. I find it to be quite convenient though 😄

Thanks to you for making all those improvements. Unfortunately, I am not as available as I would like to (a lot is going on both profesionnally and personally, so not a lot of time for 20%).

My pleasure! And times are tough so no need to apologize for being busy. I think everyone understands. I hope things are otherwise going well for you and everyone else 😄

UebelAndre · 2020-09-16T14:37:14Z

@damienmg Hey, do you have a quick moment to check out that last commit and potentially merge this?

damienmg · 2020-09-16T19:18:39Z

Thanks again for all your improvement to those repositories!

akhilles · 2020-09-16T23:24:28Z

@UebelAndre with this change, is it no longer necessary to specify:

[raze]
target = "x86_64-unknown-linux-gnu"

UebelAndre · 2020-09-16T23:27:30Z

@akhilles No, it's no longer necessary to specify target you can specify target if you want to limit your dependencies to a specific platform. You can additionally specify a list of targets with targets to limit the dependencies to more than one. But if you specify neither target or targets, you will get all dependencies supported by rules_rust.

dae · 2020-10-08T11:28:13Z

@UebelAndre thanks for your work on this, it seems to bring cargo-raze a step closer to being practical to use for the cross platform app I work on. You asked for feedback at the top; one thing I noticed when I tried removing the target restriction is that flate2 failed to compile:

ERROR: /private/var/tmp/_bazel_dae/4928556feb6f3e9c4d256de8caa46cdd/external/raze__flate2__1_0_14/BUILD.bazel:44:13: Label '@raze__miniz_oxide__0_3_7//:miniz_oxide' is duplicated in the 'deps' attribute of rule 'flate2'
ERROR: /Users/dae/Work/code/dtop/cargo/BUILD.bazel:87:6: Target '@raze__flate2__1_0_14//:flate2' contains an error and its package is in error and referenced by '//cargo:flate2'

The generated deps are:

    deps = [
        "@raze__cfg_if__0_1_10//:cfg_if",
        "@raze__crc32fast__1_2_0//:crc32fast",
        "@raze__libc__0_2_79//:libc",
        "@raze__miniz_oxide__0_4_3//:miniz_oxide",
    ] + selects.with_or({
        # cfg(all(target_arch = "wasm32", not(target_os = "emscripten")))
        (
            "@io_bazel_rules_rust//rust/platform:wasm32-unknown-unknown",
        ): [
            "@raze__miniz_oxide__0_4_3//:miniz_oxide",
        ],
        "//conditions:default": [],
    }),

If instead I set targets:

targets = [
    "aarch64-unknown-linux-gnu",
    "i686-apple-darwin",
    "i686-pc-windows-gnu",
    "i686-unknown-linux-gnu",
    "powerpc-unknown-linux-gnu",
    "x86_64-apple-darwin",
    "x86_64-pc-windows-gnu",
    "x86_64-unknown-linux-gnu",
]

then it succeeds in compiling, but now reqwests fails to compile:

error[E0432]: unresolved import `native_tls_crate`
  --> external/raze__reqwest__0_10_4/src/async_impl/client.rs:23:5
   |
23 | use native_tls_crate::TlsConnector;
   |     ^^^^^^^^^^^^^^^^ use of undeclared type or module `native_tls_crate`

If I switch back to

target = "x86_64-apple-darwin"

and the stable cargo raze 0.5.0, then the compile succeeds.

UebelAndre · 2020-10-08T12:52:58Z

@dae Thanks! I've enjoyed working on this project 😄

Can I get you to open a new issue for this and include some example source or just a Cargo.toml file I can use to run raze against and dig into this deeper? 🙏

UebelAndre mentioned this pull request Sep 8, 2020

Improved tera errors #211

Merged

UebelAndre added 3 commits September 12, 2020 22:41

Added multi target support

e62cf8d

Updated common examples

0a1e979

Updated multi target examples

5b87ac8

damienmg approved these changes Sep 14, 2020

View reviewed changes

impl/src/bazel.rs Outdated Show resolved Hide resolved

Updated supported platform triples based on pull request feedback

714b14d

UebelAndre mentioned this pull request Sep 15, 2020

New Release #216

Closed

damienmg approved these changes Sep 16, 2020

View reviewed changes

damienmg merged commit fa1672c into google:master Sep 16, 2020

UebelAndre deleted the multi-target-support branch September 16, 2020 19:18

This was referenced Sep 16, 2020

WIP: Support multiple targets in output generation #54

Closed

Is the raze-level "target" setting still needed? #131

Open

Support for targeting multiple architectures #43

Closed

dae mentioned this pull request Oct 8, 2020

Nightly target-specific feature selection not supported #234

Closed

This was referenced Oct 8, 2020

When no target set, flate2 fails to compile with latest HEAD #241

Closed

reqwests fails to compile with git HEAD #242

Closed

build script was missing platform-specific deps #243

Merged

UebelAndre mentioned this pull request Oct 12, 2020

Support for Binary dependencies #227

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi target support #212

Multi target support #212

UebelAndre commented Sep 4, 2020 •

edited

Loading

UebelAndre commented Sep 13, 2020

damienmg commented Sep 14, 2020

UebelAndre commented Sep 14, 2020

UebelAndre commented Sep 16, 2020

damienmg commented Sep 16, 2020

akhilles commented Sep 16, 2020

UebelAndre commented Sep 16, 2020

dae commented Oct 8, 2020

UebelAndre commented Oct 8, 2020

Multi target support #212

Multi target support #212

Conversation

UebelAndre commented Sep 4, 2020 • edited Loading

UebelAndre commented Sep 13, 2020

damienmg commented Sep 14, 2020

UebelAndre commented Sep 14, 2020

UebelAndre commented Sep 16, 2020

damienmg commented Sep 16, 2020

akhilles commented Sep 16, 2020

UebelAndre commented Sep 16, 2020

dae commented Oct 8, 2020

UebelAndre commented Oct 8, 2020

UebelAndre commented Sep 4, 2020 •

edited

Loading