Skip to content
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

Remove install_bazel_dependencies #1877

Closed
alexeagle opened this issue May 7, 2020 · 7 comments
Closed

Remove install_bazel_dependencies #1877

alexeagle opened this issue May 7, 2020 · 7 comments
Assignees
Labels
cleanup Tech debt, resolving it improves our own velocity
Milestone

Comments

@alexeagle
Copy link
Collaborator

alexeagle commented May 7, 2020

As a consequence of making all packages loaded from their @npm//@bazel/... locations, we no longer need to call a repository rule to load from @npm to create new workspaces.

This is important to remove from users code as much as possible since it prevents lazy-installing npm dependencies only for builds that involve frontend. (eg in a monorepo with backend code those devs shouldn't wait for npm installs)

Also consider renaming functions like "npm_bazel_karma_dependencies"? Not sure if it's worth the churn though.

Also consider removing support for the "bazelWorkspaces" package.json key which is now vestigial and maybe no one uses

@alexeagle alexeagle added this to the 2.0 milestone May 7, 2020
@alexeagle alexeagle self-assigned this May 7, 2020
@alexeagle alexeagle added the cleanup Tech debt, resolving it improves our own velocity label May 7, 2020
@alexeagle
Copy link
Collaborator Author

Check that we removed vendor_external in all the places in pkg_npm rules

@alexeagle
Copy link
Collaborator Author

  • stop using name = "npm" in some/all of the examples/e2e to prove it works with no other changes needed

@alexeagle
Copy link
Collaborator Author

working on this in #1921

@alexeagle
Copy link
Collaborator Author

Okay, I merged c18a98d so we no longer recommend using install_bazel_dependencies, and warn if you still use it.

Verified vendor_external removed in appropriate places. I'll send another PR now to change some examples to not use "npm".

I think we should leave it in this state for one major so users (Angular ng_module in particular) have a chance to do a migration while getting advantages of our 2.0. So I'll bump the rest of this issue to 3.0 milestone. We should remove install_bazel_depnedencies and the bazelWorkspaces package.json key handling at that time.

@alexeagle alexeagle modified the milestones: 2.0, 3.0 May 28, 2020
@smhmayboudi
Copy link

smhmayboudi commented Jun 25, 2020

I receive this message:

  • bazelisk --version: bazel 3.3.0
  • node -v: v12.8.1
  • yarn -v: 1.22.4
  • I only installed @bazel/typescript inside devDependencies at package.json
  • I installed buildifier and bazelisk with brew
$ bazelisk build --platforms=@build_bazel_rules_nodejs//toolchains/node:linux_amd64 //apps/action/src:action_nodejs_image
INFO: Invocation ID: 48c402b2-deff-42b8-862e-808728e0f673
ERROR: Failed to load Starlark extension '@npm_bazel_typescript//:index.bzl'.
Cycle in the workspace file detected. This indicates that a repository is used prior to being defined.
The following chain of repository dependencies lead to the missing definition.
 - @npm_bazel_typescript
This could either mean you have to add the '@npm_bazel_typescript' repository with a statement like `http_archive` in your WORKSPACE file (note that transitive dependencies are not added automatically), or move an existing definition earlier in your WORKSPACE file.
ERROR: cycles detected during target parsing
INFO: Elapsed time: 1.129s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (0 packages loaded)bazelisk build --platforms=@build_bazel_rules_nodejs//toolchains/node:linux_amd64 //apps/action/src:action_nodejs_image
INFO: Invocation ID: 48c402b2-deff-42b8-862e-808728e0f673
ERROR: Failed to load Starlark extension '@npm_bazel_typescript//:index.bzl'.
Cycle in the workspace file detected. This indicates that a repository is used prior to being defined.
The following chain of repository dependencies lead to the missing definition.
 - @npm_bazel_typescript
This could either mean you have to add the '@npm_bazel_typescript' repository with a statement like `http_archive` in your WORKSPACE file (note that transitive dependencies are not added automatically), or move an existing definition earlier in your WORKSPACE file.
ERROR: cycles detected during target parsing
INFO: Elapsed time: 1.129s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (0 packages loaded)

@alexeagle
Copy link
Collaborator Author

@mattem
Copy link
Collaborator

mattem commented Dec 9, 2020

closed via #2337

@mattem mattem closed this as completed Dec 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Tech debt, resolving it improves our own velocity
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants