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

fix(bazel): ng_module should not emit shim files under bazel and Ivy #33765

Closed
wants to merge 3 commits into from

Conversation

@IgorMinar
Copy link
Member

IgorMinar commented Nov 12, 2019

Under bazel and Ivy we don't need the shim files to be emmited by default.

We still need to the shims for blaze however because google3 code imports them.

@IgorMinar IgorMinar requested a review from angular/tools-bazel as a code owner Nov 12, 2019
@googlebot googlebot added the cla: yes label Nov 12, 2019
@IgorMinar IgorMinar requested a review from alxhub Nov 12, 2019
@ngbot ngbot bot added this to the needsTriage milestone Nov 12, 2019
@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 12, 2019

@kyliau
kyliau approved these changes Nov 12, 2019
@alxhub
alxhub approved these changes Nov 12, 2019
@IgorMinar IgorMinar force-pushed the IgorMinar:bazel/dont-emit-shims branch from 9076688 to bf643e9 Nov 15, 2019
@IgorMinar IgorMinar requested a review from angular/fw-integration as a code owner Nov 15, 2019
@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 15, 2019

You can preview bf643e9 at https://pr33765-bf643e9.ngbuilds.io/.

@IgorMinar IgorMinar force-pushed the IgorMinar:bazel/dont-emit-shims branch from bf643e9 to 77c238a Nov 15, 2019
@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 15, 2019

You can preview 77c238a at https://pr33765-77c238a.ngbuilds.io/.

@IgorMinar IgorMinar force-pushed the IgorMinar:bazel/dont-emit-shims branch from 77c238a to 48f1ed8 Nov 15, 2019
@IgorMinar IgorMinar requested review from angular/fw-core as code owners Nov 15, 2019
@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 15, 2019

@IgorMinar IgorMinar force-pushed the IgorMinar:bazel/dont-emit-shims branch from 48f1ed8 to bfad240 Nov 15, 2019
@IgorMinar IgorMinar requested a review from angular/fw-compiler as a code owner Nov 15, 2019
@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 15, 2019

@IgorMinar IgorMinar force-pushed the IgorMinar:bazel/dont-emit-shims branch 2 times, most recently from c13d065 to 816f7c8 Nov 15, 2019
@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 15, 2019

@IgorMinar IgorMinar force-pushed the IgorMinar:bazel/dont-emit-shims branch 2 times, most recently from 407832f to 45213c7 Nov 15, 2019
@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 16, 2019

@IgorMinar IgorMinar force-pushed the IgorMinar:bazel/dont-emit-shims branch from 16d3f04 to bed7d6c Nov 16, 2019
@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 16, 2019

@IgorMinar IgorMinar force-pushed the IgorMinar:bazel/dont-emit-shims branch from bed7d6c to 2c8dc08 Nov 16, 2019
@IgorMinar

This comment has been minimized.

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 16, 2019

@IgorMinar

This comment has been minimized.

Copy link
Member Author

IgorMinar commented Nov 16, 2019

merge-assistance: this PR might require the ivy flip cl/270340880 to be rebased

Copy link
Contributor

gregmagolan left a comment

LGTM

# (the shims used to be on by default)
# we can remove this check once angular/components and angular/angular-cli repos no longer depend
# on the presence of shims, or if they explicitly opt-in to their generation via ng_modules' generate_ve_shims attr
return _is_bazel() and not _enable_ivy_value(ctx) or ((hasattr(ctx.attr, "generate_ve_shims") and ctx.attr.generate_ve_shims == True) or ctx.workspace_name != "angular")

This comment has been minimized.

Copy link
@alexeagle

alexeagle Nov 18, 2019

Contributor

nit: use getter which takes default getattr(ctx.attr, "generate_ve_shims", False)

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Nov 20, 2019

Author Member

is that blaze-safe?

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Nov 20, 2019

Author Member

it is. I refactored the ng_modules.bzl to use it where appropriate.

@IgorMinar IgorMinar force-pushed the IgorMinar:bazel/dont-emit-shims branch from 2c8dc08 to d0e1a4a Nov 20, 2019
@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 20, 2019

@IgorMinar

This comment has been minimized.

Copy link
Member Author

IgorMinar commented Nov 20, 2019

g3 does seem to pass

IgorMinar added 3 commits Nov 16, 2019
the node_modules contains rxjs which contains invalid bazel files which break bazel query executed on //...
Under bazel and Ivy we don't need the shim files to be emmited by default.

We still need to the shims for blaze however because google3 code imports them.

This improves build latency by 1-2 seconds per ng_module target.
getattr improves code readability and makes the code also shorter.
@IgorMinar IgorMinar force-pushed the IgorMinar:bazel/dont-emit-shims branch from d0e1a4a to ba59c82 Nov 22, 2019
@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 22, 2019

@IgorMinar

This comment has been minimized.

Copy link
Member Author

IgorMinar commented Nov 22, 2019

merge-assistance: g3 failures are unrelated

@IgorMinar

This comment has been minimized.

Copy link
Member Author

IgorMinar commented Nov 22, 2019

I kicked off one more presubmit just to be sure: https://test.corp.google.com/ui#id=OCL:280796455:BASE:282004415:1574449954648:5eb13dcc

change has no impact on ivy in g3

@IgorMinar

This comment has been minimized.

Copy link
Member Author

IgorMinar commented Nov 22, 2019

merge-assistance: global approval

@matsko matsko closed this in 8049332 Nov 22, 2019
matsko added a commit that referenced this pull request Nov 22, 2019
…33765)

Under bazel and Ivy we don't need the shim files to be emmited by default.

We still need to the shims for blaze however because google3 code imports them.

This improves build latency by 1-2 seconds per ng_module target.

PR Close #33765
matsko added a commit that referenced this pull request Nov 22, 2019
)

getattr improves code readability and makes the code also shorter.

PR Close #33765
matsko added a commit that referenced this pull request Nov 22, 2019
…33765)

the node_modules contains rxjs which contains invalid bazel files which break bazel query executed on //...

PR Close #33765
matsko added a commit that referenced this pull request Nov 22, 2019
…33765)

Under bazel and Ivy we don't need the shim files to be emmited by default.

We still need to the shims for blaze however because google3 code imports them.

This improves build latency by 1-2 seconds per ng_module target.

PR Close #33765
matsko added a commit that referenced this pull request Nov 22, 2019
)

getattr improves code readability and makes the code also shorter.

PR Close #33765
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.