-
Notifications
You must be signed in to change notification settings - Fork 25.3k
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
Rules nodejs 1.0.1 #34589
Rules nodejs 1.0.1 #34589
Conversation
4bfd91c
to
928c969
Compare
@josephperrott @IgorMinar
I'd recommend all angular developers turn on disk_cache in their
With that on, flipping between |
61d63ec
to
a7a2c7f
Compare
Dependency sandwich exists between material & angular so we need to patch both @angular/bazel & ts-api-guardian npm packages to land this so that the material-unit-test CI job in angular/angular#34589 can be greened up.
material-unit-tests CI job waiting on angular/components#18064 to land. Temporarily updated to my angular/components rules_nodejs 1.0.0 commit for https://github.com/angular/components/pull/18064to green up CI and will update to upstream commit once angular/components#18064 lands. |
@gregmagolan Just to confirm, what you show here confirms that the results of the test targets being build is stable and leads to us having full cache hits for Did you confirm that we also are seeing (as fully as possible), remote cache hits for the This is what I want to start looking into showing in our logs with something like #34439, so we can see how many of the individual spawned actions can be cache hits. |
Dependency sandwich exists between material & angular so we need to patch both @angular/bazel & ts-api-guardian npm packages to land this so that the material-unit-test CI job in angular/angular#34589 can be greened up.
Dependency sandwich exists between material & angular so we need to patch both @angular/bazel & ts-api-guardian npm packages to land this so that the material-unit-test CI job in angular/angular#34589 can be greened up.
8abea47
to
d927122
Compare
Dependency sandwich exists between material & angular so we need to patch both @angular/bazel & ts-api-guardian npm packages to land this so that the material-unit-test CI job in angular/angular#34589 can be greened up.
b91df22
to
f255213
Compare
Remote-cache is being hit for most actions. I don't have the number to compare from before this update yet but running those benchmarks now. On a clean build with --disk_cash enabled and pre-populated I got,
On clean build with --config=remote (OSX cross-platform) enabled and pre-populated and with no --disk_cache,
|
@josephperrott @IgorMinar Just a heads up that I initially updated to @angular/cli rc.7 but that breaks the cli-hello-world-ivy-i18n integration test. Other integration tests still pass so something particular about that one. This PR now just updated to @angular/cli rc.4 to pick up the "0.0.0" @angular/core version fix otherwise the following is observed.
Somewhere between @angular/cli rc.5 & rc.7 the |
Same benchmark on current Clean build with --disk_cash enabled and pre-populated:
On clean build with --config=remote (OSX cross-platform) enabled and pre-populated and with no --disk_cache:
|
Dependency sandwich exists between material & angular so we need to patch both @angular/bazel & ts-api-guardian npm packages to land this so that the material-unit-test CI job in angular/angular#34589 can be greened up.
Dependency sandwich exists between material & angular so we need to patch both @angular/bazel & ts-api-guardian npm packages to land this so that the material-unit-test CI job in angular/angular#34589 can be greened up.
Dependency sandwich exists between material & angular so we need to patch both @angular/bazel & ts-api-guardian npm packages to land this so that the material-unit-test CI job in angular/angular#34589 can be greened up.
743f3e5
to
fde9c07
Compare
Dependency sandwich exists between material & angular so we need to patch both @angular/bazel & ts-api-guardian npm packages to land this so that the material-unit-test CI job in angular/angular#34589 can be greened up.
…#18118) This is needed for the material unit test to pass on angular repo in angular/angular#34589 otherwise the patch fails (since the changes are already made in that context) and throws and the CI job fails with: ``` if (config.fatal) throw e; ^ Error: exec: at Object.error (/tmp/material2/node_modules/shelljs/src/common.js:110:27) at execSync (/tmp/material2/node_modules/shelljs/src/exec.js:102:12) at String._exec (/tmp/material2/node_modules/shelljs/src/exec.js:205:12) at String.<anonymous> (/tmp/material2/node_modules/shelljs/src/common.js:335:23) at applyPatch (/tmp/material2/tools/bazel/postinstall-patches.js:186:26) at Object.<anonymous> (/tmp/material2/tools/bazel/postinstall-patches.js:35:1) at Module._compile (internal/modules/cjs/loader.js:936:30) at Object.Module._extensions..js (internal/modules/cjs/loader.js:947:10) at Module.load (internal/modules/cjs/loader.js:790:32) at Function.Module._load (internal/modules/cjs/loader.js:703:12) error Command failed with exit code 1. ) ``
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. |
Update to first stable release of rules_nodejs.
Folds in rules_nodejs breaking changes that were made for 1.0 after BazelCoreWeb API review. The major one that affects the angular repo is the removal of the
bootstrap
attribute in nodejs_binary, nodejs_test and jasmine_node_test in favor of using templated_args--node_options=--require=/path/to/script
. The side-effect of this is that the bootstrap script does not get the require.resolve patches with explicitly loading the targets_loader.js
file.Brings in the fix for stamping which was preventing many targets from getting cached.
Breaking changes in rules_nodejs will be more controlled in the future.
This PR also updates integration/bazel and @angular/bazel schematics to rules_nodejs 1.0.0
For the purposes of the integration test the zone.js script & bundle script tags can just go into the source index.html itself. The purpose of the integration test is is to test @angular/bazel & ng_module & ng_package so there is no need to exercise html_insert_assets in integration/bazel.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information