Improve bazel example #776

jinfwhuang · 2021-03-27T15:47:33Z

Added emscripten bindings to bazel examples.

jinfwhuang · 2021-03-27T15:48:17Z

bazel/README.md

@@ -25,13 +25,16 @@ emsdk_emscripten_deps()

 Put the following lines into your `.bazelrc`:
 ```
-build:wasm --crosstool_top=//emscripten_toolchain:everything
+build:wasm --crosstool_top=@emsdk//emscripten_toolchain:everything


This was a bug.

jinfwhuang · 2021-03-27T15:49:40Z

bazel/test_external/BUILD

@@ -3,6 +3,9 @@ load("@emsdk//emscripten_toolchain:wasm_rules.bzl", "wasm_cc_binary")
 cc_binary(
    name = "hello-world",
    srcs = ["hello-world.cc"],
+    linkopts = [
+        "--bind",


Required for embind.

https://emscripten.org/docs/porting/connecting_cpp_and_javascript/embind.html

Why is this not formatted on one line like srcs above? Is this the bazel style?

linkopts = ["--bind"],

works as well. Both comply with buildifier.

sbc100 · 2021-03-28T15:27:21Z

bazel/test_external/.gitignore

@@ -0,0 +1,2 @@
+bazel-*
+dist/


missing newline

sbc100 · 2021-03-28T15:29:39Z

bazel/test_external/hello-world.cc

-  std::cout << "hello world!" << std::endl;
+  std::cout << "start main function" << std::endl;
+  sayHello();
+  std::cout << "end main function" << std::endl;


Any not include a main function in the program in both cases? This change seem to turn this example from the executable into a library. Any particular reason for doing that?

Don't we actually run the result of building this? (under node?) We probably should.

Both are fine. The example illustrates that we can use EMSCRIPTEN_BINDINGS optionally.

I updated it so it looks more like what you are looking for.

sbc100 · 2021-03-28T15:30:20Z

bazel/test_external/BUILD

@@ -3,6 +3,9 @@ load("@emsdk//emscripten_toolchain:wasm_rules.bzl", "wasm_cc_binary")
 cc_binary(
    name = "hello-world",
    srcs = ["hello-world.cc"],
+    linkopts = [
+        "--bind",


Why is this not formatted on one line like srcs above? Is this the bazel style?

walkingeyerobot · 2021-03-29T18:08:47Z

Thanks for the PR! I think the changes to bazel/README.md are wonderful and should be merged.

The rest I'm more reluctant to accept. I don't think there's value in adding embind to the hello world example, or in removing the main function when building under emscripten. The addition of bazel/test_external/.bazelrc isn't necessary for the example, and it's already covered by bazel/bazelrc and explained in bazel/README.md. I'd also prefer having a single README instead of multiple in nested directories because there's not so much information that it needs to be split up, and it's easier to maintain with only one.

Again, thank you for your contribution. I'm happy to merge the changes to bazel/README.md if you'd like to revert the rest. I'm also happy to have further discussion concerning the other changes.

jinfwhuang · 2021-03-29T19:24:50Z

Sure. The key addition to the example is that it demonstrates how to pass opt flag to the compiler to bind c functions to javascript. These should be helpful addition to the example for a majority of the users.

I highly question that this would make this more difficult to maintain.

Without the .bazelrc, the command bazel build --config=wasm :hello-world would not work.

jinfwhuang · 2021-03-29T19:36:04Z

Remove test_external/README.md

jinfwhuang · 2021-03-29T19:36:49Z

bazel/test_external/.bazelrc

@@ -0,0 +1,3 @@
+build:wasm --crosstool_top=@emsdk//emscripten_toolchain:everything


without these, the example would not work for :hello-world.

I think it's ok for the example to not work with --config=wasm right out of the box; we'd rather promote the use of wasm_cc_binary as it produces more useful output anyway. Also, the readme covers adding the appropriate lines to your own .bazelrc file, which would enable --config=wasm builds to function. The big thing here I want to avoid is having this file be identical to bazel/bazelrc and having to keep them both in sync.

sbc100 · 2021-03-30T14:41:17Z

Sure. The key addition to the example is that it demonstrates how to pass opt flag to the compiler to bind c functions to javascript. These should be helpful addition to the example for a majority of the users.

I highly question that this would make this more difficult to maintain.

Without the .bazelrc, the command bazel build --config=wasm :hello-world would not work.

What about keeping hello world as is and adding a new test (hello_embind or test_embind?) that adds this more complex stuff?

jinfwhuang · 2021-03-30T15:16:02Z

Sure. Removed all changes to the example.

walkingeyerobot · 2021-03-31T18:14:40Z

Sure. The key addition to the example is that it demonstrates how to pass opt flag to the compiler to bind c functions to javascript. These should be helpful addition to the example for a majority of the users.

Passing linkopts using bazel is not unique to this toolchain. This is something bazel documentation covers just fine. Similarly, using embind with and without this toolchain is the same, and emscripten's embind documentation covers how to use this.

I highly question that this would make this more difficult to maintain.

The more duplication of documentation there is, the more places that need to be updated when something changes, and the more likely it is that they become out of sync, confusing users. I would be in favor of linking to other documentation instead.

jinfwhuang · 2021-03-31T20:23:57Z

Sure. The key addition to the example is that it demonstrates how to pass opt flag to the compiler to bind c functions to javascript. These should be helpful addition to the example for a majority of the users.

Passing linkopts using bazel is not unique to this toolchain. This is something bazel documentation covers just fine. Similarly, using embind with and without this toolchain is the same, and emscripten's embind documentation covers how to use this.

I highly question that this would make this more difficult to maintain.

The more duplication of documentation there is, the more places that need to be updated when something changes, and the more likely it is that they become out of sync, confusing users. I would be in favor of linking to other documentation instead.

@walkingeyerobot So you want to keep the example to be broken with respect to with the example hello-world to stay broken?

jinfwhuang · 2021-03-31T20:25:08Z

I already removed all changes to the example. The changes remained:

Updated README b/c there was a typo
Updated bazelrc file so that the example actually run.
gitignore auto generated files when the example is run.

walkingeyerobot · 2021-03-31T20:26:44Z

Sure. The key addition to the example is that it demonstrates how to pass opt flag to the compiler to bind c functions to javascript. These should be helpful addition to the example for a majority of the users.

Passing linkopts using bazel is not unique to this toolchain. This is something bazel documentation covers just fine. Similarly, using embind with and without this toolchain is the same, and emscripten's embind documentation covers how to use this.

I highly question that this would make this more difficult to maintain.

The more duplication of documentation there is, the more places that need to be updated when something changes, and the more likely it is that they become out of sync, confusing users. I would be in favor of linking to other documentation instead.

@walkingeyerobot So you want to keep the example to be broken with respect to with the example hello-world to stay broken?

The example is not broken, but I would like to keep it the way it is.. The user can either build the wasm_cc_binary as-is, or they can follow the directions in the readme to build with --config=wasm.

jinfwhuang · 2021-03-31T21:54:23Z

The issue under discussions are so minor. Please feel free to push your modifications to the PR as you see necessary.

Improve bazel example

9784c23

jinfwhuang commented Mar 27, 2021

View reviewed changes

walkingeyerobot self-requested a review March 27, 2021 19:51

sbc100 reviewed Mar 28, 2021

View reviewed changes

minor fixes

b892f7d

jinfwhuang commented Mar 29, 2021

View reviewed changes

Remove some unwanted changes

d496965

jinfwhuang force-pushed the improve-bazel-example branch from 7d53b20 to d496965 Compare March 30, 2021 15:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve bazel example #776

Improve bazel example #776

jinfwhuang commented Mar 27, 2021

jinfwhuang Mar 27, 2021

jinfwhuang Mar 27, 2021

sbc100 Mar 28, 2021

jinfwhuang Mar 29, 2021

sbc100 Mar 28, 2021

sbc100 Mar 28, 2021

jinfwhuang Mar 29, 2021

jinfwhuang Mar 29, 2021

sbc100 Mar 28, 2021

walkingeyerobot commented Mar 29, 2021

jinfwhuang commented Mar 29, 2021 •

edited

jinfwhuang commented Mar 29, 2021

jinfwhuang Mar 29, 2021

walkingeyerobot Mar 31, 2021

sbc100 commented Mar 30, 2021

jinfwhuang commented Mar 30, 2021 •

edited

walkingeyerobot commented Mar 31, 2021

jinfwhuang commented Mar 31, 2021

jinfwhuang commented Mar 31, 2021 •

edited

walkingeyerobot commented Mar 31, 2021

jinfwhuang commented Mar 31, 2021

		@@ -0,0 +1,3 @@
		build:wasm --crosstool_top=@emsdk//emscripten_toolchain:everything

		@@ -0,0 +1,2 @@
		bazel-*
		dist/

Improve bazel example #776

Are you sure you want to change the base?

Improve bazel example #776

Conversation

jinfwhuang commented Mar 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

walkingeyerobot commented Mar 29, 2021

jinfwhuang commented Mar 29, 2021 • edited

jinfwhuang commented Mar 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbc100 commented Mar 30, 2021

jinfwhuang commented Mar 30, 2021 • edited

walkingeyerobot commented Mar 31, 2021

jinfwhuang commented Mar 31, 2021

jinfwhuang commented Mar 31, 2021 • edited

walkingeyerobot commented Mar 31, 2021

jinfwhuang commented Mar 31, 2021

jinfwhuang commented Mar 29, 2021 •

edited

jinfwhuang commented Mar 30, 2021 •

edited

jinfwhuang commented Mar 31, 2021 •

edited