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

ts_devserver launcher template is platform specific #1409

Closed
gregmagolan opened this issue Dec 2, 2019 · 6 comments · Fixed by #1413
Closed

ts_devserver launcher template is platform specific #1409

gregmagolan opened this issue Dec 2, 2019 · 6 comments · Fixed by #1413
Assignees

Comments

@gregmagolan
Copy link
Collaborator

    ctx.actions.expand_template(
        template = ctx.file._launcher_template,
        output = ctx.outputs.script,
        substitutions = {
            "TEMPLATED_entry_module": ctx.attr.entry_module,
            "TEMPLATED_main": _to_manifest_path(ctx, ctx.executable.devserver),
            "TEMPLATED_manifest": _to_manifest_path(ctx, ctx.outputs.manifest),
            "TEMPLATED_packages": ",".join(packages.to_list()),
            "TEMPLATED_port": str(ctx.attr.port),
            "TEMPLATED_scripts_manifest": _to_manifest_path(ctx, ctx.outputs.scripts_manifest),
            "TEMPLATED_serving_path": ctx.attr.serving_path if ctx.attr.serving_path else "",
            "TEMPLATED_workspace": workspace_name,
        },
        is_executable = True,
    )

where

        "devserver": attr.label(
            doc = """Go based devserver executable.
            Defaults to precompiled go binary in @npm_bazel_typescript setup by @bazel/typescript npm package""",
            default = Label("//devserver"),
            executable = True,
            cfg = "host",
        ),

if build on linux RBE and then the target is run locally on OSX or Windows it will fail with:

$ yarn bazel run --config=ivy modules/benchmarks/src/tree/ng2:prodserver
yarn run v1.17.3
$ bazel run --config=ivy modules/benchmarks/src/tree/ng2:prodserver
INFO: Writing tracer profile to '/private/var/tmp/_bazel_iminar/599b792019b2af1e8b9ff258eedd177a/command.profile.gz'
INFO: Invocation ID: 94c034ba-3636-48a7-9651-67f8d9eb7d72
DEBUG: /private/var/tmp/_bazel_iminar/599b792019b2af1e8b9ff258eedd177a/external/bazel_toolchains/rules/rbe_repo.bzl:477:5: Bazel 1.1.0 is used in rbe_ubunt
u1604_angular.
INFO: Analyzed target //modules/benchmarks/src/tree/ng2:prodserver (1 packages loaded, 71 targets configured).
INFO: Found 1 target...
Target //modules/benchmarks/src/tree/ng2:prodserver up-to-date:
  dist/bin/modules/benchmarks/src/tree/ng2/prodserver_launcher.sh
  dist/bin/modules/benchmarks/src/tree/ng2/prodserver
INFO: Elapsed time: 7.769s, Critical Path: 0.10s
INFO: 0 processes.
INFO: Build completed successfully, 1 total action
INFO: Build completed successfully, 1 total action
/private/var/tmp/_bazel_iminar/599b792019b2af1e8b9ff258eedd177a/execroot/angular/bazel-out/k8-fastbuild/bin/modules/benchmarks/src/tree/ng2/prodserver: line 52: /private/var/tmp/_bazel_iminar/599b792019b2af1e8b9ff258eedd177a/external/npm_bazel_typescript/devserver/devserver-linux_x64: cannot execute binary file
error Command failed with exit code 126.
@gregmagolan gregmagolan self-assigned this Dec 2, 2019
@gregmagolan
Copy link
Collaborator Author

This is similar to the problem nodejs_binary had with cross-platform RBE in its launcher template. The fix is to choose the executable at runtime in the launcher script.

@alexeagle
Copy link
Collaborator

How about marking this template action as local? It's silly to get a remote worker to do the file io and interpolation

@gregmagolan
Copy link
Collaborator Author

gregmagolan commented Dec 2, 2019

In the case that you want to build & test remotely then it can't be marked local.

gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Dec 3, 2019
…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.
@gregmagolan
Copy link
Collaborator Author

Note. To reproduce this --config=remote on cross-platform RBE (host platform is OSX/Windows and execution platform is linux) is required in the angular repo as the ts_devserver target needs to be built on the linux remote executor and run on the host osx machine.

@alexeagle
Copy link
Collaborator

I don't think ts_devserver needs to support build&test remotely, it's meant to be a local devserver

@gregmagolan
Copy link
Collaborator Author

gregmagolan commented Dec 3, 2019

Its already supported as you can build & run a ts_devserver target on RBE as part of a protractor_web_test.

In the angular repo, ts_devserver is used quite extensively with protractor_web_test as the server attribute. That actually works fine already and has never been an issue as everything happens on the execution platform (even in cross-platform RBE).

Where the angular team is running into issues is with cross-platform RBE:

bazel run  //path/to:devserver --config=remote --cpu=k8 --host_cpu=k8

This is telling bazel to still use RBE for the build but to then run the //path/to:devserver locally.

The developer flow that gets you here is that some bazel test //path/to:protractor_test --config=remote --cpu=k8 --host_cpu=k8 failed that runs //path/to:devserver as its server and you now want to run it locally to debug. If you call,

bazel run //path/to:devserver --config=remote --cpu=k8 --host_cpu=k8

then everything is built on RBE and the resulting target is run locally as desired. However right now, the ts_devserver launcher script is hard-coded to run the linux devserver binary as the launcher script template action is run on RBE.

Note: this looks odd with the k8 cpu flags on OSX but those flags are necessary as cross-platform RBE is not well supported by Bazel yet. They don't have OSX executers on GCP so the Bazel toolchain definitions are incomplete for OSX (@josephperrott can comment more on that as he discussed this with the Bazel team).

Without --cpu=k8 --host_cpu=k8 you can't even get cross-platform RBE off the ground right now. I did start a thread with the Bazel team to get an osx_rbe platform on BazelCI so that rules can start testing against this case but that hasn't happened yet so in the meantime cross-platform RBE is in a hacky state.

A developer could run

bazel run //path/to:devserver

instead and turn off remote execution but that would force them to rebuild the entire repository to debug the failing test as the cache from --config=remote --cpu=k8 --host_cpu=k8 is not used.

As this is a fairly common workflow for angular devs I think we should support it in the current hacky RBE state. If the --cpu=k8 --host_cpu=k8 flags can be dropped in the future when building with RBE on OSX then I think there will be a better solution than the fix in #1409.

gregmagolan added a commit that referenced this issue Dec 4, 2019
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.

Issue for re-visiting this in the future is: Revisit cross-platform RBE mechanics #1415
gregmagolan added a commit to gregmagolan/angular that referenced this issue Dec 5, 2019
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.
AndrewKushnir pushed a commit to angular/angular that referenced this issue Dec 5, 2019
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
AndrewKushnir pushed a commit to angular/angular that referenced this issue Dec 5, 2019
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
josephperrott pushed a commit to josephperrott/angular that referenced this issue Dec 11, 2019
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
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 a pull request may close this issue.

2 participants