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.

Sign up for GitHub

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

fix(typescript): fix for cross platform ts_devserver issue #1409 #1413

Merged
merged 1 commit into from
Dec 4, 2019
Merged

fix(typescript): fix for cross platform ts_devserver issue #1409 #1413

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions internal/node/node_repositories.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -680,8 +680,14 @@ alias(name = "yarn_node_repositories", actual = "{node_repository}//:yarn_node_r
alias(name = "node_files", actual = "{node_repository}//:node_files")
alias(name = "yarn_files", actual = "{node_repository}//:yarn_files")
alias(name = "npm_files", actual = "{node_repository}//:npm_files")
exports_files(["index.bzl"])
""".format(node_repository = "@nodejs_%s" % os_name(repository_ctx)))

# index.bzl file for this repository
repository_ctx.file("index.bzl", content = """# Generated by node_repositories.bzl
host_platform="{host_platform}"
""".format(host_platform = os_name(repository_ctx)))

_nodejs_repo_host_os_alias = repository_rule(_nodejs_host_os_alias_impl)

def node_repositories(**kwargs):
Expand Down
9 changes: 9 additions & 0 deletions packages/typescript/docs/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,21 @@
# See the License for the specific language governing permissions and
# limitations under the License.

load("@bazel_skylib//:bzl_library.bzl", "bzl_library")
load("@build_bazel_rules_nodejs//:index.bzl", "nodejs_test")
load("//tools/stardoc:index.bzl", "stardoc")

package(default_visibility = ["//visibility:public"])

exports_files(["install.md"])

# Work-around since we don't have and don't wnat a bzl_library in the generated
# @nodejs//:BUILD.bazel file
bzl_library(
name = "nodejs_bzl",
srcs = ["@nodejs//:index.bzl"],
)

stardoc(
name = "docs",
out = "index.md",
Expand All @@ -37,6 +45,7 @@ stardoc(
"//internal/js_library:bzl",
"//internal/node:bzl",
"//internal/pkg_web:bzl",
":nodejs_bzl",
],
)

Expand Down
30 changes: 27 additions & 3 deletions packages/typescript/src/devserver/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,30 @@
# See the License for the specific language governing permissions and
# limitations under the License.

filegroup(
name = "devserver_darwin_amd64",
srcs = ["devserver-darwin_x64"],
# Don't build on CI
tags = ["manual"],
visibility = ["//visibility:public"],
)

filegroup(
name = "devserver_linux_amd64",
srcs = ["devserver-linux_x64"],
# Don't build on CI
tags = ["manual"],
visibility = ["//visibility:public"],
)

filegroup(
name = "devserver_windows_amd64",
srcs = ["devserver-windows_x64.exe"],
# Don't build on CI
tags = ["manual"],
visibility = ["//visibility:public"],
)

config_setting(
name = "darwin_x64",
constraint_values = [
Expand Down Expand Up @@ -39,9 +63,9 @@ config_setting(
filegroup(
name = "devserver",
srcs = select({
":darwin_x64": ["devserver-darwin_x64"],
":linux_x64": ["devserver-linux_x64"],
":windows_x64": ["devserver-windows_x64.exe"],
":darwin_x64": [":devserver_darwin_amd64"],
":linux_x64": [":devserver_linux_amd64"],
":windows_x64": [":devserver_windows_amd64"],
}),
# Don't build on CI
tags = ["manual"],
Expand Down
1 change: 1 addition & 0 deletions packages/typescript/src/index.from_src.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ load(
def ts_devserver(**kwargs):
_ts_devserver(
devserver = "@build_bazel_rules_typescript//devserver:devserver_bin",
devserver_host = "@build_bazel_rules_typescript//devserver:devserver_bin",
**kwargs
)

Expand Down
39 changes: 38 additions & 1 deletion packages/typescript/src/internal/devserver/launcher_template.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,45 @@ else
fi
# --- end runfiles.bash initialization ---

# Check environment for which node path to use
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we could key this off the path to node instead?

This comment was marked as spam.

Sign in to view

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry. I commented at the wrong place.

unameOut="$(uname -s)"
case "${unameOut}" in
Linux*) machine=linux ;;
Darwin*) machine=darwin ;;
CYGWIN*) machine=windows ;;
MINGW*) machine=windows ;;
MSYS_NT*) machine=windows ;;
*) machine=linux
printf "\nUnrecongized uname '${unameOut}'; defaulting to use node for linux.\n" >&2
printf "Please file an issue to https://github.com/bazelbuild/rules_nodejs/issues if \n" >&2
printf "you would like to add your platform to the supported ts_devserver platforms.\n\n" >&2
;;
esac

case "${machine}" in
# The following paths must match up with @npm_bazel_typescript//devserver binaries
darwin) readonly platform_main_manifest="npm_bazel_typescript/devserver/devserver-darwin_x64" ;;
windows) readonly platform_main_manifest="npm_bazel_typescript/devserver/devserver-windows_x64.exe" ;;
*) readonly platform_main_manifest="npm_bazel_typescript/devserver/devserver-linux_x64" ;;
esac

readonly platform_main=$(rlocation "${platform_main_manifest}")

if [ -f "${platform_main}" ]; then
readonly main=${platform_main}
else
# If the devserver binary is overridden then use the templated binary
readonly main=$(rlocation "TEMPLATED_main")
fi

if [ ! -f "${main}" ]; then
printf "\n>>>> FAIL: The ts_devserver binary '${main_platform}' not found in runfiles.\n" >&2
printf "This node toolchain was chosen based on your uname '${unameOut}'.\n" >&2
printf "Please file an issue to https://github.com/bazelbuild/rules_nodejs/issues if \n" >&2
printf "you would like to add your platform to the supported ts_devserver platforms. <<<<\n\n" >&2
exit 1
fi

readonly main=$(rlocation "TEMPLATED_main")
readonly manifest=$(rlocation "TEMPLATED_manifest")
readonly scripts_manifest=$(rlocation "TEMPLATED_scripts_manifest")

Expand Down
21 changes: 21 additions & 0 deletions packages/typescript/src/internal/devserver/ts_devserver.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ load(
"@build_bazel_rules_nodejs//internal/js_library:js_library.bzl",
"write_amd_names_shim",
)
load("@nodejs//:index.bzl", "host_platform")

# Avoid using non-normalized paths (workspace/../other_workspace/path)
def _to_manifest_path(ctx, file):
Expand Down Expand Up @@ -86,8 +87,14 @@ def _ts_devserver(ctx):
for f in script_files
]))

# With cross-platform RBE for OSX & Windows ctx.executable.devserver will be linux as --cpu and
# --host_cpu must be overridden to k8. However, we still want to be able to run the devserver on the host
# machine so we need to include the host devserver binary, which is ctx.executable.devserver_host, in the
# runfiles. For non-RBE and for RBE with a linux host, ctx.executable.devserver & ctx.executable.devserver_host
# will be the same binary.
devserver_runfiles = [
ctx.executable.devserver,
ctx.executable.devserver_host,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feels really gross to upload both binaries to RBE just because it can't select the right one. This is a limitation of cross-platform RBE being broken? Still feels like a hacky workaround

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you mean this one: https://github.com/bazelbuild/rules_nodejs/blob/dcdc98b093edf2a3ccd6962d8b4f15aa28ba6fa8/internal/common/windows_utils.bzl#L63

It's basically a hack to make the code simpler, but it should work.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm we also need a way to distinguish mac from linux, do you know of any trick to do that?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, maybe you can consider this:

Adding the following to .bazelrc

--enable_platform_specific_config
build:windows --action_env=NODE_PLATFORM=windows
build:macos --action_env=NODE_PLATFORM=macos
build:linux --action_env=NODE_PLATFORM=linux

Then you can check the value of ctx.configuration.default_shell_env['NODE_PLATFORM']

Doc for --enable_platform_specific_config

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's clever but we can't control users' .bazelrc files so this will get us in trouble

ctx.outputs.manifest,
ctx.outputs.scripts_manifest,
]
Expand Down Expand Up @@ -138,11 +145,25 @@ ts_devserver = rule(
),
"devserver": attr.label(
doc = """Go based devserver executable.

With cross-platform RBE for OSX & Windows ctx.executable.devserver will be linux as --cpu and
--host_cpu must be overridden to k8. However, we still want to be able to run the devserver on the host
machine so we need to include the host devserver binary, which is ctx.executable.devserver_host, in the
runfiles. For non-RBE and for RBE with a linux host, ctx.executable.devserver & ctx.executable.devserver_host
will be the same binary.

Defaults to precompiled go binary in @npm_bazel_typescript setup by @bazel/typescript npm package""",
default = Label("//devserver"),
executable = True,
cfg = "host",
),
"devserver_host": attr.label(
doc = """Go based devserver executable for the host platform.
Defaults to precompiled go binary in @npm_bazel_typescript setup by @bazel/typescript npm package""",
default = Label("//devserver:devserver_%s" % host_platform),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there some other way we could get this info so we don't need to create the new index.bzl file and make the rollout complicated? @meteorcloudy maybe you have some idea - I know there's a starlark trick to know if we are on Windows or not

executable = True,
cfg = "host",
),
"entry_module": attr.string(
doc = """The `entry_module` should be the AMD module name of the entry module such as `"__main__/src/index".`
`ts_devserver` concats the following snippet after the bundle to load the application:
Expand Down