Add bazel support for binaryen in open source

[mollyibot]

[codecov[bot]]

Codecov Report

Merging #5925 (74d70f3) into main (e9d0fb7) will increase coverage by 0.01%. Report is 1 commits behind head on main. The diff coverage is n/a.

:exclamation: Current head 74d70f3 differs from pull request most recent head 2bfd59a. Consider uploading reports for the commit 2bfd59a to get more accurate results

@@            Coverage Diff             @@
##             main    #5925      +/-   ##
==========================================
+ Coverage   42.60%   42.61%   +0.01%     
==========================================
  Files         484      484              
  Lines       74837    74843       +6     
  Branches    11922    11923       +1     
==========================================
+ Hits        31887    31898      +11     
+ Misses      39750    39747       -3     
+ Partials     3200     3198       -2     

see 10 files with indirect coverage changes

[walkingeyerobot]

Fixes #5902

[mobileink]

Strong recommendation: always use BUILD.bazel, not plain BUILD - avoid any possible name clash with other tools. Ditto for WORKSPACE.bazel (althought that file will be going way with 7.0)

[mobileink]

Second strong recommendation: you might as well start with Bazel modules. It's vastly superior to WORKSPACE-based dependency management. See my fork for an example.

[mobileink]

Recommendation: follow Bazel best practices; in particular, avoid toplevel build targets like bazel build :foo. Also prefer one BUILD.bazel file per source directory. Better to avoid having file paths like src/tools/wasmjs.cpp etc. in srcs. All those cc_binary targets can easily go in src/tools/BUILD.bazel, so you get build/run targets like bazel build src/tools:wasm-js. Bazel labels make this easy and convenient.

That also makes the build less complex since things are localized - a bunch of small, simple, local BUILD.bazel files instead of one monster file with everything.

[mobileink]

Just for fun I wrote a very simple C program that generates src/passes/WasmIntrinsics.cpp directly from src/passes/wasm-intrinsics.wat. No need for a template file, and this also eliminates the need to pull in additional Python resources (absl-py).

Feel free to use it if you like, its at https://github.com/obazl/binaryen/blob/bazel/src/tools/textfile_to_binary.c . To use it, there is a custom rule in src/passes/BUILD.bzl, used in target //src/passes:wasm_intrinsics.

[mollyibot]

Just for fun I wrote a very simple C program that generates src/passes/WasmIntrinsics.cpp directly from src/passes/wasm-intrinsics.wat. No need for a template file, and this also eliminates the need to pull in additional Python resources (absl-py).

Feel free to use it if you like, its at https://github.com/obazl/binaryen/blob/bazel/src/tools/textfile_to_binary.c . To use it, there is a custom rule in src/passes/BUILD.bzl, used in target //src/passes:wasm_intrinsics.

I am not too familiar with c++(can only read :/), maybe this can be a follow up? from our internal discussion, we want to use the one version of py_script between opensource and google3 for now. if you want to get rid of pyscript and related dependencies, maybe you can raise a pr after this request and this may requires another round of review( to keep google3 and github synced) and it is a bit of out of j2cl team's scope.

[kripken]

maybe this can be a follow up?

That makes sense to me. I suggest we do the simplest thing here for now, since eventually I think Binaryen can actually remove the need for any build integration for that file (we could handle it the same way we handle gen-s-parser.py for example).

[walkingeyerobot]

Just for fun I wrote a very simple C program that generates src/passes/WasmIntrinsics.cpp directly from src/passes/wasm-intrinsics.wat. No need for a template file, and this also eliminates the need to pull in additional Python resources (absl-py).

Feel free to use it if you like, its at https://github.com/obazl/binaryen/blob/bazel/src/tools/textfile_to_binary.c . To use it, there is a custom rule in src/passes/BUILD.bzl, used in target //src/passes:wasm_intrinsics.

This is perfectly portable across all bazel platforms. I would be happy including either this or a python script that does the same. Either way we definitely want to avoid any bash to maintain portability.

[walkingeyerobot]

Recommendation: follow Bazel best practices; in particular, avoid toplevel build targets like bazel build :foo. Also prefer one BUILD.bazel file per source directory. Better to avoid having file paths like src/tools/wasmjs.cpp etc. in srcs. All those cc_binary targets can easily go in src/tools/BUILD.bazel, so you get build/run targets like bazel build src/tools:wasm-js. Bazel labels make this easy and convenient.

That also makes the build less complex since things are localized - a bunch of small, simple, local BUILD.bazel files instead of one monster file with everything.

@mollyibot I would be in favor of following this approach. I recommend using @mobileink 's fork as an example over exists in google3. Once we have something working in github, we can then work to translate it into something working in google3.

[gkdn]

@walkingeyerobot Although I agree that more granular targets are better, I think we should try to keep this PR minimal and follow what we had in google3. I think all of this could be refactored later on as we have a setup in place and internal/external stuff becomes in sync. We don't really need to target the ideal scenario in the initial setup.

Similarly I recommend sticking to BUILD instead of BUILD.bazel since these build files intended to be shared with Blaze (which doesn't support later) and that's what we do in other projects. Again this is not a breaking change to do later if becomes a necessity.

[walkingeyerobot]

@walkingeyerobot Although I agree that more granular targets are better, I think we should try to keep this PR minimal and follow what we had in google3. I think all of this could be refactored later on as we have a setup in place and internal/external stuff becomes in sync. We don't really need to target the ideal scenario in the initial setup.

I am fine with doing this later, but I would like to get some commitment on this. For example, if we merge this PR and a community member submits a follow-up PR to make more granular targets, I would be inclined to merge that as well. At that point, would you or @mollyibot be able to commit to making the appropriate changes internally? I suspect it would be less work to simply do it correctly from the beginning, especially given we already have @mobileink 's fork as an example.

Similarly I recommend sticking to BUILD instead of BUILD.bazel since these build files intended to be shared with Blaze (which doesn't support later) and that's what we do in other projects. Again this is not a breaking change to do later if becomes a necessity.

I think renaming the file is a fairly straightforward transformation we can take on internally. I'll cite absl as a project that does this. See this BUILD.bazel file as an example. We should strive to be the best open source citizens that we can be.

[gkdn]

@walkingeyerobot Although I agree that more granular targets are better, I think we should try to keep this PR minimal and follow what we had in google3. I think all of this could be refactored later on as we have a setup in place and internal/external stuff becomes in sync. We don't really need to target the ideal scenario in the initial setup.

I am fine with doing this later, but I would like to get some commitment on this. For example, if we merge this PR and a community member submits a follow-up PR to make more granular targets, I would be inclined to merge that as well. At that point, would you or @mollyibot be able to commit to making the appropriate changes internally? I suspect it would be less work to simply do it correctly from the beginning, especially given we already have @mobileink 's fork as an example.

The way I was thinking about this was;

If it is easy (my anticipation), it can be done as a follow up and wouldn't require our involvement and it would be still good to start with something close to google3 instead of bundling 2 refactorings.

And if it gets involved, then that decision should be done and executed separate from this and we wouldn't be right folks to take that over.

Does it sound reasonable?

Similarly I recommend sticking to BUILD instead of BUILD.bazel since these build files intended to be shared with Blaze (which doesn't support later) and that's what we do in other projects. Again this is not a breaking change to do later if becomes a necessity.

I think renaming the file is a fairly straightforward transformation we can take on internally. I'll cite absl as a project that does this. See this BUILD.bazel file as an example. We should strive to be the best open source citizens that we can be.

If you already follow that convention in other places and handle it via transformations then being consistent with that sounds good to me.