emscripten-core / emsdk

Emscripten SDK
http://emscripten.org
Other
2.92k stars 662 forks source link

(bazel) Set @platforms//os:emscripten for platform_wasm #1363

Closed trzeciak closed 3 months ago

trzeciak commented 3 months ago

I noticed that @googlewalt added @platforms//os:emscripten LINK, now we can update the platform_wasm.

trzeciak commented 3 months ago

This change require use platforms in version 0.0.9+, in my own project, I set it in MODULE.bazel:

bazel_dep(name = "platforms", version = "0.0.9")
sbc100 commented 3 months ago

CI is reporting ERROR: /root/project/bazel/BUILD:81:9: no such target '@@platforms//os:emscripten': target 'emscripten' not declared in package 'os' defined by /root/.cache/bazel/_bazel_root/04d6701d4e894b3c38d7a891fc3b3ded/external/platforms/os/BUILD and referenced by '//:platform_wasm'

I guess something else needs updating?

walkingeyerobot commented 3 months ago

Likely a bazel version bump is needed somewhere...

trzeciak commented 3 months ago

If I understand the operation of bazel correctly, bazel has a built-in dependency for platforms (and several others), but it can be changed with an entry in the MODULE or WORKSPACE file.
I don't know when platform@0.0.9 will become built-in to bazel.
Therefore, I don't know what to do next with this MR so as not to spoil the work of others.
Or maybe it would be possible to add a dependency to a specific version of the platforms, and then it would propagate to the main project?

walkingeyerobot commented 3 months ago

Sadly I'm not entirely sure either. I suspect our bazel on CI is out of date. I think that's controlled approximately here: https://github.com/emscripten-core/emsdk/blob/main/.circleci/config.yml.

It looks like windows is using bazelisk but requesting version 5.4.0, which appears to be pretty old. It looks like mac is also using bazelisk but not requesting a version, so I'd sort of expect that one to work. And then linux... I'm not really sure what's going on with that.

What version of bazel are you running locally? @trzeciak

fmeum commented 3 months ago

Since essentially everything depends on platforms and the first definition in WORKSPACE wins, most WORKSPACE users won't be on a platforms version with this target. However, most users will know that the error they get can be solved by updating their dep on platforms - simply because this breaks all the time with WORKSPACE and even a Bazel update doesn't help if something else brings in an outdated version.

With Bzlmod, bumping the bazel_dep version is all you need.

All in all, I would say go for the update.

sbc100 commented 3 months ago

I don't think there is a question as to whether we want to upgrade, but more of question of how to fix the CI tests which are failing. We can't land this until we fix those.

fmeum commented 3 months ago

Could you try adding

maybe(
    http_archive,
    name = "platforms",
    urls = [
        "https://mirror.bazel.build/github.com/bazelbuild/platforms/releases/download/0.0.9/platforms-0.0.9.tar.gz",
        "https://github.com/bazelbuild/platforms/releases/download/0.0.9/platforms-0.0.9.tar.gz",
    ],
    sha256 = "5eda539c841265031c2f82d8ae7a3a6490bd62176e0c038fc469eabf91f6149b",
)

to this macro?

trzeciak commented 3 months ago

Oooo, this looks very promising, I will try it locally first.

trzeciak commented 3 months ago

@fmeum Ok, I test it locally on macOS, and after add your snippet to deps macro, I tested it for few different version of bazel (using bazelisk):

# USE_BAZEL_VERSION=5.4.0  # OK
# USE_BAZEL_VERSION=6.4.0  # OK
# USE_BAZEL_VERSION=6.5.0  # OK
# USE_BAZEL_VERSION=7.0.0  # ERR
# USE_BAZEL_VERSION=7.0.2  # ERR
# USE_BAZEL_VERSION=7.1.1  # ERR

and it seems that not work for 7+ version.

I also followed the circleci logs as @walkingeyerobot mentioned, and now I see that on CI was used latest bazel version (7.1.1) on linux and macOS, and and for some reason 5.4.0 on windows.

Following this line of reasoning, I suppose I can add platform@0.0.9 dependency in a new MODULE file in this repo against to WORKSPACE (I will try this way).

By the way, it makes sense that this doesn't work in Bazel 7, because the maybe method is described as adding a dependency if it doesn't exist at all:

def maybe(repo_rule, name, **kwargs):
    """Utility function for only adding a repository if it's not already present.
    …

Going further, it might be nice to update emsdk to fully support bzlmod, but maybe not in this MR.

walkingeyerobot commented 3 months ago

Thanks very much! Feel free to send further PRs my way.