Add support for passing a custom target specification.

[davidskidmore]

A label doesn't seem to express both a string (as in a triple like x86_64-unkown-none) and a path to a custom target specification (as in something like //path/to/target.json), so this change is propose treating target_json as an override of target_triple.

[UebelAndre]

Is there a way that this code path can be tested in CI? I think the rules should definitely be supportive to custom targets but I'd like to know when a change impacts support for custom code paths. Perhaps something can be added with the existing unit tests?

[UebelAndre]

Also, to follow up on #770 (comment)

I think this approach makes sense. I would probably add some protections against trying to set both target and target_json but I like the idea of two dedicated attributes for defining the targets, especially after having looked at the Embedonomicon. To me it seems like a natural translation in Bazel and a great addition to the toolchain. But others may know better than I.

[hlopko]

Thanks for working on this!

[hlopko]

LGTM, thank you!

[hlopko]

Suggested change

	target_flag_value = ctx.file.target_json.path if ctx.file.target_json != None else ctx.attr.target_triple,
	target_flag_value = ctx.file.target_json.path if ctx.file.target_json else ctx.attr.target_triple,

[hlopko]

Oh actually, +1 to @UebelAndre's comment, could you please add a check in the toolchain.bzl ensuring that target_json and target_triple are not both specified at the same time, and if yes, then fail() with a nice error message?

[davidskidmore]

Is there a way that this code path can be tested in CI? I think the rules should definitely be supportive to custom targets but I'd like to know when a change impacts support for custom code paths. Perhaps something can be added with the existing unit tests?

I'm a little out of my depth here, being pretty new to writing bazel build logic. Do you have any suggestions on where the new unit tests would go?

[UebelAndre]

I'm a little out of my depth here, being pretty new to writing bazel build logic. Do you have any suggestions on where the new unit tests would go?

We have unit tests located in test/unit. But this one might be pretty unique since the change is to the toolchain? I think a transition might be needed to build a target using a newly registered toolchain which exercises this code path. Maybe someone else might have a simpler path forward though?

[davidskidmore]

Sorry, but I don't really follow. I've tried throwing together a bare bones unit test:

"""Unit tests for rust toolchains."""

load("@bazel_skylib//lib:unittest.bzl", "analysistest")
load("//rust:toolchain.bzl", "rust_stdlib_filegroup", "rust_toolchain")

def _toolchain_specifies_target_triple_test_impl(ctx):
    env = analysistest.begin(ctx)
    target_under_test = analysistest.target_under_test(env)

    # TODO: assert something

    return analysistest.end(env)

toolchain_specifies_target_triple_test = analysistest.make(_toolchain_specifies_target_triple_test_impl)

def _toolchain_test():
    rust_stdlib_filegroup(
        name = "std_libs_none",
        srcs = [],
    )

    rust_toolchain(
        name = "rust_x86_64_none_impl",
        binary_ext = "",
        dylib_ext = ".so",
        os = "linux",
        rust_lib = ":std_libs_none",
        staticlib_ext = ".a",
        stdlib_linkflags = [],
    )

    toolchain_specifies_target_triple_test(
        name = "toolchain_specifies_target_triple_test",
        target_under_test = ":rust_toolchain",
    )

def toolchain_test_suite(name):
    _toolchain_test()

    native.test_suite(
        name = name,
        tests = [":toolchain_specifies_target_triple_test"],
    )

But bazel isn't handling handle references to rust_toolchain correctly:

ERROR: ~/rules_rust/test/unit/toolchain/BUILD:3:21: in target_under_test attribute of toolchain_specifies_target_triple_test rule //test/unit/toolchain:toolchain_specifies_target_triple_test: rule '//test/unit/toolchain:rust_toolchain' does not exist. Since this rule was created by the macro 'toolchain_test_suite', the error might have been caused by the macro implementation
ERROR: Analysis of target '//test/unit/toolchain:toolchain_specifies_target_triple_test' failed; build aborted: Analysis of target '//test/unit/toolchain:toolchain_specifies_target_triple_test' failed

[hlopko]

Oh wow, you actually almost did make it work. In your target_under_test you have ":rust_toolchain", but you actually want ":rust_x86_64_none_impl". Since you started, let's endure :)

So I guess you want to get the ToolchainInfo from the target under test in _toolchain_specifies_target_triple_test_impl and verify that the target_flag_value is as expected, and then add another test with a toolchain that sets target_json and assert different target_flag_value, and presence of target_json file in the provider. That all makes a lot of sense, thank you for the excellence!

[davidskidmore]

Thanks for the advice, that was it! I was quite able to get expect_failure working for a unit test that could demonstrate passing both triple and json at the same time, but I think we're in a pretty good place testing wise.

[UebelAndre]

Would it be possible to rename the x86_64-unknown-linux-gun.json file to something like toolchain-test-triple.json? I feel like it'd be better to set values here that would never match naturally to confirm we're not benefiting from any coincidental assumptions. I think the cross platform tests will probably catch this but I still feel it's worth going the extra step here? Could be convinced otherwise though. Let me know what you think 😅

Otherwise, I think this is looking awesome! Thanks for all the work you put into this 🙏

[UebelAndre]

Also, docs can be fixed by running ./docs/update_docs.sh, The error message in CI should describe the same but hoping this message is easier to find 😄

[davidskidmore]

The target specifications are usually named after the target they're for, but I think it makes sense for the tests to use something more unique.

[UebelAndre]

The target specifications are usually named after the target they're for, but I think it makes sense for the tests to use something more unique.

I agree and would definitely maintain this behavior for any target I define, but yeah, tests should be able to afford the flexibility.

[UebelAndre]

One final nit then otherwise looks good to me!

[hlopko]

LGTM, I think this is good to go.

I believe the nit was talked through and decided not to do anything more in this PR.