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.

Sign up for GitHub

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
wants to merge 10 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a101eff
Add SorucesInfo provider to allow for better layering in docker rules.
Aug 14, 2018
fec940e
Export SourcesInfo.
Aug 14, 2018
d22bb3c
Update to exclude fin-grained deps in SourcesInfo provider.
Sep 22, 2018
3380dd7
Cleanup
alexeagle Oct 23, 2018
6b35de4
Manually merge latest master.
Feb 14, 2019
ae7ab76
Also provide node_module sources.
Feb 14, 2019
984a031
Remove obsolete comment.
Feb 14, 2019
a87eadb
Lint and syntax fixes.
Feb 14, 2019
a5121bc
Depset fix.
Feb 14, 2019
0f6201a
Fix for node_runfiles.
Feb 14, 2019
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
4 changes: 3 additions & 1 deletion defs.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ Users should not load files under "/internal"
load("//internal/common:check_bazel_version.bzl", _check_bazel_version = "check_bazel_version")
load("//internal/node:node.bzl",
_nodejs_binary = "nodejs_binary_macro",
_nodejs_test = "nodejs_test_macro")
_nodejs_test = "nodejs_test_macro",
_sources_info = "NodeJSSourcesInfo")
load("//internal/node:node_repositories.bzl",_node_repositories = "node_repositories")
load("//internal/jasmine_node_test:jasmine_node_test.bzl", _jasmine_node_test = "jasmine_node_test")
load("//internal/npm_install:npm_install.bzl", _npm_install = "npm_install", _yarn_install = "yarn_install")
Expand All @@ -39,6 +40,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
27 changes: 20 additions & 7 deletions internal/node/node.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,11 @@ def _trim_package_node_modules(package_name):
segments += [n]
return "/".join(segments)

NodeJSSourcesInfo = provider(
doc = """Minimal files needed to run a NodeJS program, without npm packages""",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I started a doc string here but it needs to answer more questions

  • how is this intended to be used?
  • where do the npm packages come from in the consuming end?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point about "where do the npm packages come from" - in the new way of having the fine-grained node modules we indeed need to provide these here in the provider also. I updated that.

Not sure what exactly you mean with " * how is this intended to be used", but I did add a sentence for it.

fields = {"data": "A list of File objects"},
)

def _write_loader_script(ctx):
# Generates the JavaScript snippet of module roots mappings, with each entry
# in the form:
Expand Down Expand Up @@ -99,11 +104,16 @@ def _nodejs_binary_impl(ctx):
node = ctx.file.node
node_modules = ctx.files.node_modules
sources = []
non_module_sources = []
for d in ctx.attr.data:
if hasattr(d, "node_sources"):
sources += d.node_sources.to_list()
if NodeModuleInfo not in d:
non_module_sources += d.node_sources.to_list()
if hasattr(d, "files"):
sources += d.files.to_list()
if NodeModuleInfo not in d:
non_module_sources += d.files.to_list()

_write_loader_script(ctx)

Expand Down Expand Up @@ -143,14 +153,17 @@ def _nodejs_binary_impl(ctx):

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

return [DefaultInfo(
executable = ctx.outputs.script,
runfiles = ctx.runfiles(
transitive_files = runfiles,
files = [node, ctx.outputs.loader] + node_modules + sources,
collect_data = True,
return [
DefaultInfo(
executable = ctx.outputs.script,
runfiles = ctx.runfiles(
transitive_files = runfiles,
),
),
)]
NodeJSSourcesInfo(
data = non_module_sources + [ctx.outputs.loader, ctx.outputs.script]
)
]

_NODEJS_EXECUTABLE_ATTRS = {
"entry_point": attr.string(
Expand Down
17 changes: 3 additions & 14 deletions internal/node/node_repositories.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,21 +239,10 @@ alias(name = "npm", actual = "{npm}")
alias(name = "yarn", actual = "{yarn}")
filegroup(
name = "node_runfiles",
Copy link
Collaborator

Choose a reason for hiding this comment

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

in your change, you introduced a new minimal_node_runfiles but then I believe the existing one became unreferenced.

It's possible that users downstream referenced @npm//:node_runfiles and rely on the old content, we should figure that out by vetting this PR on downstream repos.

srcs = glob(
[
srcs = [
"bin/node.js",
"{node_dir}/**",
"{yarn_dir}/**",
],
exclude = [
"**/*.md",
"**/*.html",
# These files are generated during node-gyp compilation and include
# absolute paths, making them non-hermetic.
# See https://github.com/bazelbuild/rules_nodejs/issues/347
"**/*.pyc",
],
),
"{node_dir}/bin/node",
],
)
""".format(
node = node_entry,
Expand Down