[bazel] Switch to platforms-based toolchain resolution

[jfirebaugh]

Fixes #984. However, the way I have done it requires --incompatible_enable_cc_toolchain_resolution and no longer supports --crosstool_top=.... I'm not sure how to support both at the same time. If that's necessary, can someone give me pointers on how to do it?

[jfirebaugh]

It looks like test-bazel-mac runs a version of bazel that does not include @platforms//cpu:wasm32. I think this is what #766 (comment) was referring to. What's the preferred way to fix this?

[zaucy]

@jfirebaugh if you rebase now with #1049 merged in the bazel mac CI should pass!

[jfirebaugh]

Done.

Does emsdk need to continue to support both toolchain and non-toolchain (--crosstool_top=...) based resolution?

[walkingeyerobot]

I'm very sorry I haven't reviewed this yet but I will soon. Thanks very much for the PR!

[walkingeyerobot]

Ok, this looks great. My one comments is I'd like the bazelrc file restored and to contain the following:

build:wasm --incompatible_enable_cc_toolchain_resolution
build:wasm --platforms=@emsdk//:platform_wasm

I believe this will allow anyone who has a build working with --config=wasm to have their build continue working.

[walkingeyerobot]

Thanks very much for this!

As this is a breaking change, I'm going to hold off on merging it for just a bit while I figure out how to best communicate this.

[walkingeyerobot]

Ok, after a bit of testing, it looks like we can support both the old crosstool_top way and the new platforms stuff. If we add back in the transition passing compiler, cpu, and crosstool_top, everything should still work for platforms.

I'm sorry this has been such a lengthy review. This is an excellent change; I just want to be careful so as to break as few users as possible.

[sbc100]

Can/should we fold these two into a single step?

[jfirebaugh]

Seems like other rulesets tend to keep them separate. E.g. rules_go has go_rules_dependencies and go_register_toolchains , rules_nodejs has build_bazel_rules_nodejs_dependencies and node_repositories, rules_rust has rules_rust_dependencies and rust_register_toolchains.

[jfirebaugh]

I saw that #1068 spells this emsdk_register_toolchains. Based on other rulesets, it does seem like the prevailing convention is to prefix with the language or toolset, rather than the verb register. However, I would propose that we retain consistency with emscripten_deps, and therefore spell this emscripten_register_toolchains. @walkingeyerobot @jun-sheaf WDYT?

[jrandolf]

Hey @jfirebaugh! Your observation is correct. But just some information regarding this, I have upstreamed this to internal already. Hopefully by August I can get the internal toolchain open-sourced.

[jfirebaugh]

What does "upstreamed to internal" mean?

[jrandolf]

I meant the patch has already been published within Google (the internal repo).

[walkingeyerobot]

compiler, cpu, and crosstool_top need to be in outputs, not inputs. They can in fact be removed from inputs as we don't need to read them. This will fix the failing CI

[jrandolf]

This will not fix the CI. You need to set the incompatible toolchain flag. See #1060.

[jrandolf]

Just found this PR. We've been using #1060 for a while internally, so we wanted to upstream those updates. I've noticed this PR misses out on some parts. Specifically, we should label the toolchain as wasm32 (since that is what it is) and also fix the static linker flags. All of this is already done in #1060 (I've also downstreamed some of the changes here to there).
.

[walkingeyerobot]

Just found this PR. We've been using #1060 for a while internally, so we wanted to upstream those updates. I've noticed this PR misses out on some parts. Specifically, we should label the toolchain as wasm32 (since that is what it is) and also fix the static linker flags. All of this is already done in #1060 (I've also downstreamed some of the changes here to there). .

Thanks for the PR! I'm happy with the state this PR is in (sans the small fix I commented on), so I'd like to get this in before reviewing yours.

[jrandolf]

Just found this PR. We've been using #1060 for a while internally, so we wanted to upstream those updates. I've noticed this PR misses out on some parts. Specifically, we should label the toolchain as wasm32 (since that is what it is) and also fix the static linker flags. All of this is already done in #1060 (I've also downstreamed some of the changes here to there). .

Thanks for the PR! I'm happy with the state this PR is in (sans the small fix I commented on), so I'd like to get this in before reviewing yours.

I'd recommend taking a look at #1060 before moving forward with this one. There are some non-trivial fixes there and also improvements to the implementation. We also want it upstreamed sooner so we can check back out to @emsdk/main.

[jfirebaugh]

I’m away from a computer for a few weeks and won’t be able to make the changes until July. Feel free to make the changes yourself or pursue the other PR, I won’t be offended. 😄

[jfirebaugh]

Ping @walkingeyerobot -- I've made the requested changes.

[HappyCerberus]

Hi, I tried using this patch, but run into a problem. I had to comment out this from emscripten_deps.bzl:

    if "emscripten_bin_mac_arm64" not in excludes:
        http_archive(
            name = "emscripten_bin_mac_arm64",
            strip_prefix = "install",
            url = emscripten_url.format("mac", revision.hash, "-arm64", "tbz2"),
            sha256 = revision.sha_mac_arm64,
            build_file_content = BUILD_FILE_CONTENT_TEMPLATE.format(bin_extension = ""),
            type = "tar.bz2",
        )

Without removing it I was getting the following error:

                sha256 = revision.sha_mac_arm64,
Error: 'struct' value has no field or method 'sha_mac_arm64'
Available attributes: hash, sha_linux, sha_mac, sha_win

[jfirebaugh]

@HappyCerberus Are you sure it's related to this patch? It looks like you're using an emscripten version that doesn't provide an arm64 macOS build. Judging by revisions.bzl, 3.1.2 was the first version that supported it:

emsdk/bazel/revisions.bzl

Lines 124 to 136 in c220895

	"3.1.2": struct(
	hash = "6626e25d6d866cf283147ca68d54ac9326fe399f",
	sha_linux = "4fb53364a2ba1de8978445aa26b2204bfd215b41da5d7df04f231040b197010a",
	sha_mac = "a8e347accb1ff402d96a128912ac8cda1731611c9f89095fee0ad39a6a18bbc3",
	sha_mac_arm64 = "4374f5c852d0403b0a3b0e9dc8a3856a340e9d82ecf0f20aa8b36c6179d31fc8",
	sha_win = "e96f6ab8252fefa42f461676311d4c4e2d96fdc2e876ece07d9d7a49ef31aef0",
	),
	"3.1.1": struct(
	hash = "5ee64de9809592480da01372880ea11debd6c740",
	sha_linux = "ba94c5ecabacbedc89665a742c37c4c132c739aea46aa66fd744cb72b260c870",
	sha_mac = "8b5f8cec55af0e6816a08d8d1e8b873f96d0e0504fdd6e8deb2fc024957d1aa7",
	sha_win = "6cbe976aff6155cf1c48707f0520b5aa6a7770860e9b1964bfca3e5923ce7225",
	),

[HappyCerberus]

@jfirebaugh Thanks. I did not realize this was it. Retested and it works now.

Perhaps would make sense to remove the older revisions from the revisions file, since the code doesn't work with them anyway?

[walkingeyerobot]

It's been awhile, and I apologize for that. I would like to get this merged. If you could please rebase so that we can get CI to pass (I think that's the problem anyway) then we can go ahead and merge.