[rule-based toolchains] Add rules to define artifact_name_pattern

[Yannic]

Required to override Bazel's (Linux) defaults for macOS and Windows toolchains.

Fixes #332

[Yannic]

@matts1 @armandomontanez PTAL

[Yannic]

@keith thanks for pointing to the feature request. Mind taking a look as well?

[keith]

tested with our toolchain that supports macOS and this works! thanks for doing it! I do think you need this patch though?

diff --git a/cc/toolchains/toolchain.bzl b/cc/toolchains/toolchain.bzl
index 9634485..4b52515 100644
--- a/cc/toolchains/toolchain.bzl
+++ b/cc/toolchains/toolchain.bzl
@@ -67,6 +67,7 @@ def cc_toolchain(
         supports_param_files = False,
         compiler = "",
         target_system_name = "",
+        artifact_name_patterns = [],
         **kwargs):
     """A C/C++ toolchain configuration.

@@ -170,6 +171,7 @@ def cc_toolchain(
         enabled_features = enabled_features,
         compiler = compiler,
         target_system_name = target_system_name,
+        artifact_name_patterns = artifact_name_patterns,
         cpu = select({
             "@rules_cc//cc/toolchains:darwin_aarch64": "darwin_arm64",
             "@rules_cc//cc/toolchains:darwin_x86_64": "darwin_x86_64",

at least it seems like the desire from these rules is to use that entrypoint, so we'd need to thread them through there

[Yannic]

Thanks! Added the patch you're suggesting

[Yannic]

Thanks for the review @matts1!

What's needed to get this merged? @comius can you help here please?

[armandomontanez]

Apologies for the delay, at a high level this looks good to me, need to take a closer look.

[armandomontanez]

The one thing that seems to be missing from this PR is validation: it should definitely be an error to bind multiple cc_artifact_name_pattern rules that cover the same category to a toolchain. I haven't experimented with Bazel's behavior around this, so I don't know off the top of my head if there's already any built-in validation (and how clear the message is). It's often better to write a bespoke check only because you can do things like say //path/to:toolchain has multiple conflicting artifact name patterns for the following group(s): a, b, c.

With action configs, we handled this via the cc_tool_map, which emits error messages when you get this wrong:

rules_cc/tests/rule_based_toolchain/tool_map/tool_map_test.bzl

Lines 72 to 76 in 9c91c03

	expect_failure_test(
	name = name,
	target = subject_name,
	failure_message = "appears multiple times in your tool_map",
	)

With cc_artifact_name_pattern, I don't think we need to add a dictionary/map since the cc_artifact_name_pattern rules are sufficiently modular. We just need to make sure the error message is sufficiently clear.

[Yannic]

Bazel does validate that there are no duplicates here. I think it'll throw IllegalArgumentException though, so it might result in a server crash. Will add validation from Starlark to avoid that and get a better error message.

[Yannic]

PTAL

[armandomontanez]

Copybara isn't happy for some reason, I'll see if I can figure that out.

[Yannic]

Any updates on making copybara happy?