Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bazel] Use py_binary for emcc #1422

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

allsey87
Copy link
Contributor

@allsey87 allsey87 commented Jul 4, 2024

This PR was an investigation into clean up the Emscripten toolchain in Bazel using py_binary to wrap and run emcc and emar etc. This would have been nice since I think it would have allowed us to drop all the sh/bat scripts (and OS detection logic around it) that just call the Python interpreter anyway. The motivation for this is #1420 and I was hoping that by wrapping emcc with py_binary that the paths/modules/libraries would have been a bit more robust to whatever is going wrong in this issue.

Unfortunately I hit the roadblock described in this SO question that cannot be worked around without a hack. The main blocker is bazelbuild/bazel#17629, however, if there is interest in this approach, we could conditionally apply a genrule on Linux and Mac to add in the missing shebang as suggested in bazelbuild/bazel#17629 (comment).

What do you think @sbc100 and @walkingeyerobot? Do you think this is worth pursuing further? Incidentally, this PR would make #1333 redundant in my opinion.

@allsey87
Copy link
Contributor Author

allsey87 commented Jul 4, 2024

Referring to the CI results, on Linux and Mac this currently fails as described in the SO question and as follows:

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
Traceback (most recent call last):
  File "bazel-out/k8-opt-exec-ST-d57f47055a04/bin/emscripten_toolchain/emcc_compile", line 559, in <module>
    Main()
  File "bazel-out/k8-opt-exec-ST-d57f47055a04/bin/emscripten_toolchain/emcc_compile", line 457, in Main
    module_space = FindModuleSpace(main_rel_path)
  File "bazel-out/k8-opt-exec-ST-d57f47055a04/bin/emscripten_toolchain/emcc_compile", line 172, in FindModuleSpace
    raise AssertionError('Cannot find .runfiles directory for %s' % sys.argv[0])
AssertionError: Cannot find .runfiles directory for bazel-out/k8-opt-exec-ST-d57f47055a04/bin/emscripten_toolchain/emcc_compile
Target //hello-world:hello-world-wasm failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 47.031s, Critical Path: 0.11s
INFO: 9 processes: 9 internal.
ERROR: Build did NOT complete successfully

Interestingly enough, it gets further on Windows and actually starts emcc.py but then fails since I haven't added the required deps and data to the py_binary yet. Although this could be a quirk of using a very old Bazel version on Windows (5.4.0).

ERROR: C:/users/circleci/project/bazel/hello-world/BUILD:4:10: Compiling hello-world/hello-world.cc failed: (Exit 1): emcc_compile.exe failed: error executing command bazel-out\x64_windows-opt-exec-2B5CBBC6-ST-4a8fd3122b04\bin\emscripten_toolchain\emcc_compile.exe @bazel-out/wasm-fastbuild-ST-7eb0ea6b7782/bin/hello-world/_objs/hello-world/hello-world.o.params
Traceback (most recent call last):
  File "\\?\C:\Users\circleci\AppData\Local\Temp\Bazel.runfiles_mbfhpz1e\runfiles\emscripten_bin_linux\emscripten\emcc.py", line 23, in <module>
    from tools.toolchain_profiler import ToolchainProfiler
ModuleNotFoundError: No module named 'tools'
Target //hello-world:hello-world-wasm failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 91.945s, Critical Path: 0.54s
INFO: 11 processes: 9 internal, 2 local.
FAILED: Build did NOT complete successfully
FAILED: Build did NOT complete successfully

@sbc100
Copy link
Collaborator

sbc100 commented Jul 5, 2024

py_binary seems like it might be little overkill for this situation, but if it works I won't object. I assume there is an easier easy to get a hermetic python runtime and run a script without having to package that script as a py_binary/

@allsey87
Copy link
Contributor Author

allsey87 commented Jul 5, 2024

py_binary seems like it might be little overkill for this situation, but if it works I won't object. I assume there is an easier easy to get a hermetic python runtime and run a script without having to package that script as a py_binary/

Well, I've dug pretty deep into Bazel over the past weeks and while I could be mistaking, I am under the impression that what we are trying to do with emcc etc is actually one of the primary use cases for py_binary...

@sbc100
Copy link
Collaborator

sbc100 commented Jul 5, 2024

py_binary seems like it might be little overkill for this situation, but if it works I won't object. I assume there is an easier easy to get a hermetic python runtime and run a script without having to package that script as a py_binary/

Well, I've dug pretty deep into Bazel over the past weeks and while I could be mistaking, I am under the impression that what we are trying to do with emcc etc is actually one of the primary use cases for py_binary...

Fair enough. Sounds like you know way more than me at this point about this stuff :)

I though that py_binary was for producing toolchain outputs, but if it also makes sense to managing build tools within the toolchain itself then that makes sense to me.

@allsey87
Copy link
Contributor Author

allsey87 commented Jul 5, 2024

I though that py_binary was for producing toolchain outputs, but if it also makes sense to managing build tools within the toolchain itself then that makes sense to me.

You are not wrong, it is for both. Ironically it seems, however, that people actually run into more problems using py_binary externally (i.e., as an output of Bazel) than using it internally as a tool or via bazel run.

@logankaser
Copy link

This would also resolve #1263

@walkingeyerobot
Copy link
Collaborator

I think this is a more complex problem to solve than it appears. I don't think it's worth it to spend time on this, but if you can get it working I'd be inclined to accept it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants