-
Notifications
You must be signed in to change notification settings - Fork 25.6k
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
WIP: initial version of ng_package #21431
Conversation
You can preview 4cec674 at https://pr21431-4cec674.ngbuilds.io/. |
You can preview 774d202 at https://pr21431-774d202.ngbuilds.io/. |
You can preview e6e595a at https://pr21431-e6e595a.ngbuilds.io/. |
You can preview ea7ede6 at https://pr21431-ea7ede6.ngbuilds.io/. |
@alexeagle I pulled your branch, yarn installed and bazel build and then get this error because I yarn installed in the
This sounds like a trap, especially for those that manage yarn dependencies and switch branches. Could we have bazel run |
Yes, that's bazel-contrib/rules_nodejs#1
I can explain and/or find a link to why bazel doesn't install dependencies
into your project.
…On Wed, Jan 10, 2018 at 12:43 AM Igor Minar ***@***.***> wrote:
@alexeagle <https://github.com/alexeagle> I pulled your branch, yarn
installed and bazel build and then get this error because I yarn installed
in the aio/ dir and not in the root:
$ bazel build //packages/core:package
INFO: Analysed target //packages/core:package (0 packages loaded).
INFO: Found 1 target...
ERROR: /private/var/tmp/_bazel_iminar/9b8801a4939e9750a817dc0cb35bbbca/external/angular/src/packager/BUILD.bazel:4:1: Compiling TypeScript (devmode) @angular//src/packager:lib failed (Exit 1)
external/angular/src/packager/index.ts(1,22): error TS7016: Could not find a declaration file for module 'shelljs'. '/private/var/tmp/_bazel_iminar/9b8801a4939e9750a817dc0cb35bbbca/execroot/angular_src/node_modules/shelljs/shell.js' implicitly has an 'any' type.
external/angular/src/packager/index.ts(16,12): error TS7006: Parameter 'f' implicitly has an 'any' type.
external/angular/src/packager/index.ts(23,41): error TS7006: Parameter 'f' implicitly has an 'any' type.
Target //packages/core:package failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 3.259s, Critical Path: 2.59s
FAILED: Build did NOT complete successfully
This sounds like a trap, especially for those that manage yarn
dependencies and switch branches. Could we have bazel run yarn install or
verify automatically?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#21431 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAC5Iz2vhAZIbSmpnRzesT3-tGzDXUtiks5tJHg5gaJpZM4RYqGR>
.
|
I see that you just added the
If you want your PR to be merged, it has to pass all the checks. But if you have a good reason to want to merge this, please contact the caretaker to let them know. |
packages/bazel/src/ng_package.bzl
Outdated
inputs = collect_es6_sources(ctx).to_list(), | ||
executable = ctx.executable._rollup, | ||
arguments = [ | ||
# TODO(i): how to get this path in a proper way?!?!? |
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.
@alexeagle can you please advise?
packages/bazel/src/ng_package.bzl
Outdated
license_banner = '' | ||
if (license_banner_file != None): | ||
# TODO(i): read the file to string for realzzz!!!! | ||
license_banner = str(license_banner_file); |
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.
@alexeagle how does one read contents of a file? is that even possible? I'm guessing that the file doesn't even exist yet when this code is executing, but there surely must be a way to read a text file into a string.. no?
or is my only option to create a shell script or nodejs binary and read the file there?
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.
ctx.actions.template_file or so
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.
I think you meant https://docs.bazel.build/versions/master/skylark/lib/actions.html#expand_template but the problem with that is that the output is a File not a string. I think what I was trying to do is fundamentally incompatible with how bazel works - I can't read a file within the code of an action. I have to write an executable to do that or use a run_shell instead. I'll try run_shell first.
packages/bazel/src/ng_package.bzl
Outdated
outs = ["%s_package.stamped.json" % name], | ||
stamp = True, | ||
cmd = "sed \"s/0.0.0-PLACEHOLDER/$$(grep BUILD_SCM_HASH bazel-out/volatile-status.txt | cut -d' ' -f 2)/\" $< > $@", |
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.
uhh.. this seems gross. isn't there a better way to do this?
798113a
to
ac9bbf9
Compare
integration/ng_package/BUILD.bazel
Outdated
# TODO(i): move ng_package with all tests and source code into a separate directory within packages/bazel | ||
# this will require: | ||
# - moving things around within packages/bazel/src | ||
# - getting rid of the nested workspace so that we can use //packages/core:package to test against |
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.
not necessarily, the workspaces are peers. From packages/bazel
you can depend on @angular_src//
- we already do this so that the ng_module
implementation can reference compiler-cli
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.
I tried that but got weird errors from within the label (I was able to invoke the label, but the label failed to execute when invoked in this way - I didn't debug it)
integration/ng_package/BUILD.bazel
Outdated
srcs = glob(["*.ts"]), | ||
tsconfig = ":tsconfig.json", | ||
# TODO(i): this doesn't work - I can't seem to be able to reference this label.. | ||
# Maybe it's because the npm_install happens via the weird WORKSPACE setup function? |
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.
is it @bazel_ng_package_deps//...
?
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.
why @
? I'd expect it to be @angular_src//:bazel_ng_package_deps
or :bazel_ng_package_deps
. are all labels created via WORKSPACE prefixed with @ or what?
packages/common/BUILD.bazel
Outdated
) | ||
|
||
ng_package_entry_point( | ||
name = "http", |
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.
all this repetition sucks. can't we just provide the map once as the union of all these entries?
If we need to trim it for each entry point, can't we do this in the action?
then we don't need ng_package_entry_point rule
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.
yeah. I'm not happy about it. this is already way better than what Chuck had in his branch. +1 for streamlining this more.
packages/common/http/BUILD.bazel
Outdated
@@ -1,8 +1,8 @@ | |||
package(default_visibility = ["//visibility:public"]) | |||
|
|||
load("@build_bazel_rules_typescript//:defs.bzl", "ts_library") | |||
load("@angular//:index.bzl", "ng_module") |
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 generally load from //tools:defaults.bzl
so everything in the repo is built with the one tsconfig the editor sees
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.
oh. I need to rebase to get defaults.bzl
# Release support # | ||
############################### | ||
|
||
build --workspace_status_command=./tools/bazel_stamp_vars.sh |
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.
note for me: we need to test with --nostamp
to make sure it doesn't blow up
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.
can you explain?
4959b19
to
b350a6e
Compare
You can preview b350a6e at https://pr21431-b350a6e.ngbuilds.io/. |
b350a6e
to
80f71f7
Compare
You can preview 80f71f7 at https://pr21431-80f71f7.ngbuilds.io/. |
80f71f7
to
8288bdf
Compare
You can preview 8288bdf at https://pr21431-8288bdf.ngbuilds.io/. |
You can preview d266b67 at https://pr21431-d266b67.ngbuilds.io/. |
the esm5 build fails with:
I haven't debugged this yet. I'll try tomorrow. @alexeagle does this look familiar? your branch gave me the same error before I cherry-picked it over. |
Hi @IgorMinar! This PR has merge conflicts due to recent upstream merges. |
e31c196
to
d673a71
Compare
You can preview d673a71 at https://pr21431-d673a71.ngbuilds.io/. |
b94ab77
to
96f6f2f
Compare
You can preview 96f6f2f at https://pr21431-96f6f2f.ngbuilds.io/. |
You can preview 080156b at https://pr21431-080156b.ngbuilds.io/. |
Hi @IgorMinar! This PR has merge conflicts due to recent upstream merges. |
2 similar comments
Hi @IgorMinar! This PR has merge conflicts due to recent upstream merges. |
Hi @IgorMinar! This PR has merge conflicts due to recent upstream merges. |
Hi @IgorMinar! This PR has merge conflicts due to recent upstream merges. |
1 similar comment
Hi @IgorMinar! This PR has merge conflicts due to recent upstream merges. |
landing a rebased subset of this in #22176 |
Obsolete; |
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. |
No description provided.