build: use bazel version from node modules by devversion · Pull Request #26691 · angular/angular

devversion · 2018-10-23T17:46:19Z

No longer depends on a custom CircleCI docker image that comes with Bazel pre-installed. Since Bazel is now available through NPM, we should be able to use the version from @bazel/bazel in order to enforce a consistent environment on CI and locally.
This also reduces the amount of packages that need to be published (ngcontainer is removed)

devversion · 2018-10-23T19:35:03Z

This is blocked on bazel-contrib/rules_nodejs#390. Need to wait for the fix being available in @bazel/bazel.

alexeagle · 2018-10-23T19:02:12Z

I'd rather it was more parallel and less clever.

Both docker vars should just be the image, then below when you need to override do the whole

docker: - image: *browsers_docker_image

Also put the two docker images next to each other in the file so it's more clear they should be updated together

alexeagle · 2018-10-23T19:03:20Z

actually, please remove this whole alias

Bazel no longer depends on the node_modules directory so users should never manually install

alexeagle · 2018-10-23T19:03:57Z

Remove the sections above about installing things

devversion · 2018-10-24T08:56:20Z

@alexeagle Addressed your feedback. Thanks. This should be ready for another review.

alexeagle

Don't delete the tools/ngcontainer yet - a bunch of other repos still depend on it

thanks for working on this! looks great

alexeagle · 2018-10-24T13:01:50Z

0.7.0 should be changed (to 10.12 I think) - out of caution we like the cache key to change whenever the container does, because we had a cache corruption once

and keep the comment about "if you change the docker_image version"
but no longer need "and the version of com_github... in the /WORKSPACE"

Done. I've used node-10.12 in order to make it clear that the 10.12 stands for the NodeJS version.

alexeagle · 2018-10-24T13:05:37Z

no point running bazel info release anymore, we know it's the one in yarn.lock

gkalpak

This also needs rebasing on latest master, because config.yml has changed (e.g. f8741c0).

gkalpak · 2018-10-26T06:08:05Z

the the --> the

Good catch.

gkalpak · 2018-10-26T06:17:03Z

gkalpak · 2018-10-26T19:28:01Z

Inconsistent (although arguably more reasonable) indentation.

gkalpak · 2018-10-26T19:31:02Z

Let's keep the comments consistent between test_docs_examples_0 and test_docs_examples_1 🙏

gkalpak · 2018-10-26T19:32:15Z

This was running yarn install with the --cwd aio option.

gkalpak · 2018-10-26T19:35:13Z

Nit: It is not only the PWA score tests that require Chrome. (We run some minimal e2e tests as well now.)

devversion · 2018-10-26T21:08:38Z

Addressed all comments and rebased multiple times (with the recent changes to config.yml)

Closing and re-opening in favor of triggering CircleCI.

googlebot · 2018-10-26T21:23:15Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

mary-poppins · 2018-10-30T13:32:43Z

You can preview f69489f at https://pr26691-f69489f.ngbuilds.io/.

This makes yarn_install of ngdeps under Bazel faster, since we don't need many of the large dependencies. It's important because downstream angular/bazel users will observe the same install time.

* No longer depends on a custom CircleCI docker image that comes with Bazel pre-installed. Since Bazel is now available through NPM, we should be able to use the version from `@bazel/bazel` in order to enforce a consistent environment on CI and locally. * This also reduces the amount of packages that need to be published (ngcontainer is removed)

mary-poppins · 2018-10-30T15:58:42Z

You can preview 3d48fac at https://pr26691-3d48fac.ngbuilds.io/.

devversion · 2018-10-30T16:22:31Z

This is finally green (after soo much closing and reopening; CI still doesn't run properly).

There was a failing test within the compiler-cli that unintentionally ran because initially we locked the NodeJS rules to v0.15.2 which doesn't include: bazel-contrib/rules_nodejs@b576f1e

alexeagle · 2018-10-30T16:24:49Z

Caretaker: this was approved but pullapprove is stale, merge-assistance please

This makes yarn_install of ngdeps under Bazel faster, since we don't need many of the large dependencies. It's important because downstream angular/bazel users will observe the same install time. PR Close #26691

* No longer depends on a custom CircleCI docker image that comes with Bazel pre-installed. Since Bazel is now available through NPM, we should be able to use the version from `@bazel/bazel` in order to enforce a consistent environment on CI and locally. * This also reduces the amount of packages that need to be published (ngcontainer is removed) PR Close #26691

By default, `webdriver-manager update` will download the latest ChromeDriver version, which might not be compatible with the Chrome version included in the [docker image used on CI], causing CI failures. Previously, we used to pin the ChromeDriver version on CI in [ngcontainer's Dockerfile][2]. This was accidentally broken in angular#26691, while moving from ngcontainer to default CircleCI docker images. This commit fixes the issue by pinning ChromeDriver to a known compatible version. [1]: https://github.com/angular/angular/blob/bfd48d156d5663cc49d1ec40c2d73ff76a2fe62f/.circleci/config.yml#L16 [2]: https://github.com/angular/angular/blob/bfd48d156d5663cc49d1ec40c2d73ff76a2fe62f/tools/ngcontainer/Dockerfile#L63

…me (#28494) By default, `webdriver-manager update` will download the latest ChromeDriver version, which might not be compatible with the Chrome version included in the [docker image used on CI], causing CI failures. Previously, we used to pin the ChromeDriver version on CI in [ngcontainer's Dockerfile][2]. This was accidentally broken in #26691, while moving from ngcontainer to default CircleCI docker images. This commit fixes the issue by pinning ChromeDriver to a known compatible version. [1]: https://github.com/angular/angular/blob/bfd48d156d5663cc49d1ec40c2d73ff76a2fe62f/.circleci/config.yml#L16 [2]: https://github.com/angular/angular/blob/bfd48d156d5663cc49d1ec40c2d73ff76a2fe62f/tools/ngcontainer/Dockerfile#L63 PR Close #28494

angular-automatic-lock-bot · 2019-09-14T06:23:05Z

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.}

googlebot added the cla: yes label Oct 23, 2018

devversion force-pushed the build/use-bazel-from-node-modules branch 2 times, most recently from 787cb0b to b916a55 Compare October 23, 2018 18:25

alexeagle reviewed Oct 23, 2018

View reviewed changes

devversion force-pushed the build/use-bazel-from-node-modules branch 4 times, most recently from 35200c0 to 14294e7 Compare October 24, 2018 08:06

devversion changed the title ~~WIP build: use bazel version from node modules~~ build: use bazel version from node modules Oct 24, 2018

devversion force-pushed the build/use-bazel-from-node-modules branch from 14294e7 to c0682b8 Compare October 24, 2018 08:23

alexeagle reviewed Oct 24, 2018

View reviewed changes

devversion force-pushed the build/use-bazel-from-node-modules branch from c0682b8 to f1dd3d6 Compare October 24, 2018 14:21

alexeagle approved these changes Oct 24, 2018

View reviewed changes

alexeagle added target: patch This PR is targeted for the next patch release action: merge The PR is ready for merge by the caretaker labels Oct 24, 2018

gkalpak suggested changes Oct 26, 2018

View reviewed changes

gkalpak added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Oct 26, 2018

devversion force-pushed the build/use-bazel-from-node-modules branch from f1dd3d6 to 6327372 Compare October 26, 2018 17:55

gkalpak suggested changes Oct 26, 2018

View reviewed changes

devversion force-pushed the build/use-bazel-from-node-modules branch from 6327372 to 20d37e2 Compare October 26, 2018 21:03

devversion closed this Oct 26, 2018

devversion reopened this Oct 26, 2018

devversion force-pushed the build/use-bazel-from-node-modules branch from 20d37e2 to 2d96449 Compare October 26, 2018 21:13

googlebot added cla: no and removed cla: yes labels Oct 26, 2018

alexeagle and others added 2 commits October 30, 2018 16:53

build: Move non-bazel deps to devDependencies

bfca9e8

This makes yarn_install of ngdeps under Bazel faster, since we don't need many of the large dependencies. It's important because downstream angular/bazel users will observe the same install time.

devversion force-pushed the build/use-bazel-from-node-modules branch from f69489f to 3d48fac Compare October 30, 2018 15:54

devversion closed this Oct 30, 2018

devversion reopened this Oct 30, 2018

alexeagle approved these changes Oct 30, 2018

View reviewed changes

IgorMinar approved these changes Oct 30, 2018

View reviewed changes

matsko closed this in 66be3c9 Oct 30, 2018

devversion mentioned this pull request Jan 12, 2019

ci: move the remaining travis tests to circle #27937

Closed

gkalpak mentioned this pull request Feb 1, 2019

ci: pin ChromeDriver to a version compatible with docker image's Chrome #28494

Closed

2 of 3 tasks

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 14, 2019

Conversation

devversion commented Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devversion commented Oct 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devversion commented Oct 24, 2018

Uh oh!

alexeagle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gkalpak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devversion commented Oct 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented Oct 26, 2018

Uh oh!

mary-poppins commented Oct 30, 2018

Uh oh!

mary-poppins commented Oct 30, 2018

Uh oh!

devversion commented Oct 30, 2018

Uh oh!

alexeagle commented Oct 30, 2018

Uh oh!

angular-automatic-lock-bot Bot commented Sep 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

devversion commented Oct 23, 2018 •

edited

Loading

devversion commented Oct 26, 2018 •

edited

Loading