crate_universe does not consolidate configurations that match the same set of platforms.

[pikajude]

Here's the Cargo.toml for rustix-0.33.7: https://docs.rs/crate/rustix/0.33.7/source/Cargo.toml

The resulting generated file: https://gist.github.com/pikajude/4c6eec44aba219e58f5803a7efecf10b

The condition # cfg(all(windows, not(target_vendor = "uwp"))) duplicates the condition # cfg(windows). See lines 53 and 97. This results in the error:

ERROR: Traceback (most recent call last):
        File "/home/ubuntu/.cache/bazel/_bazel_ubuntu/76ed2d273358d4668b08d66cb9d677b9/external/crate_index__rustix-0.33.7/BUILD.bazel", line 101, column 10, in <toplevel>
                ): [
Error: dictionary expression has duplicate key: ("@rules_rust//rust/platform:i686-pc-windows-msvc", "@rules_rust//rust/platform:x86_64-pc-windows-msvc")
ERROR: /home/ubuntu/.cache/bazel/_bazel_ubuntu/76ed2d273358d4668b08d66cb9d677b9/external/crate_index__wasmtime-runtime-0.35.3/BUILD.bazel:33:13: no such target '@crate_index__rustix-0.33.7//:rustix': target 'rustix' not declared in package '' defined by /home/ubuntu/.cache/bazel/_bazel_ubuntu/76ed2d273358d4668b08d66cb9d677b9/external/crate_index__rustix-0.33.7/BUILD.bazel and referenced by '@crate_index__wasmtime-runtime-0.35.3//:wasmtime_runtime'

This is not a duplicate of #1236 as we aren't using supported_platform_triples in our workspace file.

[UebelAndre]

What rev of rules_rust are you using?

This seems to be caused by uwp not being represented in a unique windows platform but is still a unique configuration. The fix is to either define that as a platform and pass it to supported_platform_triples or to update crate_universe to flatten configurations that cannot be uniquely represented. I thought we did the latter already... I'll need to double check when I get a chance.

[pikajude]

0.6.0. How recently did you make the change you mentioned?

[UebelAndre]

Sorry for the delay here. This issue comes from the unique configurations not being consolidated when there aren't unique platforms (represented by Bazel) to match them (code here). This creates conflicting select cases and you get the error you see. I think the fix is probably just to write a wrapper for select_with_or that would do this consolidation (since I think the rust code is correct in representing configurations the way it currently is). I'd expect the implementation would result in something like the following diff:

diff --git a/BUILD.rustix-0.33.7.bazel b/BUILD.rustix-0.33.7.bazel
index 9c8ddade..fd6c0260 100644
--- a/BUILD.rustix-0.33.7.bazel
+++ b/BUILD.rustix-0.33.7.bazel
@@ -21,7 +21,7 @@ load(
 )

 # buildifier: disable=bzl-visibility
-load("@rules_rust//crate_universe/private:selects.bzl", "select_with_or")
+load("@rules_rust//crate_universe/private:selects.bzl", "consolidate", "select_with_or")

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

@@ -33,7 +33,7 @@ package(default_visibility = ["//visibility:public"])
 rust_library(
     name = "rustix",
     deps = [
-    ] + select_with_or({
+    ] + select_with_or(consolidate({
         # cfg(all(not(rustix_use_libc), not(miri), target_os = "linux", any(target_arch = "x86", all(target_arch = "x86_64", not(target_pointer_width = "32")), target_arch = "arm", target_arch = "aarch64", target_arch = "powerpc64", target_arch = "riscv64", target_arch = "mips", target_arch = "mips64")))
         (
             "@rules_rust//rust/platform:aarch64-unknown-linux-gnu",
@@ -112,7 +112,7 @@ rust_library(
             "@crate_index__io-lifetimes-0.5.3//:io_lifetimes",
             "@crate_index__rustix-0.33.7//:build_script_build",
         ],
-    }),
+    })),
     proc_macro_deps = [
     ] + select_with_or({
         "//conditions:default": [

Does that sound reasonable (wondering @illicitonion's thoughts too)? And is that something you'd (@pikajude) have bandwidth to work on? I'd be more than happy to review and collaborate on in a PR 😄

[illicitonion]

I'm not sure consolidate can be written as described; it's still being passed a dict literal with multiple definitions of the same key? But I guess it could take a list of pairs or something...

I don't have strong thoughts on whether it makes sense to do this consolidation on the rust side or the starlark side, other than that in general rust is more expressive which can be useful :)

[sayrer]

I just patched this in cargo-raze in Rust. I'm not sure my implementation is as brief as it could be (although, in raze, it's generic over features too). But, fwiw, I ended up having to do a list of pairs with some set operations at the end.

google/cargo-raze#512

[nathanosdev]

I just encountered this issue too, I can make an attempt at a PR @UebelAndre

[illicitonion]

I can make an attempt at a PR @UebelAndre

I would gladly review it! Thanks @nathanosdev!

[wmatthews-google]

@nathanosdev - if you haven't had a chance to work on this, I can attempt a PR for this in the next few days.

[nathanosdev]

@wmatthews-google That would be awesome. If you change your mind or something else gets in the way, let me know and I can pick it back up again.

[wmatthews-google]

Ok... I've got a draft PR out that works for everything I've thrown at it thus far (other than an issue with platforms and crate_features with wgpu-hal, but that's separate): #1647

[wmatthews-google]

I think this is now fixed?

[UebelAndre]

I think this is now fixed?

Closing this out until someone says otherwise 😄