-
Notifications
You must be signed in to change notification settings - Fork 6.8k
build(material): add bazel rules for all components #8457
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
Conversation
18628c2 to
c12ba80
Compare
|
Updated now with CircleCI config to build everything |
alexeagle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments apply to all BUILD.bazel files
.circleci/config.yml
Outdated
|
|
||
| - run: bazel run @nodejs//:npm install | ||
| # For some reason, circleci needs the postinstall to be run explicitly. | ||
| # This may be unnecessary once ngcontainer uses nodejs 8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's rules_nodejs that needs to switch, not ngcontainer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/lib/autocomplete/BUILD.bazel
Outdated
| package(default_visibility=["//visibility:public"]) | ||
| load("@angular//:index.bzl", "ng_module") | ||
| load("@io_bazel_rules_sass//sass:sass.bzl", "sass_library") | ||
| load("@io_bazel_rules_sass//sass:sass.bzl", "sass_binary") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can load these two symbols in one load statement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| tsconfig = ":tsconfig-build.json", | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
buildozer is available externally, you could format all your BUILD files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack, I'll pursue that in a follow-up.
| name = "autocomplete_css", | ||
| srcs = [":autocomplete_scss"], | ||
| outs = ["autocomplete.css"], | ||
| cmd = "cat $(locations :autocomplete_scss) > $@", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since you have a single src you can
cmd = "cp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, you do this enough that you might want to make a macro around sass_binary...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sass_binary rule claims that it has multiple sources even though there is just one
| deps = ["//src/lib/core:core_scss_lib"], | ||
| ) | ||
|
|
||
| # TODO(jelbourn): remove this when sass_binary supports specifying an output filename and dir. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe file an issue for that on rules_sass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c12ba80 to
56aea99
Compare
|
Even if srcs accepts an array I think $< still works if there happens to be
one value?
…On Wed, Nov 15, 2017, 5:22 PM Jeremy Elbourn ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In .circleci/config.yml
<#8457 (comment)>:
> @@ -34,7 +34,7 @@ jobs:
# For some reason, circleci needs the postinstall to be run explicitly.
# This may be unnecessary once ngcontainer uses nodejs 8
Done
------------------------------
In src/lib/autocomplete/BUILD.bazel
<#8457 (comment)>:
> @@ -0,0 +1,40 @@
+package(default_visibility=["//visibility:public"])
***@***.***//:index.bzl", "ng_module")
***@***.***_bazel_rules_sass//sass:sass.bzl", "sass_library")
***@***.***_bazel_rules_sass//sass:sass.bzl", "sass_binary")
Done
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#8457 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAC5I9RgWn7V-c_T7mzMoCHe1rBlSN1Rks5s245tgaJpZM4Qff7n>
.
|
|
2a6516b to
a640b83
Compare
b795252 to
407eb09
Compare
ced197c to
34e3363
Compare
alexeagle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should make sure tazel can re-generate all your BUILD files
| working_directory: ~/ng | ||
| docker: | ||
| - image: angular/ngcontainer:0.0.4 | ||
| - image: angular/ngcontainer:0.0.7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider adding buildifier lint
| - run: bazel run @nodejs//:npm install | ||
| # For some reason, circleci needs the postinstall to be run explicitly. | ||
| # This may be unnecessary once ngcontainer uses nodejs 8 | ||
| # This may be unnecessary once rules_nodejs uses nodejs 8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it does now, upgrade to 0.3.1
.circleci/config.yml
Outdated
| - run: bazel run @nodejs//:npm run postinstall | ||
| - run: bazel build src/cdk/... | ||
| - run: bazel build src/lib/datepicker | ||
| - run: cat /root/.cache/bazel/_bazel_root/049ed6f058a98cba95753eee774cab7a/server/jvm.out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
debugging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm in the middle of debugging now
8e17141 to
3ad8d6e
Compare
3ad8d6e to
700250d
Compare
700250d to
e18736e
Compare
|
CircleCI issues are now resolved |
josephperrott
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* Applies angular#8457 to divider
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Part of #8369
cc @devversion
The genrules for transforming the css output are unfortunate but should be able to be removed without a ton of effort.