Pass linker script through to sandbox

[m3rcuriel]

Currently, there is no mechanism by which to forward files into the sandbox for compile-time use.

The specific use-case I ran into differs from #79, because instead of dealing with Rust's mechanisms for including files at compile time, I am attempting to forward a linker script all the way through, such that the following code will work:

rust_binary(
  ...
  rustc_flags = [
    "--codegen=link-arg=-Tscript.ld",
  ],
)

This serves the usecase of embedded Rust by allowing the author to lay out the binary in specific ways. Additionally, this mirrors bazelbuild/bazel#184.

Based on the "proof of concept" implementation I have (and I use the word loosely), this more or less comes down to needing to filter these out in rustc.bzl::collect_deps, wrap them into a new field of dep_info, add them into compile_inputs, and do something like args.add_all(["-L" + file.dirname for file in dir_info.link_scripts]). Furthermore, this would just boil down to checking file extensions for .ld, .lds, and .ldscript, which is how Bazel handles this in Java for CcBinary anyways.

Another way to handle this, of course, is to do it in the toolchain itself but since this is the behavior of cc_binary it presumably can be argued for rust_binary.

[mfarrugi]

This sounds reasonable.

I'm unfamiliar with this, so you can explain how linker scripts would be used from transitive dependencies? Should "--codegen=link-arg=-Tscript.ld", still be declared per binary, or is having a linker script in your transitive dependencies enough? If there are multiple linker scripts, what order are they passed in?

When you say the alternative is putting it in the toolchain, do you mean having an optional linker_scripts attr on rust_toolchain? I agree it's fair to do the same thing as cc_binary, but does it make sense for a target to be able to build with different linker scripts / toolchains?

I think another question is whether we should be more permissive in what files we allow as sources, but that's out of scope for this since providing linker flags in the rules is The Right Thing to do.

[m3rcuriel]

--codegen=link-arg=-Tscript.ld, according to the cc_binary model would be declared only on the cc_binary target, although I suppose it's unavoidable for it to be transitive.

I haven't checked, but I imagine it would be transitive for cc_ rules as well, so that's a little bit yikes. The examples in bazelbuild/bazel#716 and bazelbuild/bazel#807 use linkopts to set the version-script and help address some of this. I'd take a stab at making everything Rust look just like cc_binary but I really don't understand custom rules enough to work though that.

(Actually bazelbuild/bazel#716 convinces me that the linkopts are definitely transitive, which is what I thought, but that they aren't transitive into the sandbox, which is hilarious and also not fixed yet.)

As for the alternative, considering this is the linking step (bear in mind I'm still digging into the rules_rust compilation logic post #133), I could probably do it via the cc_toolchain configuration, but that's... meh?

Considering that platforms are now vaguely released, another option might be inheriting toolchains and letting that handle the use-case. An example is two different STM32 boards might have the same target triple, i.e arm-none-eabi, but have a different FLASH/RAM layout, different memory mapped registers, etc.

[mfarrugi]

If this shouldn't obviously be transitive it might make sense to add a linker script arg to rust_binary, which is a little simpler than updating collect_deps and the providers. We don't have an equivalent to (transitive) linkopts, that could be added as well. I haven't read about platforms, so I can't comment on that. The cc_toolchain is a bit of a black box still. Even if a toolchain/platform is the right place to put this sort of hardware specific configuration, it might be better to at least start with the simpler solution.

…

On Wed, Mar 13, 2019, 00:27 Lee Mracek ***@***.***> wrote: --codegen=link-arg=-Tscript.ld, according to the cc_binary model would be declared only on the cc_binary target, although I suppose it's unavoidable for it to be transitive. I haven't checked, but I imagine it would be transitive for cc_ rules as well, so that's a little bit yikes. The examples in bazelbuild/bazel#716 <bazelbuild/bazel#716> and bazelbuild/bazel#807 <bazelbuild/bazel#807> use linkopts to set the version-script and help address some of this. I'd take a stab at making everything Rust look just like cc_binary but I really don't understand custom rules enough to work though that. (Actually bazelbuild/bazel#716 <bazelbuild/bazel#716> convinces me that the linkopts are definitely transitive, which is what I thought, but that they aren't transitive into the sandbox, which is hilarious and also not fixed yet.) As for the alternative, considering this is the linking step (bear in mind I'm still digging into the rules_rust compilation logic post #133 <#133>), I could probably do it via the cc_toolchain configuration, but that's... meh? Considering that platforms are now vaguely released, another option might be inheriting toolchains and letting that handle the use-case. An example is two different STM32 boards might have the same target triple, i.e arm-none-eabi, but have a different FLASH/RAM layout, different memory mapped registers, etc. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#204 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACLjeCUqTh0EBUAXR-U380fh4StI3zruks5vWH4hgaJpZM4bsVhM> .

[m3rcuriel]

So the place to do this without touching rules_rust is definitely platforms, and I know exactly how I would go about making that work; I just did it for C++ as a trial.

However, I think it makes sense to support it within the rules.

Regarding transitivity: The argument made in Bazel upstream is that a library should be able to say "hey I need to be linked this way" but I don't agree with that and think it should be only used on cc_binary.

It's also much easier when it doesn't have to work for libraries.

I'm not sure how I feel about adding a link_script argument. The easiest way to deal with this in my opinion is just to allow files with the .ld, .lds, .ldscript extensions to be passed as dependencies into rust_binary.

For example, here is my working cc_binary rule using the same ldscript:

cc_binary(
    name = "hello_cc",
    srcs = ["hello.cc"],
    linkstatic = 1,
    deps = [
        "//tools/embedded:stm32f30_link.ld",
    ],
    linkopts = [
        "-T$(location //tools/embedded:stm32f30_link.ld)",
    ],
)

I'd like the Rust one to be constructed identically, replacing linkopts with rustc_flags

[mfarrugi]

Does this work?

rust_binary(
    name = "hello_rs",
    srcs = ["hello.rs"],
    data = [
        "//tools/embedded:stm32f30_link.ld",
    ],
    rustc_flags = [
        "-T$(location //tools/embedded:stm32f30_link.ld)",
    ],
)

https://github.com/bazelbuild/rules_rust/blob/master/rust/private/rustc.bzl#L187 should be making data available at compile time.

I think needing to pass "-T$(location //tools/embedded:stm32f30_link.ld)", is not particularly nice, and the rules would be able to figure that part out if we allow .ld deps. My current understanding is that linker scripts are most closely associated with a platform, but this approach is light enough that I don't see the need to implement this that way.

[m3rcuriel]

You're right that does totally work.

The reason why I didn't do that initially is because I was trying to use a filegroup to mash a bunch of link scripts together, and now I just gave up and use a genrule to cat them all together :^).

I agree that passing -T is not particularly nice, but also I think it's just the right level. This is theoretically a fairly rare use-case.

I think that the current status quo is fine then? And in exploring build_settings I may have found a way to make it work in the toolchain itself and still not have to have n*(number of link scripts) toolchains now that toolchains work in Starlark.

So, for me, I think this is a closed issue. Not having support for filegroups of linker scripts is a little bit rough but I think unreasonable to ask for; cc_binary doesn't support that at all.

[mfarrugi]

I agree it's a relatively rare issue, but it would be nice for rules_rust to really shine in some of the places where Cargo gets hairy (linker scripts, build scripts, etc). It would also be a fairly lightweight change if you change your mind.

FWIW, I didn't suggest using data earlier because I didn't realize we could use location substitution in parameters like that.

[hlopko]

Since #208 is merged, this issue can be closed, right? Please complain if that's not true :)