refactor: migrate to rules_js #2143
Conversation
|
Hey @thesayyn, @alexeagle, thanks for the PR!!! This is super cool :) Just to set expectations a little bit, and make sure you don't think we're ignoring this PR, we're gonna do some performance and other RBE testing of this vs our existing setup and give it a little time before we decide to merge this. But we're really excited about rules_js and the direction this is going! (@bduffany can help point you at some docs to make sure rbe is working) |
proto/defs.bzl
Outdated
| data = [":" + proto_target] + _PROTOBUFJS_CLI_DEPS, | ||
| srcs = [":" + proto_target], | ||
| copy_srcs_to_bin = False, | ||
| chdir = "../../../", |
There was a problem hiding this comment.
could pass
env = {
"BAZEL_BIN": "."
},
instead of the chdir I believe; tho I think we should add an explicit attribute for this situation such as run_out_of_execroot = True or make it an opt-out run_out_of_bazel_bin = False
| @@ -75,15 +53,17 @@ def ts_proto_library(name, proto, **kwargs): | |||
| "--root=%s" % name, | |||
| "--strict-long", # Force usage of Long type with int64 fields | |||
| "--out=$@", | |||
| "$(execpaths %s)" % proto_target, | |||
| "$(locations %s)" % proto_target, | |||
There was a problem hiding this comment.
why locations? perhaps you want rootpaths?
There was a problem hiding this comment.
It fails for external sources when I use rootpaths for a strange reason.
| "bazel-bin", | ||
| "bazel-out/k8-opt/bin", | ||
| "bazel-out/k8-fastbuild/bin", | ||
| "bazel-out/darwin-opt/bin", |
|
👋 Thanks for kicking this off! The CI test failures (e.g. this one) are a blocker for us, since we want to ensure that our app builds on our RBE service. To build the app on RBE and reproduce those failures, you can try building the app with If you want to dig deeper and debug with a local BB stack (and add print statements to the executor etc.), I can also send over some instructions for doing that. Feel free to also reach out over in the Bazel Slack if it's easier! |
|
rules_js actions are known to not work on RBE due to bazelbuild/bazel#10298 |
|
@alexeagle why does rules_js not work? After a quick reading of the discussion thread on #10298 the only thing I see is the requirement for |
|
Right, it's not just unresolved symlinks that are broken - the RBE protocol doesn't send symlinks to the remote worker at all. They are all resolved to regular files by the time the remote execroot is laid out, which breaks the pnpm package manager (the nodejs require algorithm must walk a symlink into the virtual package store for transitive resolutions to work) |
|
Thanks! I thought that we had fixed that in the past but I apparently remembered it wrong. |
|
Hey @bduffany (and maybe @brentleyjones will be curious) - rules_js can work with RBE now: aspect-build/rules_js#308 (comment) |
|
Line 3 in f6efe09 shouldn't this line be removed? As it is necessary by rules_nodejs only |
|
FYI Bazel 6.0.0-pre.20220922.1 now includes all the symlink fixes. It seems there may be something broken on Buildbuddy with the symlinks however, based on recent reports on Bazel slack |
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
|
@thesayyn Closing this PR and resuming the migration in #2688 I haven't quite managed to get it building yet; see https://bazelbuild.slack.com/archives/CEZUUKQ6P/p1671033211592849 |
Overview of changes;
Version bump: TODO