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

fix(bazel): replay compilation uses wrong compiler for building esm5 #28053

Closed
wants to merge 3 commits into
base: master
from

Conversation

@devversion
Copy link
Member

devversion commented Jan 10, 2019

What's the issue?

With the update to TypeScript 3.2.x, a big issue seems to have appeared for downstream Bazel users. If the downstream user still uses a lower TypeScript version, normal Bazel targets using the ng_module rule are still compiled with the expected TypeScript version (assuming they set the node_modules attribute properly).

But, if they build the previous Bazel targets by specifying them within a ng_package rule, the TypeScript version from the Angular workspace is being used for the replayed ESM5 compilation.

Why is this?

This is because we resolve the replay compiler to ngc_wrapped or tsc_wrapped Bazel executables which are defined as part of the angular workspace. This means that the compilers are different if the downstream user uses ngc-wrapped from the @npm repository because the replayed compilation would use the compiler with @ngdeps//typescript.

How can we fix it

In order to fix this, we should just use the compiler that is defined in the @angular//BUILD.bazel file. This target by defaults to the "@npm" workspace which is working for downstream users. This is similar to how it is handled for tsc-wrapped. tsc-wrapped works as expected for downstream users.

Future things to explore
Note that in general the way we use the "replay_compiler" is not perfect because ideally we would completely respect the compiler option from the base ng_module, but this is not possible in a hermetic way, unless we somehow accept the compiler as an attribute that builds all transitive deps. This is something we should explore in the future. For now, we just fix this in a reasonable way that is also used for tsc_wrapped from the TypeScript rules (as mentioned above)

@devversion devversion requested a review from angular/fw-dev-infra as a code owner Jan 10, 2019

@googlebot googlebot added the cla: yes label Jan 10, 2019

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Jan 10, 2019

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Jan 10, 2019

fix(bazel): replay compilation uses wrong compiler for building esm5
With the update to TypeScript 3.2.x, a big issue seems to have appeared for downstream Bazel users. If the downstream user still uses a lower TypeScript version, normal Bazel targets using the `ng_module` rule are still compiled with the correct/old TypeScript version (assuming they set the `node_modules` attribute properly).

But, if they build the previous Bazel targets by specifying them within a `ng_package` rule, the TypeScript version from the Angular `workspace` is being used for the replayed ESM5 compilation. This is because we resolve the replay compiler to `ngc_wrapped` or `tsc_wrapped` Bazel executables which are defined as part of the `angular` workspace. This means that the compilers are different if the downstream user uses `ngc-wrapped` from the `@npm` repository because the replayed compilation would use the compiler with `@ngdeps//typescript`.

In order to fix this, we should just use the compiler that is defined in the `@angular//BUILD.bazel` file. This target by defaults to the "@npm" workspace which is working for downstream users. This is similar to how it is handled for `tsc-wrapped`. `tsc-wrapped` works as expected for downstream users.

**Note**: This is not the ideal solution because ideally we would
completely respect the `compiler` option from the base `ng_module`, but
this is not possible in a hermetic way, unless we somehow accept the
`compiler` as an attribute that builds all transitive deps. This is
something we should explore in the future. For now, we just fix this in
a reasonable way that is also used for `tsc_wrapped` from the TypeScript
rules.

@devversion devversion force-pushed the devversion:fix/bazel-replay-compilation-uses-wrong-compiler branch from 1afa2e9 to b34776d Jan 10, 2019

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Jan 10, 2019

fixup! fix(bazel): replay compilation uses wrong compiler for buildin…
…g esm5

Fix that when building in the Angular repository, we don't have @angular/bazel installed through NPM. We want to build from the actual sources
@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Jan 10, 2019

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Jan 10, 2019

# a "npm" workspace with the "@angular/bazel" NPM package installed.
if not replay_compiler_path.startswith("../"):
compiler = ctx.executable._internal_ngc_wrapped

This comment has been minimized.

@devversion

devversion Jan 10, 2019

Member

Note that buildifier enforces this empty line. Also I just added the BEGIN-INTERNAL markers because if we ever end up publishing this as part of the npm_package, we don't want this logic to be inside.

This comment has been minimized.

@alexeagle

alexeagle Jan 11, 2019

Contributor

The INTERNAL fences are already load-bearing I think. We have downstream users of ng_package who can't use our internal label. Maybe you're double-guarding this by also checking for the replay_compiler_path.startswith("../") ? if it's in an INTERNAL fence, I think we should just always replace replay_compiler_name.startswith("ngc-wrapped") with ctx.executable._internal_ngc_wrapped instead of guarding it twice

This comment has been minimized.

@devversion

devversion Jan 11, 2019

Member

The BEGIN-INTERNAL is basically a noop at the moment, so we need to guard again here by checking the replay compiler path. It's currently internal because we don't ship the .bzl files through the NPM package.

I've added it so that it it is clearly marked as internal and if we ever end up using the npm package for the Bazel rules, it would be already removed.

AndrewKushnir added a commit that referenced this pull request Jan 14, 2019

fix(bazel): replay compilation uses wrong compiler for building esm5 (#…
…28053)

With the update to TypeScript 3.2.x, a big issue seems to have appeared for downstream Bazel users. If the downstream user still uses a lower TypeScript version, normal Bazel targets using the `ng_module` rule are still compiled with the correct/old TypeScript version (assuming they set the `node_modules` attribute properly).

But, if they build the previous Bazel targets by specifying them within a `ng_package` rule, the TypeScript version from the Angular `workspace` is being used for the replayed ESM5 compilation. This is because we resolve the replay compiler to `ngc_wrapped` or `tsc_wrapped` Bazel executables which are defined as part of the `angular` workspace. This means that the compilers are different if the downstream user uses `ngc-wrapped` from the `@npm` repository because the replayed compilation would use the compiler with `@ngdeps//typescript`.

In order to fix this, we should just use the compiler that is defined in the `@angular//BUILD.bazel` file. This target by defaults to the "@npm" workspace which is working for downstream users. This is similar to how it is handled for `tsc-wrapped`. `tsc-wrapped` works as expected for downstream users.

**Note**: This is not the ideal solution because ideally we would
completely respect the `compiler` option from the base `ng_module`, but
this is not possible in a hermetic way, unless we somehow accept the
`compiler` as an attribute that builds all transitive deps. This is
something we should explore in the future. For now, we just fix this in
a reasonable way that is also used for `tsc_wrapped` from the TypeScript
rules.

PR Close #28053

@devversion devversion deleted the devversion:fix/bazel-replay-compilation-uses-wrong-compiler branch Jan 14, 2019

petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Jan 14, 2019

fix(bazel): replay compilation uses wrong compiler for building esm5 (a…
…ngular#28053)

With the update to TypeScript 3.2.x, a big issue seems to have appeared for downstream Bazel users. If the downstream user still uses a lower TypeScript version, normal Bazel targets using the `ng_module` rule are still compiled with the correct/old TypeScript version (assuming they set the `node_modules` attribute properly).

But, if they build the previous Bazel targets by specifying them within a `ng_package` rule, the TypeScript version from the Angular `workspace` is being used for the replayed ESM5 compilation. This is because we resolve the replay compiler to `ngc_wrapped` or `tsc_wrapped` Bazel executables which are defined as part of the `angular` workspace. This means that the compilers are different if the downstream user uses `ngc-wrapped` from the `@npm` repository because the replayed compilation would use the compiler with `@ngdeps//typescript`.

In order to fix this, we should just use the compiler that is defined in the `@angular//BUILD.bazel` file. This target by defaults to the "@npm" workspace which is working for downstream users. This is similar to how it is handled for `tsc-wrapped`. `tsc-wrapped` works as expected for downstream users.

**Note**: This is not the ideal solution because ideally we would
completely respect the `compiler` option from the base `ng_module`, but
this is not possible in a hermetic way, unless we somehow accept the
`compiler` as an attribute that builds all transitive deps. This is
something we should explore in the future. For now, we just fix this in
a reasonable way that is also used for `tsc_wrapped` from the TypeScript
rules.

PR Close angular#28053

AndrewKushnir added a commit that referenced this pull request Jan 16, 2019

fix(bazel): replay compilation uses wrong compiler for building esm5 (#…
…28053)

With the update to TypeScript 3.2.x, a big issue seems to have appeared for downstream Bazel users. If the downstream user still uses a lower TypeScript version, normal Bazel targets using the `ng_module` rule are still compiled with the correct/old TypeScript version (assuming they set the `node_modules` attribute properly).

But, if they build the previous Bazel targets by specifying them within a `ng_package` rule, the TypeScript version from the Angular `workspace` is being used for the replayed ESM5 compilation. This is because we resolve the replay compiler to `ngc_wrapped` or `tsc_wrapped` Bazel executables which are defined as part of the `angular` workspace. This means that the compilers are different if the downstream user uses `ngc-wrapped` from the `@npm` repository because the replayed compilation would use the compiler with `@ngdeps//typescript`.

In order to fix this, we should just use the compiler that is defined in the `@angular//BUILD.bazel` file. This target by defaults to the "@npm" workspace which is working for downstream users. This is similar to how it is handled for `tsc-wrapped`. `tsc-wrapped` works as expected for downstream users.

**Note**: This is not the ideal solution because ideally we would
completely respect the `compiler` option from the base `ng_module`, but
this is not possible in a hermetic way, unless we somehow accept the
`compiler` as an attribute that builds all transitive deps. This is
something we should explore in the future. For now, we just fix this in
a reasonable way that is also used for `tsc_wrapped` from the TypeScript
rules.

PR Close #28053
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment