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

Managed directories should rerun repository rule if directory is deleted or timestamp changes #8487

Closed
gregmagolan opened this issue May 29, 2019 · 9 comments
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: bug

Comments

@gregmagolan
Copy link
Contributor

gregmagolan commented May 29, 2019

Description of the problem / feature request:

rules_nodejs yarn_install & npm_install with managed directories can't handle deleted or manually regenerated node_modules folder. See bazel-contrib/rules_nodejs#802 for full description.

Feature requests: what underlying problem are you trying to solve with this feature?

When using managed directories with yarn_install & npm_install in rules_nodejs 0.30.0+, when a user deletes the node_modules folder and runs bazel the repository rule should re-run (managed directory missing) which would recreate the node_modules folder from scratch.

If a user deletes the node_modules folder and recreates it manually using yarn or npm install then the repository rule should also re-run in that case as well (managed directory timestamp changed).

Currently after deleting node_modules, the only way to get back to a working build (besides modifying package.json or the lock file) is to run bazel clean --expunge to clear out the external repository and force the yarn_install or npm_install rule to rerun.

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

case 1:

git clone https://github.com/gregmagolan/bazel_with_users_node_modules
cd bazel_with_users_node_modules
bazel version
bazel test ...
rm -rf ./node_modules
bazel test ...

case 2:

git clone https://github.com/gregmagolan/bazel_with_users_node_modules
cd bazel_with_users_node_modules
bazel version
bazel test ...
rm -rf ./node_modules
yarn install
bazel test ...

error in both cases:

greg@macbook bazel_with_users_node_modules (master) $ bazel test ...
ERROR: /private/var/tmp/_bazel_greg/8cb9ee79286b251e74f070e4ed053074/external/npm/typescript/BUILD.bazel:123:1: no such package '@npm//node_modules/typescript': BUILD file not found on package path and referenced by '@npm//typescript:typescript'
ERROR: Analysis of target '//:main' failed; build aborted: Analysis failed
INFO: Elapsed time: 0.205s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (0 packages loaded, 0 targets configured)
FAILED: Build did NOT complete successfully (0 packages loaded, 0 targets configured)

What operating system are you running Bazel on?

OSX but issue is OS agnostic.

What's the output of bazel info release?

release 0.26.0-homebrew

@gregmagolan
Copy link
Contributor Author

/cc @irengrig @alexeagle

@aiuto aiuto added team-Local-Exec Issues and PRs for the Execution (Local) team untriaged labels May 31, 2019
irengrig added a commit to irengrig/bazel that referenced this issue Jun 5, 2019
…ess:

- If managed directory is declared for the external repository and does not exist (probably was deleted by user), make external repository dirty and re-fetch it.
- Add tests to demonstrate the added behavior to ManagedDirectoriesBlackBoxTest.
- Closes bazelbuild#8487.
gregmagolan added a commit to gregmagolan/angular that referenced this issue Jun 7, 2019
* entry_point attribute of nodejs_binary & rollup_bundle is now a label
* symlinking of node_modules for yarn_install temporarily disabled (except for integration/bazel) until the fix for bazelbuild/bazel#8487 makes it into a future bazel release
gregmagolan added a commit to gregmagolan/angular that referenced this issue Jun 7, 2019
* entry_point attribute of nodejs_binary & rollup_bundle is now a label
* symlinking of node_modules for yarn_install temporarily disabled (except for integration/bazel) until the fix for bazelbuild/bazel#8487 makes it into a future bazel release
gregmagolan added a commit to gregmagolan/angular that referenced this issue Jun 10, 2019
…SPACE and bazel schematics

This is needed to work-around issue: yarn_install & npm_install with managed directories can't handle deleted or manually regenerated node_modules folder [bazel-contrib/rules_nodejs#802]
Underlying issue has been fixed in Bazel bazelbuild/bazel#8487 but hasn't landed in a release yet
gregmagolan added a commit to gregmagolan/angular that referenced this issue Jun 10, 2019
* entry_point attribute of nodejs_binary & rollup_bundle is now a label
* symlinking of node_modules for yarn_install temporarily disabled (except for integration/bazel) until the fix for bazelbuild/bazel#8487 makes it into a future bazel release
gregmagolan added a commit to gregmagolan/angular that referenced this issue Jun 10, 2019
…SPACE and bazel schematics

This is needed to work-around issue: yarn_install & npm_install with managed directories can't handle deleted or manually regenerated node_modules folder [bazel-contrib/rules_nodejs#802]
Underlying issue has been fixed in Bazel bazelbuild/bazel#8487 but hasn't landed in a release yet
gregmagolan added a commit to gregmagolan/angular that referenced this issue Jun 10, 2019
* entry_point attribute of nodejs_binary & rollup_bundle is now a label
* symlinking of node_modules for yarn_install temporarily disabled (except for integration/bazel) until the fix for bazelbuild/bazel#8487 makes it into a future bazel release
gregmagolan added a commit to gregmagolan/angular that referenced this issue Jun 10, 2019
…SPACE and bazel schematics

This is needed to work-around issue: yarn_install & npm_install with managed directories can't handle deleted or manually regenerated node_modules folder [bazel-contrib/rules_nodejs#802]
Underlying issue has been fixed in Bazel bazelbuild/bazel#8487 but hasn't landed in a release yet
IgorMinar pushed a commit to angular/angular that referenced this issue Jun 11, 2019
* entry_point attribute of nodejs_binary & rollup_bundle is now a label
* symlinking of node_modules for yarn_install temporarily disabled (except for integration/bazel) until the fix for bazelbuild/bazel#8487 makes it into a future bazel release

PR Close #30627
IgorMinar pushed a commit to angular/angular that referenced this issue Jun 11, 2019
…SPACE and bazel schematics (#30627)

This is needed to work-around issue: yarn_install & npm_install with managed directories can't handle deleted or manually regenerated node_modules folder [bazel-contrib/rules_nodejs#802]
Underlying issue has been fixed in Bazel bazelbuild/bazel#8487 but hasn't landed in a release yet

PR Close #30627
gregmagolan added a commit to gregmagolan/angular that referenced this issue Jun 11, 2019
* Entry_point attribute of nodejs_binary & rollup_bundle is now a label
* Nodejs rules 0.30.1 has new feature to symlink node_modules with yarn_install and bazel 0.26.0 includes new managed_directories feature which enables this
* Symlinking of node_modules for yarn_install temporarily disabled (except for integration/bazel) until the fix for bazelbuild/bazel#8487 makes it into a future bazel release. This is needed to work-around issue: yarn_install & npm_install with managed directories can't handle deleted or manually regenerated node_modules folder [bazel-contrib/rules_nodejs#802]. Underlying issue has been fixed in Bazel bazelbuild/bazel#8487 but hasn't landed in a release yet
gregmagolan added a commit to gregmagolan/angular that referenced this issue Jun 11, 2019
* entry_point attribute of nodejs_binary & rollup_bundle is now a label
* nodejs rules 0.30.1 has new feature to symlink node_modules with yarn_install and bazel 0.26.0 includes new managed_directories feature which enables this
* Symlinking of node_modules for yarn_install temporarily disabled (except for integration/bazel) until the fix for bazelbuild/bazel#8487 makes it into a future bazel release. This is needed to work-around issue: yarn_install & npm_install with managed directories can't handle deleted or manually regenerated node_modules folder [bazel-contrib/rules_nodejs#802]. Underlying issue has been fixed in Bazel bazelbuild/bazel#8487 but hasn't landed in a release yet
AndrewKushnir pushed a commit to angular/angular that referenced this issue Jun 11, 2019
* entry_point attribute of nodejs_binary & rollup_bundle is now a label
* nodejs rules 0.30.1 has new feature to symlink node_modules with yarn_install and bazel 0.26.0 includes new managed_directories feature which enables this
* Symlinking of node_modules for yarn_install temporarily disabled (except for integration/bazel) until the fix for bazelbuild/bazel#8487 makes it into a future bazel release. This is needed to work-around issue: yarn_install & npm_install with managed directories can't handle deleted or manually regenerated node_modules folder [bazel-contrib/rules_nodejs#802]. Underlying issue has been fixed in Bazel bazelbuild/bazel#8487 but hasn't landed in a release yet

PR Close #30965
laurentlb pushed a commit that referenced this issue Jun 12, 2019
…ess.

- If managed directory is declared for the external repository and does not exist (probably was deleted by user), make external repository dirty and re-fetch it.
- Add tests to demonstrate the added behavior to ManagedDirectoriesBlackBoxTest.
- Closes #8487.

Closes #8564.

PiperOrigin-RevId: 251882207
ayazhafiz pushed a commit to ayazhafiz/angular that referenced this issue Jun 12, 2019
* entry_point attribute of nodejs_binary & rollup_bundle is now a label
* symlinking of node_modules for yarn_install temporarily disabled (except for integration/bazel) until the fix for bazelbuild/bazel#8487 makes it into a future bazel release

PR Close angular#30627
ayazhafiz pushed a commit to ayazhafiz/angular that referenced this issue Jun 12, 2019
…SPACE and bazel schematics (angular#30627)

This is needed to work-around issue: yarn_install & npm_install with managed directories can't handle deleted or manually regenerated node_modules folder [bazel-contrib/rules_nodejs#802]
Underlying issue has been fixed in Bazel bazelbuild/bazel#8487 but hasn't landed in a release yet

PR Close angular#30627
@gregmagolan
Copy link
Contributor Author

gregmagolan commented Jun 12, 2019

Tested 0.27.0rc5 in my repro repo and the first case of the node_modules folder being deleted is fixed:

greg@macbook tmp1 $ git clone https://github.com/gregmagolan/bazel_with_users_node_modules
Cloning into 'bazel_with_users_node_modules'...
remote: Enumerating objects: 42, done.
remote: Counting objects: 100% (42/42), done.
remote: Compressing objects: 100% (31/31), done.
remote: Total 42 (delta 14), reused 37 (delta 9), pack-reused 0
Unpacking objects: 100% (42/42), done.

greg@macbook tmp1 $ cd bazel_with_users_node_modules

greg@macbook bazel_with_users_node_modules (master) $ bazel version
Starting local Bazel server and connecting to it...
Build label: 0.27.0rc5
Build target: bazel-out/darwin-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Wed Jun 12 12:10:43 2019 (1560341443)
Build timestamp: 1560341443
Build timestamp as int: 1560341443

greg@macbook bazel_with_users_node_modules (master) $ bazel test ...
yarn install v1.13.0
warning package.json: No license field
warning package-lock.json found. Your project contains lock files generated by tools other than Yarn. It is advised not to mix package managers in order to avoid resolution inconsistencies caused by unsynchronized lock files. To clear this warning, remove package-lock.json.
warning No license field
[1/4] Resolving packages...
[2/4] Fetching packages...
info @bazel/[email protected]: The platform "darwin" is incompatible with this module.
info "@bazel/[email protected]" is an optional dependency and failed compatibility check. Excluding it from installation.
info @bazel/[email protected]: The platform "darwin" is incompatible with this module.
info "@bazel/[email protected]" is an optional dependency and failed compatibility check. Excluding it from installation.
[3/4] Linking dependencies...
[4/4] Building fresh packages...
Done in 0.36s.
INFO: Analyzed target //:main (30 packages loaded, 436 targets configured).
INFO: Found 1 test target...
Target //:main up-to-date:
  bazel-bin/main_bin.sh
  bazel-bin/main
INFO: Elapsed time: 13.486s, Critical Path: 0.56s
INFO: 2 processes: 2 darwin-sandbox.
INFO: Build completed successfully, 7 total actions
//:main                                                                  PASSED in 0.4s

Executed 1 out of 1 test: 1 test passes.
There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see which ones these arINFO: Build completed successfully, 7 total actions

greg@macbook bazel_with_users_node_modules (master) $ rm -rf ./node_modules

greg@macbook bazel_with_users_node_modules (master) $ bazel test ...
yarn install v1.13.0
warning package.json: No license field
warning package-lock.json found. Your project contains lock files generated by tools other than Yarn. It is advised not to mix package managers in order to avoid resolution inconsistencies caused by unsynchronized lock files. To clear this warning, remove package-lock.json.
warning No license field
[1/4] Resolving packages...
[2/4] Fetching packages...
info @bazel/[email protected]: The platform "darwin" is incompatible with this module.
info "@bazel/[email protected]" is an optional dependency and failed compatibility check. Excluding it from installation.
info @bazel/[email protected]: The platform "darwin" is incompatible with this module.
info "@bazel/[email protected]" is an optional dependency and failed compatibility check. Excluding it from installation.
[3/4] Linking dependencies...
[4/4] Building fresh packages...
Done in 0.31s.
INFO: Analyzed target //:main (1 packages loaded, 79 targets configured).
INFO: Found 1 test target...
Target //:main up-to-date:
  bazel-bin/main_bin.sh
  bazel-bin/main
INFO: Elapsed time: 1.164s, Critical Path: 0.00s
INFO: 0 processes.
INFO: Build completed successfully, 1 total action
//:main                                                         (cached) PASSED in 0.4s

Executed 0 out of 1 test: 1 test passes.
There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see which ones these arINFO: Build completed successfully, 1 total action

@gregmagolan
Copy link
Contributor Author

gregmagolan commented Jun 12, 2019

The 2nd case described where node_modules is deleted and then manually re-created with yarn install or npm install outside of Bazel is still broken.

Repro is:

git clone https://github.com/gregmagolan/bazel_with_users_node_modules
cd bazel_with_users_node_modules
bazel version
bazel test ...
rm -rf ./node_modules
yarn install
bazel test ...

=>

greg@macbook tmp2 $ git clone https://github.com/gregmagolan/bazel_with_users_node_modules
Cloning into 'bazel_with_users_node_modules'...
remote: Enumerating objects: 42, done.
remote: Counting objects: 100% (42/42), done.
remote: Compressing objects: 100% (31/31), done.
remote: Total 42 (delta 14), reused 37 (delta 9), pack-reused 0
Unpacking objects: 100% (42/42), done.

greg@macbook tmp2 $ cd bazel_with_users_node_modules

greg@macbook bazel_with_users_node_modules (master) $ bazel version
Starting local Bazel server and connecting to it...
Build label: 0.27.0rc5
Build target: bazel-out/darwin-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Wed Jun 12 12:10:43 2019 (1560341443)
Build timestamp: 1560341443
Build timestamp as int: 1560341443

greg@macbook bazel_with_users_node_modules (master) $ bazel test ...
Loading: 0 packages loaded
rm -rf ./node_modules
yarn install
bazel test ...
Loading: 0 packages loaded
yarn install v1.13.0
warning package.json: No license field
warning package-lock.json found. Your project contains lock files generated by tools other than Yarn. It is advised not to mix package managers in order to avoid resolution inconsistencies caused by unsynchronized lock files. To clear this warning, remove package-lock.json.
warning No license field
[1/4] Resolving packages...
[2/4] Fetching packages...
info @bazel/[email protected]: The platform "darwin" is incompatible with this module.
info "@bazel/[email protected]" is an optional dependency and failed compatibility check. Excluding it from installation.
info @bazel/[email protected]: The platform "darwin" is incompatible with this module.
info "@bazel/[email protected]" is an optional dependency and failed compatibility check. Excluding it from installation.
[3/4] Linking dependencies...
[4/4] Building fresh packages...
Done in 0.54s.
INFO: Analyzed target //:main (30 packages loaded, 432 targets configured).
INFO: Found 1 test target...
Target //:main up-to-date:
  bazel-bin/main_bin.sh
  bazel-bin/main
INFO: Elapsed time: 13.991s, Critical Path: 0.51s
INFO: 2 processes: 2 darwin-sandbox.
INFO: Build completed successfully, 7 total actions
//:main                                                                  PASSED in 0.4s

Executed 1 out of 1 test: 1 test passes.
There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see which ones these arINFO: Build completed successfully, 7 total actions

greg@macbook bazel_with_users_node_modules (master) $ rm -rf ./node_modules

greg@macbook bazel_with_users_node_modules (master) $ yarn install
yarn install v1.13.0
warning package.json: No license field
warning package-lock.json found. Your project contains lock files generated by tools other than Yarn. It is advised not to mix package managers in order to avoid resolution inconsistencies caused by unsynchronized lock files. To clear this warning, remove package-lock.json.
warning No license field
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
info @bazel/[email protected]: The platform "darwin" is incompatible with this module.
info "@bazel/[email protected]" is an optional dependency and failed compatibility check. Excluding it from installation.
info @bazel/[email protected]: The platform "darwin" is incompatible with this module.
info "@bazel/[email protected]" is an optional dependency and failed compatibility check. Excluding it from installation.
[3/4] 🔗  Linking dependencies...
[4/4] 🔨  Building fresh packages...
✨  Done in 0.34s.

greg@macbook bazel_with_users_node_modules (master) $ bazel test ...
ERROR: /private/var/tmp/_bazel_greg/394d38fc2d1ee1f2aaf1fa189803b84d/external/npm/typescript/BUILD.bazel:123:1: no such package '@npm//node_modules/typescript': BUILD file not found in directory 'node_modules/typescript' of external repository @npm and referenced by '@npm//typescript:typescript'
ERROR: Analysis of target '//:main' failed; build aborted: Analysis failed
INFO: Elapsed time: 0.116s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (0 packages loaded, 0 targets configured)
FAILED: Build did NOT complete successfully (0 packages loaded, 0 targets configured)

@alexeagle alexeagle reopened this Jun 13, 2019
irengrig added a commit to irengrig/bazel that referenced this issue Jun 18, 2019
…ess.

- If managed directory is declared for the external repository and does not exist (probably was deleted by user), make external repository dirty and re-fetch it.
- Add tests to demonstrate the added behavior to ManagedDirectoriesBlackBoxTest.
- Closes bazelbuild#8487.

Closes bazelbuild#8564.

PiperOrigin-RevId: 251882207
irengrig added a commit to irengrig/bazel that referenced this issue Jul 15, 2019
…ess.

- If managed directory is declared for the external repository and does not exist (probably was deleted by user), make external repository dirty and re-fetch it.
- Add tests to demonstrate the added behavior to ManagedDirectoriesBlackBoxTest.
- Closes bazelbuild#8487.

Closes bazelbuild#8564.

PiperOrigin-RevId: 251882207
@schroederc
Copy link
Contributor

Is this fixed in any released version?

@irengrig
Copy link
Contributor

irengrig commented Aug 8, 2019

The fix to determine if the node_modules (managed directory) exists is in Bazel 0.27.1.
The other part, about the node_modules (managed directories) being deleted and re-created, is tracked here: #9080

@rupertks rupertks added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. and removed team-Local-Exec Issues and PRs for the Execution (Local) team labels Aug 22, 2019
@kylecordes
Copy link

I think this is a potentially big problem - is it really solved by the proposed solution?

Out in the wild, it's common for developers to perform operations that mutate contents of node_modules. It's not rare for Node-based software to write to node_modules. Therefore, it seems insufficient (for a correct and reliable development experience) to just look for a missing or time stamped node_modules as a signal to recreate.

So I wonder if this is really a case echoing some other discussion (forget where) about the need for users to have (read-only) access to intermediate build output like generated code.

How can a writable node_modules exposed to users or tools be trusted to contain the results of a yarn-install?

At the moment, my impression from all this (maybe wrong) is that to get a reliable build, a user must freshly:

  • rm -rf node_modules (sometimes more than one in a complex project)
  • bazel build --expunge
  • Wait for lots of stuff to be rebuilt needlessly, if it’s a big project

What can be done to make node_modules (both what users see, and what rules see) reliably contain a pure function of the contents of package.json, lock file, and upstream NPM repos?

@laurentlb laurentlb added P3 We're not considering working on this, but happy to review a PR. (No assignee) type: bug and removed untriaged labels Jan 9, 2020
@migueloller
Copy link

@kylecordes, if Node-based software truly writes to node_modules then there would be an issue whether managed directories is being used or not.

More info here: https://docs.bazel.build/versions/master/skylark/repository_rules.html#when-is-the-implementation-function-executed

A way to avoid having to run those commands is to just call bazel sync. This will take care of refetching the repositories. According to the help documentation this command seems to be experimental and the semantics of it might change but it seems like the tool for solving this specific job. A big improvement to this command would be the ability to sync a specific label to avoid fetching all the repos in the workspace.

@philwo philwo added the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Jun 15, 2020
@alexeagle
Copy link
Contributor

One possible way to resolve some of the user pain is for bazel clean --expunge to delete the managed directory in the user's sources at the same time the external repository is deleted. At least this way there's a single method for uncorrupting the external repo state, rather than also requiring rm -rf node_modules

@philwo philwo removed the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Nov 29, 2021
@gregmagolan
Copy link
Contributor Author

No longer needed as managed directories is removed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants