-
Notifications
You must be signed in to change notification settings - Fork 12k
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
build: use Bazel from npm #14170
build: use Bazel from npm #14170
Conversation
@@ -20,6 +20,10 @@ | |||
], | |||
"scripts": { | |||
"admin": "node ./bin/devkit-admin", |
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.
Nit: would be nice to have "bazel": "bazel",
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.
npx bazel
?
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.
Having the script proper is better I feel. It makes it pretty obvious that it's the right way of using bazel
, whether via yarn
or npm
, and it's more ergonomic for scripts too. I don't know if all the docker images we use have npx
either, but I guess they would.
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 comes with npm. If you’re only using yarn you should be able to just do ‘yarn bazel’
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 you can yarn bazel
if you want to pick your targets.
1a3d1e9
to
2a7dbd3
Compare
Also hook up bazel format/lint
need to fix them upstream
19df2c2
to
84a57fe
Compare
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.
Mostly LGTM, a few requests.
@@ -73,6 +73,14 @@ const ignoredPackages = [ | |||
'[email protected]', // Apache-2.0 but broken license in package.json | |||
'[email protected]', // zlib license | |||
|
|||
// * Need to publish these with LICENSE files | |||
// see https://github.com/bazelbuild/rules_nodejs/pull/696 |
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.
bazel-contrib/rules_nodejs#696 is now merged, can we get a release that removes this TODO?
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.
well, the npm version numbers for the bazel packages are identical to the upstream version numbers. If I publish @bazel/[email protected]
it will be confusing since the latest is https://github.com/bazelbuild/bazel/releases/tag/0.24.1
var_1: &docker_image circleci/node:10.12 | ||
var_2: &cache_key angular_devkit-0.10.0-{{ checksum "yarn.lock" }} | ||
var_1: &default_docker_image circleci/node:10.12 | ||
var_2: &browsers_docker_image circleci/node:10.12-browsers |
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 this needed? We use puppeteer
for our e2e tests so I didn't expect it to be necessary.
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.
sadly headless chrome still requires dynamic linked dependency on libx11 which is pretty lame. We should raise with the Chrome team.
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. |
Also hook up bazel format/lint