Fix issue #1417 - Group deps by platform triple rather than cfg(...) string.

[wmatthews-google]

Group deps and aliases by platform triple rather than by cfg string when generating BUILD files. This avoid Bazel errors due to duplicate keys/deps. See #1417.

This is done purely at render time by adding a new Tera filter which will remap the conditions in the deps SelectList from "cfg(...)" strings to platform triple strings.

This is my first PR here, so please bear with me if I'm doing things wrong!

[UebelAndre]

Thanks! Could you show an example of what these files render out to? One of the reason cfg strings were used was to try and make it clearer what was happening in the select statements so that if a dependency was unexpectedly missing or present, we can see what Cargo says and how that maps to rust. Just wanna make sure that's still clear.

[wmatthews-google]

@UebelAndre - thanks for taking a look! Right now it's not clear at all in the output how deps map to cfg blocks...

e.g. for errno whose crate has

[target."cfg(target_os=\"dragonfly\")".dependencies.errno-dragonfly]
version = "0.1.1"
[target."cfg(target_os=\"hermit\")".dependencies.libc]
version = "0.2"
[target."cfg(target_os=\"wasi\")".dependencies.libc]
version = "0.2"
[target."cfg(unix)".dependencies.libc]
version = "0.2"
[target."cfg(windows)".dependencies.winapi]
version = "0.3"

we now generate

    deps = [
    ] + select({
        "@rules_rust//rust/platform:aarch64-apple-darwin": [
            "@crates_vendor__libc-0.2.137//:libc",
        ],
        "@rules_rust//rust/platform:aarch64-apple-ios": [
            "@crates_vendor__libc-0.2.137//:libc",
        ],
        "@rules_rust//rust/platform:aarch64-apple-ios-sim": [
            "@crates_vendor__libc-0.2.137//:libc",
        ],
        "@rules_rust//rust/platform:aarch64-linux-android": [
            "@crates_vendor__libc-0.2.137//:libc",
        ],
        "@rules_rust//rust/platform:aarch64-unknown-linux-gnu": [
            "@crates_vendor__libc-0.2.137//:libc",
        ],
        "@rules_rust//rust/platform:arm-unknown-linux-gnueabi": [
            "@crates_vendor__libc-0.2.137//:libc",
        ],
        "@rules_rust//rust/platform:armv7-linux-androideabi": [
            "@crates_vendor__libc-0.2.137//:libc",
        ],
        "@rules_rust//rust/platform:armv7-unknown-linux-gnueabi": [
            "@crates_vendor__libc-0.2.137//:libc",
        ],
        "@rules_rust//rust/platform:i686-apple-darwin": [
            "@crates_vendor__libc-0.2.137//:libc",
        ],
        "@rules_rust//rust/platform:i686-linux-android": [
            "@crates_vendor__libc-0.2.137//:libc",
        ],
        "@rules_rust//rust/platform:i686-pc-windows-msvc": [
            "@crates_vendor__winapi-0.3.9//:winapi",
        ],
        "@rules_rust//rust/platform:i686-unknown-freebsd": [
            "@crates_vendor__libc-0.2.137//:libc",
        ],
        "@rules_rust//rust/platform:i686-unknown-linux-gnu": [
            "@crates_vendor__libc-0.2.137//:libc",
        ],
        "@rules_rust//rust/platform:powerpc-unknown-linux-gnu": [
            "@crates_vendor__libc-0.2.137//:libc",
        ],
        "@rules_rust//rust/platform:s390x-unknown-linux-gnu": [
            "@crates_vendor__libc-0.2.137//:libc",
        ],
        "@rules_rust//rust/platform:wasm32-wasi": [
            "@crates_vendor__libc-0.2.137//:libc",
        ],
        "@rules_rust//rust/platform:x86_64-apple-darwin": [
            "@crates_vendor__libc-0.2.137//:libc",
        ],
        "@rules_rust//rust/platform:x86_64-apple-ios": [
            "@crates_vendor__libc-0.2.137//:libc",
        ],
        "@rules_rust//rust/platform:x86_64-linux-android": [
            "@crates_vendor__libc-0.2.137//:libc",
        ],
        "@rules_rust//rust/platform:x86_64-pc-windows-msvc": [
            "@crates_vendor__winapi-0.3.9//:winapi",
        ],
        "@rules_rust//rust/platform:x86_64-unknown-freebsd": [
            "@crates_vendor__libc-0.2.137//:libc",
        ],
        "@rules_rust//rust/platform:x86_64-unknown-linux-gnu": [
            "@crates_vendor__libc-0.2.137//:libc",
        ],
        "//conditions:default": [
        ],
    }),

... which makes Bazel happy, but I agree is less clear to humans.

A possible idea:

Suppose I make remap_conditions return SelectList<WithOriginalConditions<T>> where (roughly)

struct WithOriginalConditions<T> {
    value: T,
    original_conditions: BTreeSet<Option<String>>
}

then render the original_conditions (Some("cfg(...)") etc., or None for default) as comments above each dep.

... then the output would be something like

        # ...
        "@rules_rust//rust/platform:x86_64-unknown-linux-gnu": [
            # cfg(unix)
            "@crates_vendor__libc-0.2.137//:libc",
        ],
        # ...

... somewhat long and verbose but at least explicit. (And hopefully humans rarely read these generated files anyway).

There is still a down side that conditions which don't match any platform triples end up silently dropped in remap_conditions. If this is a concern, remap_conditions could return an additional BTreeSet<String> of unmapped conditions which could be rendered as comments too.

WDYT?

Related question: Should I regenerate (at the same locked version) all BUILD files under crate_universe/3rdparty as part of this PR?

[UebelAndre]

re: #1647 (comment)

Thanks for showing a rendering, the change is way easier to comprehend now! My concern about readability is more for debugability. Humans likely won't look at it unless there's a problem and in developing crate_universe I've learned that dependency management concepts can be difficult to keep paged into my brain. I think it'd be a good thing to make sure the future version of maintainers and new developers have an easier time figuring out what's going on. So I like the proposal to retain info for commenting the original configurations. 👍

Related question: Should I regenerate (at the same locked version) all BUILD files under crate_universe/3rdparty as part of this PR?

It would be helpful if you had a commit with those refreshed but it does make PRs harder to comprehend post-merge. Your call 😄

[wmatthews-google]

Updated the code as discussed. The implementation is a big uglier than before (WithOriginalConfigurations<T> isn't a great value type inside SelectList), however we can now render all the original cfg strings in the resulting build files.

See #1655 for a re-rendering of crate_universe/3rdparty BUILD files after this CL. We can choose to merge it too or not as you deem best.

[UebelAndre]

I think this is looking good! There's some error which I'm not entirely sure isn't an infrastructure issue so you may want to push a new commit to force CI to retrigger.

One last thought is it might be good to have some kind of test for the renderer that ensures a crate with a conditional dependency renders a build file that contains a comment for the original config and the expected triple somewhere in the output. There aren't a ton of render tests so this might be more scope increase than I thought but I think that'd be a really good test to have. But to be clear, the requested change is just to investigate CI.

[wmatthews-google]

Thanks!

Added a build file rendering test. LMK if you think it'd be better to frame the assertion in terms of regexes. As-is it's quite clear how the comments map to dependencies, but this comes at the expense of being a bit brittle.

[UebelAndre]

This looks great! One last request but looks good to me otherwise 😄

[UebelAndre]

Awesome work! Thank you so much for fixing this issue 🎉

[wmatthews-google]

@UebelAndre thanks for the review!