Skip to content

Conversation

@jelbourn
Copy link
Member

@jelbourn jelbourn commented Mar 5, 2019

This allows us to cherry-pick @kara's static query updates from the
ivy-2019 branch.

This change includes:

  • Adding missing dependencies for various Angular subpackages
  • Add umd assets where they are now necessary
  • Configure bazel to target ES5 output as a workaround for forwardRef run-time error with ES2015 output under bazel angular#29107
  • Updates most npm dependencies to the @npm// workspace
  • Updates general workspace setup, RBE config, ngcontainer
  • Update to rxjs 6.4.0 (building from source no longer needed)
  • Added rxjs_shims for ts_web_test and ts_devserver targets
  • Removed index.bzl and angular_material_setup_workspace (not needed anymore since material won't be built from source)
  • Use browser_repositories() from @npm_bazel_karma which have pinned browsers that work on all platforms
  • Change dgeni processor functions from arrow function to function to
    work around @npm//di not supporting arrow functions

cc @gregmagolan

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Mar 5, 2019
@jelbourn jelbourn force-pushed the angular-bazel-vers8 branch from de62dc5 to 851c0e8 Compare March 5, 2019 23:54
@googlebot

This comment has been minimized.

@googlebot googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Mar 5, 2019
@jelbourn jelbourn added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Mar 5, 2019
@googlebot

This comment has been minimized.

@jelbourn jelbourn force-pushed the angular-bazel-vers8 branch from 851c0e8 to db71984 Compare March 6, 2019 00:04
@googlebot

This comment has been minimized.

@googlebot googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Mar 6, 2019
@jelbourn jelbourn added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Mar 6, 2019
@googlebot

This comment has been minimized.

build:remote --host_java_toolchain=@bazel_tools//tools/jdk:toolchain_hostjdk8
build:remote --java_toolchain=@bazel_tools//tools/jdk:toolchain_hostjdk8
build:remote --crosstool_top=@bazel_toolchains//configs/ubuntu16_04_clang/1.1/bazel_0.18.0/default:toolchain
build:remote --crosstool_top=@bazel_toolchains//configs/ubuntu16_04_clang/1.1/bazel_0.22.0/default:toolchain
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe for later: I like the approach in angular/angular of vendoring one file from the bazel team, the filename is versioned with Bazel so it's easy to treat it as a black box, then you just import that file to get all these settings

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack, can be a future improvement.

package.json Outdated
"@bazel/ibazel": "^0.9.0",
"@bazel/karma": "0.22.1",
"@bazel/typescript": "0.22.1",
"@bazel/jasmine": "0.26.0-beta.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: shouldn't need the beta here, 0.26.0 is released

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

WORKSPACE Outdated
name = "npm",
path = "tools/npm-workspace"
sha256 = "86ea92217dfd4a84e1e335cc07dfd942b12899796b080492546b947f12c5ab77",
urls = ["https://github.com/bazelbuild/rules_nodejs/releases/download/0.26.0-beta.0/rules_nodejs-0.26.0-beta.0.tar.gz"],
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: shouldn't need to use beta, 0.26.0 is released

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

LGTM.

package.json Outdated
"@angular/platform-browser-dynamic": "^7.2.1",
"@angular/platform-server": "^7.2.1",
"@angular/router": "^7.2.1",
"@angular/bazel": "angular/bazel-builds#8.0.0-beta.5+8f8f9a6",
Copy link
Member

Choose a reason for hiding this comment

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

Can you quickly elaborate why we need @angular/bazel from the builds repository?

Maybe 8.0.0-beta.6 already includes a particular change that wasn't part of beta.5.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to beta.6 (which wasn't released when I started)

src/BUILD.bazel Outdated
"@npm//@angular/animations",
"@npm//@angular/platform-browser",
"@npm//dgeni",
"@npm//dgeni-packages",
Copy link
Member

Choose a reason for hiding this comment

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

Technically these should go into the dgeni_api_docs rule. Though I understand that this PR is already big enough. I'll make a note on my end if you want to leave it out for this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed. I only had these here for debugging.

"@npm//node_modules/@angular/core:bundles/core.umd.js",
"@npm//node_modules/@angular/forms:bundles/forms.umd.js",
"@npm//node_modules/@angular/router:bundles/router.umd.js",
"@npm//node_modules/@angular/platform-browser-dynamic:bundles/platform-browser-dynamic-testing.umd.js",
Copy link
Member

Choose a reason for hiding this comment

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

Is that just temporary as well? I don't see why we would need -testing bundles here. Maybe add a comment like you did in defaults.bzl for the tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was just being lazy and copied from the testing default. I moved these out into a constant in packages.bzl and added a comment where it's used

"@rxjs",
"@npm//@angular/common",
"@npm//@angular/core",
"@npm//@angular/forms", # TODO(jelbourn): transitive dep via generated code
Copy link
Member

Choose a reason for hiding this comment

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

For later reference: This is actually expected as we turned of the symbol re-exports (see angular/angular@91b7152).

@@ -0,0 +1,41 @@
/**
* @license
* Copyright Google Inc. All Rights Reserved.
Copy link
Member

Choose a reason for hiding this comment

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

Also just realized this. This license is not in sync with what we usually have in Material. I know that this file is from angular/angular and they don't use Google LLC.. thought just checking if we want to update it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@jelbourn jelbourn force-pushed the angular-bazel-vers8 branch from db71984 to 6aeeb3f Compare March 6, 2019 19:53
@googlebot googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Mar 6, 2019
@jelbourn jelbourn force-pushed the angular-bazel-vers8 branch from 6aeeb3f to ac6a23b Compare March 6, 2019 19:54
@jelbourn jelbourn added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Mar 6, 2019
@googlebot

This comment has been minimized.

@angular angular deleted a comment from googlebot Mar 6, 2019
@jelbourn jelbourn added the target: major This PR is targeted for the next major release label Mar 6, 2019
@jelbourn jelbourn force-pushed the angular-bazel-vers8 branch from ac6a23b to 55c869d Compare March 6, 2019 21:08
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Mar 6, 2019
Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

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

LGTM

This allows us to cherry-pick @kara's static query updates from the
`ivy-2019` branch.

This change includes:
* Adding missing dependencies for various Angular subpackages
* Add umd assets where they are now necessary
* Configure bazel to target ES5 output as a workaround for angular/angular#29107
* Updates most npm dependencies to the @npm// workspace
* Updates general workspace setup, RBE config, ngcontainer
* Update to rxjs 6.4.0 (building from source no longer needed)
* Added rxjs_shims for ts_web_test and ts_devserver targets
* Removed index.bzl and angular_material_setup_workspace (not needed anymore since material won't be built from source)
* Use browser_repositories() from @npm_bazel_karma which have pinned browsers that work on all platforms
* Change dgeni processor functions from arrow function to `function` to
work around `@npm//di` not supporting arrow functions

Co-authored-by: Greg Magolan <gmagolan@gmail.com>
@josephperrott josephperrott added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Mar 6, 2019
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@jelbourn jelbourn force-pushed the angular-bazel-vers8 branch from 55c869d to 5e072ab Compare March 6, 2019 21:13
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@josephperrott josephperrott added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Mar 6, 2019
@googlebot googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Mar 6, 2019
@josephperrott josephperrott added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Mar 6, 2019
@ngbot
Copy link

ngbot bot commented Mar 6, 2019

I see that you just added the pr: merge ready label, but the following checks are still failing:
    failure status "cla/google" is failing
    pending status "ci/circleci: api_golden_checks" is pending
    pending status "ci/circleci: bazel_build_test" is pending
    pending status "ci/circleci: build_release_packages" is pending
    pending status "ci/circleci: lint" is pending
    pending status "ci/circleci: prerender_build" is pending
    pending status "ci/circleci: tests_browserstack" is pending
    pending status "ci/circleci: tests_local_browsers" is pending
    pending status "ci/circleci: tests_saucelabs" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@josephperrott
Copy link
Member

Correcting googlebot as all authors have CLA on file.

@josephperrott josephperrott merged commit 5fb022f into angular:master Mar 6, 2019
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants