Skip to content

Commit

Permalink
fix: limit concurrency when generating BUILD files in npm_install and…
Browse files Browse the repository at this point in the history
… yarn_install (#3509)

* chore: add .gitattributes to ignore changes in generated docs files on PR reviews

* fix: limit concurrency when generating BUILD files in npm_install and yarn_install
  • Loading branch information
gregmagolan authored Jul 25, 2022
1 parent c962f12 commit 4001716
Show file tree
Hide file tree
Showing 6 changed files with 82 additions and 14 deletions.
12 changes: 12 additions & 0 deletions .gitattributes
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
docs/Built-ins.md linguist-generated=true
docs/Concatjs.md linguist-generated=true
docs/Cypress.md linguist-generated=true
docs/esbuild.md linguist-generated=true
docs/Jasmine.md linguist-generated=true
docs/Karma.md linguist-generated=true
docs/Protractor.md linguist-generated=true
docs/Providers.md linguist-generated=true
docs/Rollup.md linguist-generated=true
docs/Terser.md linguist-generated=true
docs/Toolchains.md linguist-generated=true
docs/TypeScript.md linguist-generated=true
39 changes: 30 additions & 9 deletions docs/Built-ins.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 13 additions & 2 deletions internal/npm_install/generate_build_file.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ let LEGACY_NODE_MODULES_PACKAGE_NAME = '$node_modules$';
let config: any = {
exports_directories_only: false,
generate_local_modules_build_files: false,
generate_build_files_concurrency_limit: 64,
included_files: [],
links: {},
package_json: 'package.json',
Expand Down Expand Up @@ -688,7 +689,7 @@ async function findPackagesAndPush(pkgs: Dep[], p: string, dependencies: Set<str

const listing = await fs.readdir(p);

await Promise.all(listing.map(async f => {
let promises = listing.map(async f => {
// filter out folders such as `.bin` which can create
// issues on Windows since these are "hidden" by default
if (f.startsWith('.')) return [];
Expand All @@ -702,7 +703,17 @@ async function findPackagesAndPush(pkgs: Dep[], p: string, dependencies: Set<str
await findPackagesAndPush(pkgs, path.posix.join(pf, 'node_modules'), dependencies);
}
}
}));
});

if (config.generate_build_files_concurrency_limit < 1) {
// unlimited concurrency
await Promise.all(promises);
} else {
while (promises.length) {
// run batches of generate_build_files_concurrency_limit at a time
await Promise.all(promises.splice(0, config.generate_build_files_concurrency_limit))
}
}
}

/**
Expand Down
13 changes: 11 additions & 2 deletions internal/npm_install/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ let LEGACY_NODE_MODULES_PACKAGE_NAME = '$node_modules$';
let config = {
exports_directories_only: false,
generate_local_modules_build_files: false,
generate_build_files_concurrency_limit: 64,
included_files: [],
links: {},
package_json: 'package.json',
Expand Down Expand Up @@ -443,7 +444,7 @@ async function findPackagesAndPush(pkgs, p, dependencies) {
return;
}
const listing = await fs_1.promises.readdir(p);
await Promise.all(listing.map(async (f) => {
let promises = listing.map(async (f) => {
if (f.startsWith('.'))
return [];
const pf = path.posix.join(p, f);
Expand All @@ -456,7 +457,15 @@ async function findPackagesAndPush(pkgs, p, dependencies) {
await findPackagesAndPush(pkgs, path.posix.join(pf, 'node_modules'), dependencies);
}
}
}));
});
if (config.generate_build_files_concurrency_limit < 1) {
await Promise.all(promises);
}
else {
while (promises.length) {
await Promise.all(promises.splice(0, config.generate_build_files_concurrency_limit));
}
}
}
async function findScopes() {
const p = nodeModulesFolder();
Expand Down
11 changes: 10 additions & 1 deletion internal/npm_install/npm_install.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,14 @@ Using managed_directories will mean that
default = 3600,
doc = """Maximum duration of the package manager execution in seconds.""",
),
"generate_build_files_concurrency_limit": attr.int(
default = 64,
doc = """Limit the maximum concurrency of npm package processing when generating
BUILD files from the node_modules tree. Unlimited concurrency can lead to too many
open files errors (https://github.com/bazelbuild/rules_nodejs/issues/3507).
Set to 0 or negative for unlimited concurrency.""",
),
})

def _apply_pre_install_patches(repository_ctx):
Expand Down Expand Up @@ -466,6 +474,7 @@ def _create_build_files(repository_ctx, rule_type, node, lock_file, generate_loc
generate_config_json = json.encode(
struct(
exports_directories_only = repository_ctx.attr.exports_directories_only,
generate_build_files_concurrency_limit = repository_ctx.attr.generate_build_files_concurrency_limit,
generate_local_modules_build_files = generate_local_modules_build_files,
included_files = repository_ctx.attr.included_files,
links = validated_links,
Expand All @@ -475,8 +484,8 @@ def _create_build_files(repository_ctx, rule_type, node, lock_file, generate_loc
rule_type = rule_type,
strict_visibility = repository_ctx.attr.strict_visibility,
workspace = repository_ctx.attr.name,
workspace_rerooted_path = _WORKSPACE_REROOTED_PATH,
workspace_rerooted_package_json_dir = paths.normalize(paths.join(_WORKSPACE_REROOTED_PATH, package_json_dir)),
workspace_rerooted_path = _WORKSPACE_REROOTED_PATH,
),
)
repository_ctx.file("generate_config.json", generate_config_json)
Expand Down
6 changes: 6 additions & 0 deletions npm_deps.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -163,24 +163,28 @@ js_library(
},
package_json = "//internal/linker/test/multi_linker/test_a:package.json",
yarn_lock = "//internal/linker/test/multi_linker/test_a:yarn.lock",
generate_build_files_concurrency_limit = 0,
)

yarn_install(
name = "internal_test_multi_linker_test_b_deps",
package_json = "//internal/linker/test/multi_linker/test_b:package.json",
yarn_lock = "//internal/linker/test/multi_linker/test_b:yarn.lock",
generate_build_files_concurrency_limit = 0,
)

yarn_install(
name = "internal_test_multi_linker_test_c_deps",
package_json = "//internal/linker/test/multi_linker/test_c:package.json",
yarn_lock = "//internal/linker/test/multi_linker/test_c:yarn.lock",
generate_build_files_concurrency_limit = 0,
)

yarn_install(
name = "internal_test_multi_linker_test_d_deps",
package_json = "//internal/linker/test/multi_linker/test_d:package.json",
yarn_lock = "//internal/linker/test/multi_linker/test_d:yarn.lock",
generate_build_files_concurrency_limit = 0,
)

yarn_install(
Expand All @@ -189,6 +193,7 @@ js_library(
args = ["--production"],
package_json = "//internal/linker/test/multi_linker/lib_b:package.json",
yarn_lock = "//internal/linker/test/multi_linker/lib_b:yarn.lock",
generate_build_files_concurrency_limit = 0,
)

yarn_install(
Expand All @@ -197,6 +202,7 @@ js_library(
args = ["--production"],
package_json = "//internal/linker/test/multi_linker/lib_c:lib/package.json",
yarn_lock = "//internal/linker/test/multi_linker/lib_c:lib/yarn.lock",
generate_build_files_concurrency_limit = 0,
)

yarn_install(
Expand Down

0 comments on commit 4001716

Please sign in to comment.