Skip to content

/ components

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

Jump to bottom

Update to Angular 7.0.1 http_archive & @angular/bazel package #13831

Merged
merged 2 commits into from Oct 29, 2018
Merged

Update to Angular 7.0.1 http_archive & @angular/bazel package #13831

merged 2 commits into from Oct 29, 2018

Conversation

@gregmagolan
Copy link
Contributor

@gregmagolan gregmagolan commented Oct 26, 2018

Updates to idiomatic install of @angular/bazel package v7.0.1 which should match the version of the angular http_archive() in WORKSPACE. Also removes the compiler override in ng_module. Not necessary when @angular/bazel is installed from npm.

Some cleanup as well:

  • checks for minimum Bazel version of 0.18.0
  • adds .bazelignore
  • removes rules_typescript_dependencies() from WORKSPACE (these deps are installed by by rules_angular_dependencies())
@gregmagolan gregmagolan requested review from devversion and jelbourn as code owners Oct 26, 2018
@gregmagolan
Copy link
Contributor Author

@gregmagolan gregmagolan commented Oct 26, 2018

@alexeagle Discussing with @devversion if this repo should use the @angular/bazel npm package just override the compiler attribute in ng_module. Thoughts?
@devversion
Copy link
Member

@devversion devversion commented Oct 26, 2018

My point is that we want to build Angular Material completely from source. Since we include the @angular workspace, we already pull in the appropriate @angular/bazel package and shouldn't need the package from NPM.

With this change, we would need to keep the WORKSPACE and package.json in sync and are blocked on releases of @bazel/angular while we could actually just change the version in the WORKSPACE (as it's usually done w/ Bazel).

So, practically - I don't see any benefit of using the @angular/bazel package here. It might make sense for people that just pull in the NPM package, but not for Bazel projects that build completely from source.

If we go through NPM, we could also just build Material from the Bazel managed deps (@npm//@angular/X)
@devversion
devversion reviewed Oct 26, 2018
View changes
WORKSPACE Outdated
http_archive(
name = "build_bazel_rules_nodejs",

This comment has been minimized.

Sign in to view
@devversion

devversion Oct 26, 2018 •
edited
Member

I've proposed deleting the rules_nodejs and rules_typescript transitive dependencies here as well (at some point), but since we use them explicitly and not only transitively as part of the ng_module rule, @jelbourn and I agreed that we should keep them here.

This comment has been minimized.

Sign in to view
@jelbourn

jelbourn Oct 26, 2018
Member

Yes- we use these rules directly and should not depend on transitive deps.

This comment has been minimized.

Sign in to view
@gregmagolan

gregmagolan Oct 26, 2018
Author Contributor

These are installed via rules_angular_dependencies() which will install the correct versions needed to build with ng_module. The same thing is done in the angular/angular WORKSPACE file. The versions loaded can be overridden explicitly by loading a specific version of either or both before rules_angular_dependencies() but will be easier to maintain by using the versions installed by rules_angular_dependencies() unless you need a particular fix or feature from either before it is updated in angular/angular.
@jelbourn
Copy link
Member

@jelbourn jelbourn commented Oct 26, 2018

The original impetus to build Angular from source was that it was thought that it would workaround angular/angular#25644. It turns out that it has no effect either way.

Without having that motivation, I think we should consume Angular the same way that other projects are meant to, whatever the recommendation is.
@devversion
Copy link
Member

@devversion devversion commented Oct 26, 2018

@jelbourn That's reasonable. I think it's fine moving away from @angular//packages to just @npm/@angular.

Nevertheless it was just easier to maintain the whole Bazel setup, if we could pull specific commits that haven't been released yet (e.g. how we used the bazel branch). Same applies for tools we'd like to use, such as ts-api-guardian.
@gregmagolan
Copy link
Contributor Author

@gregmagolan gregmagolan commented Oct 26, 2018

We'll still be using @angular//packages either way for now until Ivy is released and we can get away from the angular http_archive entirely.
@devversion
Copy link
Member

@devversion devversion commented Oct 26, 2018

@gregmagolan Do you have a small explanation why we would need to wait for Ivy before switching to @npm//@angular. Is it because source files are not shipped with NPM?
@devversion
devversion approved these changes Oct 26, 2018
View changes
Copy link
Member

@devversion devversion left a comment

LGTM. If you can re-add the archives for the NodeJS and TypeScript rules.
@gregmagolan
Copy link
Contributor Author

@gregmagolan gregmagolan commented Oct 26, 2018

@gregmagolan Do you have a small explanation why we would need to wait for Ivy before switching to @npm//@angular. Is it because source files are not shipped with NPM?

There a few issues that are blocking us from using @angular npm packages (minus the @angular/bazel package which is bazel specific) with bazel right now.

angular/angular#25644
angular/angular#24521 (work-around for this was to build angular from source downstream: angular/angular-bazel-example#160)
@gregmagolan
Copy link
Contributor Author

@gregmagolan gregmagolan commented Oct 26, 2018

LGTM. If you can re-add the archives for the NodeJS and TypeScript rules.

That's not necessary IMO. You'll get the correct versions of both transitively via rules_angular_dependencies() and don't have to worry about updating the versions since updates will flow through the transitive deps.
@devversion
Copy link
Member

@devversion devversion commented Oct 26, 2018 •
edited

I actually wanted to go with the same approach, but Jeremy and Alex convinced me that we should rather explicitly specify these dependencies. See the conversation here
@gregmagolan
Copy link
Contributor Author

@gregmagolan gregmagolan commented Oct 26, 2018

I actually wanted to with the same approach, but Jeremy and Alex convinced me that we should rather explicitly specify these dependencies. See the conversation here

Sounds good. I'll add in the explicit deps again. As mentioned in the discussion here (#13408 (comment)), ng_setup_workspace() and ts_setup_workspace() will error out if the versions explicitly specified are older than the minimum required.
@gregmagolan
gregmagolan reviewed Oct 26, 2018
View changes
WORKSPACE
@@ -5,42 +5,20 @@ http_archive(
name = "build_bazel_rules_nodejs",
url = "https://github.com/bazelbuild/rules_nodejs/archive/0.15.3.zip",
strip_prefix = "rules_nodejs-0.15.3",
sha256 = "05afbbc13b0b7d5056e412d66c98853978bd46a94bc8e7b71c7fba4349b77eef",

This comment has been minimized.

Sign in to view
@gregmagolan

gregmagolan Oct 26, 2018
Author Contributor

Note: not recommended to use sha256 with github.com archives since the zip files is generated on the fly by github and the sha256 may change in the future
@gregmagolan
gregmagolan reviewed Oct 26, 2018
View changes
WORKSPACE
strip_prefix = "rules_typescript-0.20.3",
sha256 = "2a03b23c30c5109ab0863cfa60acce73ceb56337d41efc2dd67f8455a1c1d5f3",
url = "https://github.com/bazelbuild/rules_typescript/archive/8ea1a55cf5cf8be84ddfeefc0940769b80da792f.zip",
strip_prefix = "rules_typescript-8ea1a55cf5cf8be84ddfeefc0940769b80da792f",

This comment has been minimized.

Sign in to view
@gregmagolan

gregmagolan Oct 26, 2018
Author Contributor

Update this version to support --nolegacy_external_runfiles
@gregmagolan
Update to Angular & @angular/bazel 7.0.1
Loading status checks…
614fd5b
@gregmagolan
gregmagolan reviewed Oct 26, 2018
View changes
.circleci/config.yml
var_1: &docker_image angular/ngcontainer:0.6.0
var_2: &cache_key v2-ng-mat-{{ .Branch }}-{{ checksum "yarn.lock" }}-0.6.0
var_1: &docker_image angular/ngcontainer:0.7.0
var_2: &cache_key v2-ng-mat-{{ .Branch }}-{{ checksum "yarn.lock" }}-0.7.0

This comment has been minimized.

Sign in to view
@gregmagolan

gregmagolan Oct 26, 2018
Author Contributor

Updates CircleCI to Bazel 0.18.0
@devversion
devversion approved these changes Oct 26, 2018
View changes
WORKSPACE Outdated Show resolved Hide resolved
@gregmagolan
Remove sha256 from io_bazel_rules_sass http_archive
Loading status checks…
106266f
@jelbourn
jelbourn approved these changes Oct 26, 2018
View changes
Copy link
Member

@jelbourn jelbourn left a comment

LGTM
@jelbourn jelbourn merged commit b3a0037 into angular:master Oct 29, 2018
10 checks passed
10 checks passed
@ngbot
ci/angular: merge status All checks passed!
ci/circleci: bazel_build_test Your tests passed on CircleCI!
Details
ci/circleci: demo_app_aot Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: prerender_build Your tests passed on CircleCI!
Details
ci/circleci: tests_browserstack Your tests passed on CircleCI!
Details
ci/circleci: tests_local_browsers Your tests passed on CircleCI!
Details
@googlebot
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
atscott added a commit to atscott/material2 that referenced this pull request Nov 5, 2018
@gregmagolan @atscott
build: update to Angular 7.0.1 http_archive & @angular/bazel package (a…
a3f86ce 
…ngular#13831)

* Update to Angular & @angular/bazel 7.0.1
* Remove sha256 from io_bazel_rules_sass http_archive
jelbourn added a commit that referenced this pull request Nov 6, 2018
@gregmagolan @jelbourn
build: update to Angular 7.0.1 http_archive & @angular/bazel package (#…
Loading status checks…
73a9417 
…13831)

* Update to Angular & @angular/bazel 7.0.1
* Remove sha256 from io_bazel_rules_sass http_archive
@angular-automatic-lock-bot
Copy link

@angular-automatic-lock-bot angular-automatic-lock-bot bot commented Sep 10, 2019

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

@jelbourn jelbourn

@devversion devversion

Assignees

@alexeagle alexeagle

@jelbourn jelbourn

Labels
cla: yes merge ready merge safe target: patch
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@gregmagolan @devversion @jelbourn @alexeagle @googlebot