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

WIP: Support multiple targets in output generation #54

Closed
wants to merge 4 commits into from

Conversation

acmcarther
Copy link
Member

This will resolve #43

This is an active PR, but i want to be able to see the diff. I'll tag stakeholders when PR is ready.

The strategy pivots around generating config_setting entries in the cargo dir BUILD file that can be referenced by all generated build rules. Those config_settings basically translate target triples into @bazel_tools//platform entries, so they're a near enough match.

Doing this requires differentiating between deps common to all envs, and deps specific to one or more targets, so planning needs some updates as well.

Hypothetical output BUILD file:

 package(default_visibility = ["//visibility:public"])

INTERNAL_WORKSPACE = "@raze_internal"

licenses([
  "notice", # "BSD-3-Clause"
])

load(
    "@io_bazel_rules_rust//rust:rust.bzl",
    "rust_library",
    "rust_binary",
    "rust_test",
    "rust_bench_test",
)

load(
    INTERNAL_WORKSPACE + "//:util.bzl",
    "mandatory_flags",
    "crate_target",
    "crate_config_setting",
)

rust_library(
    name = "adler32",
    crate_root = "src/lib.rs",
    crate_type = "lib",
    srcs = glob(["**/*.rs"]),
    deps = [
      crate_target("semver", "0_8_0")
    ] + select({
        INTERNAL_WORKSPACE + ":x86_64-unknown-linux-gnu": [
          crate_target("error_chain", "0_11_0")
        ]
    }),
    rustc_flags = [
    ] + mandatory_flags() + select({
        INTERNAL_WORKSPACE + ":x86_64-unknown-linux-gnu": [
            "--target=x86_64-unknown-linux-gnu",
        ]
    }),
    crate_features = [
    ] + select({
        INTERNAL_WORKSPACE + ":x86_64-unknown-linux-gnu": [
            "some_feature"
        ]
    }),
)

@damienmg
Copy link
Member

A few follow-up after our VC:

  1. Why not put all the constraints in the rules_rust repository? Also probably better if they are linked to the actual toolchain since they are definitely matching the platform concept in Bazel.
  2. The target flag is definitely something that should be set by the toolchain, you don't want to set it by rule whereas it should always be there.

@acmcarther
Copy link
Member Author

  1. Why not put all the constraints in the rules_rust repository? Also probably better if they are linked to the actual toolchain since they are definitely matching the platform concept in Bazel.

The constraint settings can live in rules rust -- there isn't anything workspace-specific about it. I do want to have a good story for extending the default supported triples... but we can hammer this out in a rules_rust PR.

  1. The target flag is definitely something that should be set by the toolchain, you don't want to set it by rule whereas it should always be there.

When you say "target flag", are you referring to the target triple? If so then yes, I did envision somehow using the target triple from the toolchain to select the appropriate config settings.... or perhaps something in reverse. Maybe the toolchain should be internally configured based on the config settings values to match this. I'm not sure if the toolchain can be configured using config_settings though... I would need to do some research to find out.

@damienmg
Copy link
Member

  1. The target flag is definitely something that should be set by the toolchain, you don't want to set it by rule whereas it should always be there.

When you say "target flag", are you referring to the target triple? If so then yes, I did envision somehow using the target triple from the toolchain to select the appropriate config settings.... or perhaps something in reverse. Maybe the toolchain should be internally configured based on the config settings values to match this. I'm not sure if the toolchain can be configured using config_settings though... I would need to do some research to find out

Yes I am referring to the target triple. The toolchain rule transport provider and can provide various attribute to the rule that can inject directly those flag or target specific dependencies.

@damienmg
Copy link
Member

@acmcarther: Is this still something you are working on? I am interested by having it working :)

@acmcarther
Copy link
Member Author

@damienmg:

I need to review the changes again as I haven't looked in a while. IIRC this predates my reworking of toolchain management in rules_rust (which was a blocker of this PR). The toolchain-side issues were not completely resolved to my satisfaction, as there needs to be a way to properly select a crosslinker (to run on the exec_triple and target the target_triple).

I can commit to updating and polishing this up and providing a more specific set of blockers over the weekend, if that works for you.

@damienmg
Copy link
Member

damienmg commented Oct 20, 2018 via email

@acmcarther
Copy link
Member Author

acmcarther commented Oct 22, 2018

@damienmg: I didn't get a chance to devote the time to this that it deserved this weekend. Regardless, I do have a good suggestion on how you can help push it forward:

I've mentioned several times that selecting the right linker per toolchain {exec_triple, target_triple} is a blocker. It remains a blocker. Its quite difficult to supply dedicated linker binaries for all exec/target combinations. I haven't explored the solution space very much, but I did have a possible avenue of exploration if you've got time to look at it:

Rust is currently moving to use LLD (the llvm linker) instead of gcc/msvc. This linker has the benefit of being guaranteed to be able to cross link to whatever target is specified. I think it might be reasonable to partially enable cross compilation via this linker (using the appropriate rustc invocation flag). Unfortunately, it is nightly only (as well as tier 1 plat only -- so the impl needs to fail gracefully), but we could probably constraint our cross compilation support to nightly in the short term.

I checked out rustc-nightly-x86_64-unknown-linux-gnu and the lld binary is indeed present @ rustc/lib/rustlib/x86_64-unknown-linux-gnu/bin/rust-lld. Rustc can probably find it automatically with just the feature enabled, though, provided that the rust-lld bin is present in the filegroup specified by the toolchain BUILD contents.

@damienmg
Copy link
Member

@acmcarther at the moment I am not so much interested in cross compilation but rather to make my protobuf PR works on OSX too. I can manually amend the BUILD file to add the correct select but that does not scale (especially since I intend to add documentation about how to do it, since most of the case you want to regenerate it for your own project for now).

@acmcarther
Copy link
Member Author

Oh I see. I misunderstood your objectives.

I lost the thread of the original design of this PR, but I believe it requires predefined platforms to be added to rules_rust. I've added them here: bazelbuild/rules_rust#144, and we can start using them in cargo-raze once that is submitted.

@sayrer
Copy link
Contributor

sayrer commented Mar 12, 2019

Is there any update on this?

It looks like this a volunteer effort, so I don't really have many expectations, but the docs on the README are wrong now (the code no longer produces a rustc target argument). Or, would it make more sense to wait for a "cargo_crate" rule in rules_rust?

@sayrer
Copy link
Contributor

sayrer commented Mar 13, 2019

or, maybe this mostly works now? I just saw bazelbuild/rules_rust#205 (that would mean the docs need some updates)

@UebelAndre UebelAndre mentioned this pull request Sep 4, 2020
damienmg pushed a commit that referenced this pull request Sep 16, 2020
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:
```python
"""
@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.
```toml
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
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
Copy link
Collaborator

This PR can likely be closed since #212 got merged.

@UebelAndre
Copy link
Collaborator

@acmcarther this PR could probably be closed

@acmcarther acmcarther closed this Oct 16, 2020
@acmcarther acmcarther deleted the acm-support-multi-target branch October 16, 2020 17:41
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.

Support for targeting multiple architectures
4 participants