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

Add NodeJSSourcesInfo provider to allow for better layering in docker rules. #283

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions defs.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ load(
"//internal/node:node.bzl",
_nodejs_binary = "nodejs_binary_macro",
_nodejs_test = "nodejs_test_macro",
_sources_info = "NodeJSSourcesInfo",
)
load("//internal/node:node_repositories.bzl", _node_repositories = "node_repositories")
load("//internal/npm_install:npm_install.bzl", _npm_install = "npm_install", _yarn_install = "yarn_install")
Expand All @@ -41,6 +42,7 @@ npm_install = _npm_install
yarn_install = _yarn_install
rollup_bundle = _rollup_bundle
npm_package = _npm_package
NodeJSSourcesInfo = _sources_info
history_server = _history_server
http_server = _http_server
# ANY RULES ADDED HERE SHOULD BE DOCUMENTED, run yarn skydoc to verify
Expand Down
74 changes: 2 additions & 72 deletions internal/node/generate_build_file.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,7 @@ function main() {
module.exports = { main };

function generateBuildFile() {
// *.pyc files are generated during node-gyp compilation and include
// absolute paths, making them non-hermetic.
// See https://github.com/bazelbuild/rules_nodejs/issues/347
const excludedFiles = ['.md', '.html', '.pyc'];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where did this code go? Don't we still need to exclude these non-hermetic inputs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just bringing the changes that you did here: 3380dd7#diff-67b0f7c2b38c3bbb6dd40c85eb923174L242 up-to-date with the new code. Basically node_runfiles includes a whole load of extra stuff like yarn and everything else that node ships with but imo that seems unnecessary, as if someone e.g. needs yarn they should just add it as a dependency explicitly rather than everyone having to pay the cost for this. And our own internal app is still working fine after this change and all tests here are still passing so it seems to me these files are not needed for a nodejs_binary and they add quite a bit of weight to the resulting docker image.

const nodejsSrcFiles = filterFilesForFilegroup(listFiles(NODE_DIR), [], excludedFiles)
const yarnSrcFiles = filterFilesForFilegroup(listFiles(YARN_DIR), [], excludedFiles)
const nodeRunfiles = fs.existsSync(`${NODE_DIR}/bin/node`) ? `"${NODE_DIR}/bin/node"` : '';

const binaryExt = IS_WINDOWS ? '.cmd' : '';
const buildFile = `# Generated by node_repositories.bzl
Expand All @@ -82,74 +77,9 @@ alias(name = "yarn", actual = "${YARN_ACTUAL}")
filegroup(
name = "node_runfiles",
srcs = [
${nodejsSrcFiles.map(f => `"${NODE_DIR}/${f}",`).join('\n ')}
${yarnSrcFiles.map(f => `"${YARN_DIR}/${f}",`).join('\n ')}
${nodeRunfiles}
],
)
`
writeFileSync('BUILD.bazel', buildFile);
}

/**
* Returns an array of all the files under a directory as relative
* paths to the directory.
*/
function listFiles(rootDir, subDir = '') {
const dir = path.posix.join(rootDir, subDir);
if (!fs.existsSync(dir) || !fs.statSync(dir).isDirectory()) {
return [];
}
return fs
.readdirSync(dir)
.reduce((files, file) => {
const fullPath = path.posix.join(dir, file);
const relPath = path.posix.join(subDir, file);
let stat;
try {
stat = fs.statSync(fullPath);
} catch (e) {
throw e;
}
return stat.isDirectory() ? files.concat(listFiles(rootDir, relPath)) :
files.concat(relPath);
}, []);
}

/**
* A filter function for a bazel filegroup.
* @param files array of files to filter
* @param allowedExts list of white listed extensions; if empty, no filter is done on extensions;
* '' empty string denotes to allow files with no extensions, other extensions
* are listed with '.ext' notation such as '.d.ts'.
*/
function filterFilesForFilegroup(files, allowedExts = [], excludedExts = []) {
// Files with spaces (\x20) or unicode characters (<\x20 && >\x7E) are not allowed in
// Bazel runfiles. See https://github.com/bazelbuild/bazel/issues/4327
files = files.filter(f => !f.match(/[^\x21-\x7E]/));
if (allowedExts.length) {
const allowNoExts = allowedExts.includes('');
files = files.filter(f => {
// include files with no extensions if noExt is true
if (allowNoExts && !path.extname(f)) return true;
// filter files in allowedExts
for (const e of allowedExts) {
if (e && f.endsWith(e)) {
return true;
}
}
return false;
})
}
if (excludedExts.length) {
files = files.filter(f => {
// filter out files in excludedExts
for (const e of excludedExts) {
if (e && f.endsWith(e)) {
return false;
}
}
return true;
})
}
return files;
}
58 changes: 36 additions & 22 deletions internal/node/node.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,16 @@ def _trim_package_node_modules(package_name):
segments += [n]
return "/".join(segments)

NodeJSSourcesInfo = provider(
doc = """All application files needed to run the NodeJS binary, split by data files and npm package files.
This can be used for example when creating a docker image to be able to put npm packages and data files on
to separate layers for better caching.""",
fields = {
"data": "A list of File objects",
"node_modules": "A list of node modules file objects, i.e. third party dependencies",
},
)

def _write_loader_script(ctx):
# Generates the JavaScript snippet of module roots mappings, with each entry
# in the form:
Expand Down Expand Up @@ -106,14 +116,20 @@ def _nodejs_binary_impl(ctx):
# Using a depset will allow us to avoid flattening files and sources
# inside this loop. This should reduce the performances hits,
# since we don't need to call .to_list()
sources = depset()
non_module_sources = depset()
node_module_sources = depset()

for d in ctx.attr.data:
if hasattr(d, "node_sources"):
sources = depset(transitive = [sources, d.node_sources])
if NodeModuleInfo not in d:
non_module_sources = depset(transitive = [non_module_sources, d.node_sources])
else:
node_module_sources = depset(transitive = [node_module_sources, d.node_sources])
if hasattr(d, "files"):
sources = depset(transitive = [sources, d.files])

if NodeModuleInfo not in d:
non_module_sources = depset(transitive = [non_module_sources, d.files])
else:
node_module_sources = depset(transitive = [node_module_sources, d.files])
_write_loader_script(ctx)

# Avoid writing non-normalized paths (workspace/../other_workspace/path)
Expand Down Expand Up @@ -151,25 +167,23 @@ def _nodejs_binary_impl(ctx):
is_executable = True,
)

runfiles = depset([node, ctx.outputs.loader, ctx.file._repository_args] + node_modules + ctx.files._node_runfiles, transitive = [sources])

return [DefaultInfo(
executable = ctx.outputs.script,
runfiles = ctx.runfiles(
transitive_files = runfiles,
files = [
node,
ctx.outputs.loader,
] + ctx.files._source_map_support_files + node_modules +

# We need this call to the list of Files.
# Calling the .to_list() method may have some perfs hits,
# so we should be running this method only once per rule.
# see: https://docs.bazel.build/versions/master/skylark/depsets.html#performance
sources.to_list(),
collect_data = True,
runfiles = depset(
[node, ctx.outputs.loader, ctx.file._repository_args] + ctx.files._source_map_support_files + node_modules + ctx.files._node_runfiles,
transitive = [non_module_sources, node_module_sources],
)

return [
DefaultInfo(
executable = ctx.outputs.script,
runfiles = ctx.runfiles(
transitive_files = runfiles,
),
),
NodeJSSourcesInfo(
data = depset([ctx.outputs.loader, ctx.outputs.script], transitive = [non_module_sources]),
node_modules = node_module_sources,
),
)]
]

_NODEJS_EXECUTABLE_ATTRS = {
"bootstrap": attr.string_list(
Expand Down