Add bazel support for binaryen in open source

[mollyibot]

No description provided.

[codecov]

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.

❗ 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]

let's name this wasm-opt

[mollyibot]

when this is pulled to google3 with git_to_third_party, the BUILD file will be inserted:

py_binary(name = "wasm-opt"..)
cc_library(name = "wasm-opt"..)

so I keep this name for avoiding confilict, however I think we can walk around this issue by , would this work for you?

cc_binary(
#copybara:strip_begin
name = "wasm-opt-impl",
#copybara:strip_end_and_replace_begin
# name = "wasm-opt-impl",
#copybara:replace_end
)

[mobileink]

if you put the cc_binary target in src/tools/BUILD.bazel then there is no name clash.

[mollyibot]

when we sync the change to google3 to BUILD file, the BUILD file should stay the same as current BUILD of google3 version, and should the wasm-opt stay in BUILD.bazel of root directory?

[mobileink]

Sorry, I don't understand "sync the change to google3 to BUILD file". But in general there is no reason to put any targets in the root BUILD.bazel. See Packages.

[mollyibot]

The BUILD files refers to "src/support/xxx","src/tools/xxx" and "src/**/*" so I think we should still put it under root directory. We do not want to keep two versions of BUILD files in two different locations between google 3(third_party) and github(source of truth), so we use git_to_third_party and copybara:strip and copybara:insert to resolve the difference. In google3(third party) the build file is in root directory.

[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.

[mobileink]

Might want to use stamping for this. See Stamping Bazel builds with selective delivery

[mollyibot]

Thanks for the link, good to know the timestamp. stamping sometimes may slow git operation. I read the section when to use timestamp and looks like the stamp is not exposed to Bazel rules in any consistent way. here we do not necessarily need to expose the timestamp/commit information to users, I add a dummy value HEAD to mimic https://github.com/WebAssembly/binaryen/blob/main/config.h.in to avoid additional changes to where it is called in https://github.com/WebAssembly/binaryen/blob/main/src/support/command-line.cpp#L18

[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.