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

--incompatible_disable_managed_directories: deprecate managed_directories() #15463

Closed
lberki opened this issue May 11, 2022 · 5 comments
Closed
Assignees
Labels
incompatible-change Incompatible/breaking change P2 We'll consider working on this in future. (Assignee optional) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file.

Comments

@lberki
Copy link
Contributor

lberki commented May 11, 2022

The reasons for deprecation and the migration steps are documented in the design doc:

https://docs.google.com/document/d/1u9V5RUc7i6Urh8gGfnSurxpWA7JMRtwCi1Pr5BHeE44/edit

This represents a loss of functionality for Bazel. We believe this is possible because the motivating use case for managed_directories() was rules_nodejs, which now recommends against using it.

The incompatible flag will remain available for a month.

@lberki lberki added P2 We'll consider working on this in future. (Assignee optional) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. incompatible-change Incompatible/breaking change labels May 11, 2022
@lberki lberki self-assigned this May 11, 2022
@SrodriguezO
Copy link
Contributor

SrodriguezO commented May 11, 2022

We use managed_directories at Lucid to avoid needing to install node_modules manually for tooling to work. We clean expunge between CI builds anyway, so we don't experience the mentioned downside from the node_nodules directory also being deleted between builds.

The contents of our managed directory change often, and requiring devs to run some command in order to bring their workspace up-to-date would become tiresome. It'd be extra painful for developers using worktrees. It's convenient for it to happen automatically whenever Bazel loads.

We don't absolutely need this feature, just chiming in that we currently do rely on it.

@alexeagle
Copy link
Contributor

for tooling to work

Could you give some motivating examples there? What tooling do you run outside of Bazel, where you'd want versions of those tools to update following a bazel command? I can think of typescript as an obvious case, where an editor will use the version of TypeScript found in the node_modules folder and you'd want developers getting intellisense for the same TS version as Bazel will run to type-check their code. Do you have others?

@SrodriguezO
Copy link
Contributor

When the typescript language server is running, it must be able to find dependencies in order to provide type information and go-to support.

Additionally, we use some IDE extensions that rely on the presence of node_modules, such as ESLint and Prettier. Although our definitive "formatter" that runs in CI is powered by Bazel, the IDE extensions improve our developers' dev experience as they get mostly-correct formatting and linting on the files they're working with.

It's nice to get this "for free" when Bazel loads rather than needing manual intervention (especially for developers using git worktrees)

@lberki
Copy link
Contributor Author

lberki commented May 16, 2022

@SrodriguezO : given the "don't absolutely need this feature" part, I won't consider this a blocking issue.

Maybe you can get away with putting node_modules into .bazelignore and rigging some script up to symlink that to wherever rules_nodejs puts its node_modules directory?

@alexeagle
Copy link
Contributor

@SrodriguezO I see how that's a handy workflow. But it only happens to work when the user's workflow is

  • change git worktrees
  • run a bazel build of a target that depends on the npm install
  • try to run a tool outside of Bazel

Obviously if you just changed worktrees and then ran the tool, nothing would have updated the node_modules directory. But I understand you have this working okay for you.

There's yet another line of reasoning for this change:

Having a single node_modules folder for the entire project in the external folder is not the only way to integrate npm with Bazel. In aspect-build/rules_js for example we use pnpm as the package manager (rather than npm or yarn) which uses a "content addressable store" for the downloaded packages in external. Then the node_modules tree is constructed in bazel-bin per-action, containing only packages needed for it.

As such, I think Bazel core shouldn't have to carry a feature which is particular to just one NodeJS implementation choice, and not inherent to that problem.

@lberki lberki changed the title --incompatible_disallow_managed_directories: deprecate managed_directories() --incompatible_disable_managed_directories: deprecate managed_directories() May 18, 2022
bazel-io pushed a commit that referenced this issue May 18, 2022
This change implements the design doc "Deprecating managed_directories".

If your build is broken by this change, you can temporarily re-enable the
functionality using the `--noincompatible_disable_managed_directories` command
line option.

Tracking bug: #15463

RELNOTES[INC]: workspace(managed_directories=) is not available anymore.

PiperOrigin-RevId: 449469507
hanwen pushed a commit to GerritCodeReview/gerrit that referenced this issue Jun 7, 2022
Bazel removed `managed_directories` feature on Bazel@HEAD, see: [1].
Design document is here: [2].

Also extend the .bazelignore and add explicitly the path to the
node_modules directories. Note, that Bazel currently doesn't support
the same semantic as .gitignore. For more details see this issue: [3]
and this issue specific to rules_nodejs: [4].

[1] bazelbuild/bazel#15463
[2] https://docs.google.com/document/d/1u9V5RUc7i6Urh8gGfnSurxpWA7JMRtwCi1Pr5BHeE44/edit
[3] bazelbuild/bazel#7093
[4] bazelbuild/bazel#8106

Release-Notes: skip
Change-Id: I5dc582e05e4116064fc06d438d4b8a8b57b6bb8d
aranguyen pushed a commit to aranguyen/bazel that referenced this issue Jun 27, 2022
Fixes bazelbuild#15463.

The design doc for the deprecation and the removal is available here:

https://docs.google.com/document/d/1u9V5RUc7i6Urh8gGfnSurxpWA7JMRtwCi1Pr5BHeE44/edit

RELNOTES: --noincompatible_disable_managed_directories, and with that, workspace(managed_directories=) is not supported anymore.
PiperOrigin-RevId: 456228902
Change-Id: I5fcbf96b9a508f47cbcabf9715163cd7120020bf
iain-macdonald added a commit to buildbuddy-io/buildbuddy that referenced this issue Nov 21, 2022
This option is deprecated. The recommended replacement is to add an
option to npm_install directives, but we don't have any in this repo.
More here: bazelbuild/bazel#15463
tylersaunders added a commit to thesummitdev/rook that referenced this issue Dec 24, 2022
jonathan-enf added a commit to jonathan-enf/enkit that referenced this issue Dec 26, 2022
amuramoto added a commit to googlemaps/openapi-specification that referenced this issue Jan 11, 2023
- Removes deprecated managed_directories, see bazelbuild/bazel#15463
-  Fixes issue with @build_bazel_rules_nodejs, see bazel-contrib/rules_nodejs#2733 (comment)
- Sets test action to use Ubuntu-20.04 because it's needed for pkg_tar (bazelbuild/bazel#11554) since python2 was removed from Ubuntu after 20.04.
amuramoto added a commit to googlemaps/openapi-specification that referenced this issue Jan 12, 2023
- Removes deprecated managed_directories, see bazelbuild/bazel#15463
-  Fixes issue with @build_bazel_rules_nodejs, see bazel-contrib/rules_nodejs#2733 (comment)
- Sets test action to use Ubuntu-20.04 because it's needed for pkg_tar (bazelbuild/bazel#11554) since python2 was removed from Ubuntu after 20.04.
amuramoto added a commit to googlemaps/openapi-specification that referenced this issue Jan 12, 2023
- Removes deprecated managed_directories, see bazelbuild/bazel#15463
-  Fixes issue with @build_bazel_rules_nodejs, see bazel-contrib/rules_nodejs#2733 (comment)
- Sets test action to use Ubuntu-20.04 because it's needed for pkg_tar (bazelbuild/bazel#11554) since python2 was removed from Ubuntu after 20.04.
amuramoto added a commit to googlemaps/openapi-specification that referenced this issue Feb 2, 2023
- Removes deprecated managed_directories, see bazelbuild/bazel#15463
-  Fixes issue with @build_bazel_rules_nodejs, see bazel-contrib/rules_nodejs#2733 (comment)
- Sets test action to use Ubuntu-20.04 because it's needed for pkg_tar (bazelbuild/bazel#11554) since python2 was removed from Ubuntu after 20.04.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
incompatible-change Incompatible/breaking change P2 We'll consider working on this in future. (Assignee optional) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file.
Projects
None yet
Development

No branches or pull requests

3 participants