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

gregmagolan · 2019-12-03T03:01:26Z

This change allows a ts_devserver target to be build with cross platform RBE (host platform is osx or Windows & execution platform is linux) and still run on the host platform.

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.

BREAKING CHANGE:

@bazel/typescript version needs to be in sync with @build_bazel_rule_nodejs for this fix so this is a breaking change between the two release artifacts. Next release should be a bump on the minor version (breaking change for 0.x.x in semver).

PR Checklist

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature (please, look at the "Scope of the project" section in the README.md file)
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

Yes
No

@bazel/typescript version needs to be in sync with @build_bazel_rule_nodejs for this fix so this is a breaking change between the two release artifacts.

Other information

…rib#1409 This change allows a ts_devserver target to be build with cross platform RBE (host platform is osx or Windows & execution platform is linux) and still run on the host platform. 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.

alexeagle

Can't we just tag the action that generates the script with "local" so it doesn't try to run it on RBE? Seems unhelpful to have a remote machine interpolate some strings, it's just I/O bound on the upload/download

alexeagle

feels like the core issue is that we are doing something complex here where we should do something simple instead. All of this is just to serve a bit of bundled JS to angular protractor tests. Surely we could just switch them to http_server for nearly all these use cases?

alexeagle · 2019-12-03T22:53:48Z

packages/typescript/src/internal/devserver/ts_devserver.bzl

+        "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),


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

alexeagle · 2019-12-03T22:55:32Z

packages/typescript/src/internal/devserver/ts_devserver.bzl

    devserver_runfiles = [
        ctx.executable.devserver,
+        ctx.executable.devserver_host,


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

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.

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

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

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

alexeagle · 2019-12-03T22:56:19Z

packages/typescript/src/internal/devserver/launcher_template.sh

@@ -42,8 +42,45 @@ else
 fi
 # --- end runfiles.bash initialization ---

+# Check environment for which node path to use


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

Oh, sorry. I commented at the wrong place.

alexeagle

Not really excited about the hacks here but it seems we have to get some fixes in RBE itself to do it correctly, and we think Angular team treats this as a P1 so we can merge and come back to it maybe

gregmagolan · 2019-12-04T17:09:11Z

@alexeagle Created Revisit cross-platform RBE mechanics #1415. .Will add more details about how it is currently working and how it should work.

This release brings two bug fixes we're waiting on: 1) fix(builtin): additional_root_paths in pkg_web should also include paths in genfiles and bin dirs (bazel-contrib/rules_nodejs#1402), which is a pre-req for angular#34112 2) fix(typescript): fix for cross platform ts_devserver issue angular#1409 (bazel-contrib/rules_nodejs#1413) which resolves ts_devserver launcher template is platform specific angular#1409 (bazel-contrib/rules_nodejs#1409) --- this fixes an OSX developer workflow with --config=remote This does not upgrade integration/bazel or integation/schematics. That will be done in another PR.

This release brings two bug fixes we're waiting on: 1) fix(builtin): additional_root_paths in pkg_web should also include paths in genfiles and bin dirs (bazel-contrib/rules_nodejs#1402), which is a pre-req for #34112 2) fix(typescript): fix for cross platform ts_devserver issue #1409 (bazel-contrib/rules_nodejs#1413) which resolves ts_devserver launcher template is platform specific #1409 (bazel-contrib/rules_nodejs#1409) --- this fixes an OSX developer workflow with --config=remote This does not upgrade integration/bazel or integation/schematics. That will be done in another PR. PR Close #34243

This release brings two bug fixes we're waiting on: 1) fix(builtin): additional_root_paths in pkg_web should also include paths in genfiles and bin dirs (bazel-contrib/rules_nodejs#1402), which is a pre-req for angular#34112 2) fix(typescript): fix for cross platform ts_devserver issue angular#1409 (bazel-contrib/rules_nodejs#1413) which resolves ts_devserver launcher template is platform specific angular#1409 (bazel-contrib/rules_nodejs#1409) --- this fixes an OSX developer workflow with --config=remote This does not upgrade integration/bazel or integation/schematics. That will be done in another PR. PR Close angular#34243

gregmagolan requested review from alexeagle and soldair as code owners December 3, 2019 03:01

googlebot added the cla: yes label Dec 3, 2019

alexeagle reviewed Dec 3, 2019

View reviewed changes

alexeagle approved these changes Dec 4, 2019

View reviewed changes

gregmagolan merged commit 172caff into bazel-contrib:master Dec 4, 2019

gregmagolan mentioned this pull request Dec 5, 2019

build: update to rules_nodejs 0.42.2 angular/angular#34243

Closed

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gregmagolan commented Dec 3, 2019

alexeagle left a comment

alexeagle left a comment

alexeagle Dec 3, 2019

alexeagle Dec 3, 2019

meteorcloudy Dec 4, 2019

alexeagle Dec 4, 2019

meteorcloudy Dec 4, 2019

alexeagle Dec 4, 2019

alexeagle Dec 3, 2019

This comment was marked as spam.

meteorcloudy Dec 4, 2019

alexeagle left a comment

gregmagolan commented Dec 4, 2019

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

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

Conversation

gregmagolan commented Dec 3, 2019

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

alexeagle left a comment

Choose a reason for hiding this comment

alexeagle left a 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

This comment was marked as spam.

Choose a reason for hiding this comment

alexeagle left a comment

Choose a reason for hiding this comment

gregmagolan commented Dec 4, 2019