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
Include Javascript files in JVM binary #4264
Changes from 46 commits
0c4c696
7ebbf32
dfe9c91
6c1e8e4
abb642d
07198d8
5dafffc
a89e0ac
caefbfc
08d4ffb
9b82e4e
aa2d6dc
26e98fe
710d9ef
624e0d5
e2ffa2b
058b63b
177418a
dcbe36b
1d4211d
9b4cacf
ddb0048
cb22899
52cf7d8
e6d507e
c46e1c1
d336c18
8369bdb
1da9f25
939ce00
b827c0d
ab2e269
7d44280
9e04853
157e85e
542f794
80a86f0
cefc5a2
5cb745a
3e745ac
17d71ae
688bcba
e4b72c2
93828b4
1a510a4
c011d57
0f26643
ff9b491
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
jvm_binary( | ||
source = 'JsResourcesMain.java', | ||
main = 'org.pantsbuild.testproject.jsresources.JsResourcesMain', | ||
dependencies = [ | ||
'3rdparty:guava', | ||
'contrib/node/examples/src/node/web-component-button:web-component-button-processed', | ||
] | ||
) | ||
|
||
jvm_binary(name = 'jsresources-with-dependency-artifacts', | ||
source = 'JsResourcesMain.java', | ||
main = 'org.pantsbuild.testproject.jsresources.JsResourcesMain', | ||
dependencies = [ | ||
'3rdparty:guava', | ||
'contrib/node/examples/src/node/web-component-button:web-component-button-processed-with-dependency-artifacts', | ||
] | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
package org.pantsbuild.testproject.jsresources; | ||
|
||
import com.google.common.base.Charsets; | ||
import com.google.common.io.Resources; | ||
import java.io.IOException; | ||
import java.net.URL; | ||
|
||
public class JsResourcesMain { | ||
private static String RESOURCE_PATH = "web-component-button-processed/Button.js"; | ||
private static String ALT_RESOURCE_PATH = ( | ||
"web-component-button-processed-with-dependency-artifacts/Button.js"); | ||
|
||
public static void main(String[] args) throws IOException { | ||
// Introduce a 3rdparty library so we can test if Manifest's Class-Path entry is | ||
// set properly for both internal and external dependencies. | ||
URL resourceUrl; | ||
try { | ||
resourceUrl = Resources.getResource(RESOURCE_PATH); | ||
} catch (IllegalArgumentException e) { | ||
resourceUrl = Resources.getResource(ALT_RESOURCE_PATH); | ||
} | ||
String content = Resources.toString(resourceUrl, Charsets.UTF_8); | ||
// Ensure resource is loaded properly. | ||
System.out.println(content); | ||
} | ||
|
||
private JsResourcesMain() { | ||
// not called. placates checkstyle | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,13 @@ | ||
# web-component-button target installs all node module dependencies but does not run webpack | ||
# transpile since there is no build_script declared. | ||
node_module( | ||
name='web-component-button', | ||
sources=globs('package.json', 'webpack.config.js', 'src/*', 'test/*'), | ||
dependencies=[ | ||
'contrib/node/examples/3rdparty/node/mocha', | ||
'contrib/node/examples/3rdparty/node/react', | ||
'contrib/node/examples/src/node/web-build-tool', | ||
] | ||
], | ||
) | ||
|
||
node_test( | ||
|
@@ -25,7 +27,39 @@ node_test( | |
tags={'integration'}, | ||
) | ||
|
||
# Contains non-transpiled codes including all subdirectories under node_modules dirctory. | ||
node_bundle( | ||
name='web-component-button-bundle', | ||
node_module=':web-component-button' | ||
) | ||
|
||
# web-component-button-processed target invokes webpack transpiling via build_script. | ||
node_module( | ||
name='web-component-button-processed', | ||
sources=globs('package.json', 'webpack.config.js', 'src/*', 'test/*'), | ||
dependencies=[ | ||
'contrib/node/examples/3rdparty/node/mocha', | ||
'contrib/node/examples/3rdparty/node/react', | ||
'contrib/node/examples/src/node/web-build-tool', | ||
], | ||
build_script='build', | ||
) | ||
|
||
# Contains transpiled codes. | ||
node_bundle( | ||
name='web-component-button-processed-bundle', | ||
node_module=':web-component-button-processed' | ||
) | ||
|
||
# Used to testing dependencies bundling. | ||
node_module( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe a comment indicating why both this and the above target exist? Could this target depend on the other one? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a line of comments. This target could be depending on current BUILD rule =>
change BUILD rule to
will generate =>
Basically, each module's artifacts will be included at JAR root separately, disregarding the hierarchy. |
||
name='web-component-button-processed-with-dependency-artifacts', | ||
sources=globs('package.json', 'webpack.config.js', 'src/*', 'test/*'), | ||
dependencies=[ | ||
'contrib/node/examples/3rdparty/node/mocha', | ||
'contrib/node/examples/3rdparty/node/react', | ||
'contrib/node/examples/src/node/web-build-tool', | ||
'contrib/node/examples/src/node/web-dependency-test', | ||
], | ||
build_script='build', | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
# No build script specified. Dependents will include raw installation results: | ||
# src/*, package.json and node_modules installed by mocha. | ||
node_module( | ||
sources=globs('package.json', 'src/*'), | ||
dependencies=[ | ||
'contrib/node/examples/3rdparty/node/mocha', | ||
] | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
{ | ||
"name": "web-dependency-test" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
console.writeln("Hello World.") |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,18 +6,32 @@ | |
unicode_literals, with_statement) | ||
|
||
from pants.base.payload import Payload | ||
from pants.base.payload_field import PrimitiveField | ||
|
||
from pants.contrib.node.targets.node_package import NodePackage | ||
|
||
|
||
class NodeModule(NodePackage): | ||
"""A Node module.""" | ||
|
||
def __init__(self, sources=None, address=None, payload=None, **kwargs): | ||
def __init__( | ||
self, sources=None, build_script=None, output_dir='dist', | ||
dev_dependency=False, address=None, payload=None, **kwargs): | ||
""" | ||
:param sources: Javascript and other source code files that make up this module; paths are | ||
relative to the BUILD file's directory. | ||
:type sources: `globs`, `rglobs` or a list of strings | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These make a lot more sense now, thanks. One last bit of confusion is the I guess that's not really a thing we can fix unless we're able to encourage people to split their target into "target" and "test target", as most other languages do. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a bit confusion because the usage scenario is a bit confusion. When dev_depedency is set to True, all we really do is not to include this module in class path (or bundlable js product) so that it does not show up in the final bundle. Its dependent node_module targets are treated separately based on their target definition. The reason is sometimes developers writes a build time tool in JS. They might include this build tool as a dependency. An example is |
||
:param build_script: build script name as defined in package.json. All files that are needed | ||
for the build script must be included in sources. The script should output build results | ||
in the directory specified by output_dir. If build_script is not supplied, the node | ||
installation results will be considered as output. The output can be archived or included as | ||
resources for JVM target. | ||
:param output_dir: relative path to assets generated by build script. The path will be | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A longer example would be good here: this all gets rendered to http://www.pantsbuild.org/build_dictionary.html , so it's worthwhile! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fleshed out a little bit more. Not sure if necessary to add some examples codes. Let me know if I should consider to do that. |
||
preserved in the created JAR if the target is used as a JVM target dependency. | ||
:param dev_dependency: boolean value. Default is False. If a node_module is used as parts | ||
of devDependencies and thus should not be included in the final bundle or JVM binaries, set | ||
this value to True. | ||
""" | ||
# TODO(John Sirois): Support devDependencies, etc. The devDependencies case is not | ||
# clear-cut since pants controlled builds would provide devDependencies as needed to perform | ||
|
@@ -26,8 +40,11 @@ def __init__(self, sources=None, address=None, payload=None, **kwargs): | |
# of pre-existing package.json files as node_module targets will require this. | ||
payload = payload or Payload() | ||
payload.add_fields({ | ||
'sources': self.create_sources_field(sources=sources, | ||
sources_rel_path=address.spec_path, | ||
key_arg='sources'), | ||
'sources': self.create_sources_field( | ||
sources=sources, sources_rel_path=address.spec_path, key_arg='sources'), | ||
'build_script': PrimitiveField(build_script), | ||
'output_dir': PrimitiveField(output_dir), | ||
'dev_dependency': PrimitiveField(dev_dependency), | ||
}) | ||
|
||
super(NodeModule, self).__init__(address=address, payload=payload, **kwargs) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,88 @@ | ||
# coding=utf-8 | ||
# Copyright 2015 Pants project contributors (see CONTRIBUTORS.md). | ||
# Licensed under the Apache License, Version 2.0 (see LICENSE). | ||
|
||
from __future__ import (absolute_import, division, generators, nested_scopes, print_function, | ||
unicode_literals, with_statement) | ||
|
||
import os | ||
from collections import defaultdict | ||
|
||
from pants.backend.jvm.tasks.classpath_products import ClasspathProducts | ||
from pants.base.exceptions import TaskError | ||
from pants.base.workunit import WorkUnitLabel | ||
from pants.goal.products import MultipleRootedProducts | ||
from pants.util.contextutil import pushd | ||
from pants.util.dirutil import absolute_symlink | ||
|
||
from pants.contrib.node.tasks.node_paths import NodePaths | ||
from pants.contrib.node.tasks.node_task import NodeTask | ||
|
||
|
||
class NodeBuild(NodeTask): | ||
"""Create an archive bundle of NodeModule targets.""" | ||
|
||
@classmethod | ||
def product_types(cls): | ||
# runtime_classpath is used for JVM target to include node build results as resources. | ||
return ['bundleable_js', 'runtime_classpath'] | ||
|
||
@classmethod | ||
def prepare(cls, options, round_manager): | ||
super(NodeBuild, cls).prepare(options, round_manager) | ||
round_manager.require_data(NodePaths) | ||
|
||
@property | ||
def create_target_dirs(self): | ||
return True | ||
|
||
def _run_build_script(self, target, results_dir, node_installed_path): | ||
target_address = target.address.reference() | ||
# If there is build script defined, run the build script and return build output directory; | ||
# If there is not build script defined, use installation directory as build output. | ||
if target.payload.build_script: | ||
self.context.log.info('Running node build {} for {} at {}\n'.format( | ||
target.payload.build_script, target_address, node_installed_path)) | ||
result, npm_build_command = self.execute_npm( | ||
['run-script', target.payload.build_script], | ||
workunit_name=target_address, | ||
workunit_labels=[WorkUnitLabel.COMPILER]) | ||
# Make sure script run is successful. | ||
if result != 0: | ||
raise TaskError( | ||
'Failed to run build for {}:\n\t{} failed with exit code {}'.format( | ||
target_address, npm_build_command, result)) | ||
|
||
def _get_output_dir(self, target, node_installed_path): | ||
return os.path.normpath(os.path.join( | ||
node_installed_path, | ||
target.payload.output_dir if target.payload.build_script else '')) | ||
|
||
def execute(self): | ||
node_paths = self.context.products.get_data(NodePaths) | ||
runtime_classpath_product = self.context.products.get_data( | ||
'runtime_classpath', init_func=ClasspathProducts.init_func(self.get_options().pants_workdir)) | ||
bundleable_js_product = self.context.products.get_data( | ||
'bundleable_js', init_func=lambda: defaultdict(MultipleRootedProducts)) | ||
|
||
targets = self.context.targets(predicate=self.is_node_module) | ||
with self.invalidated(targets, invalidate_dependents=True) as invalidation_check: | ||
for vt in invalidation_check.all_vts: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When using an invalidated block, I strongly recommend making the block as small as possible, and breaking out the "thing to do only if a target is invalid" and "things to do regardless" bits as methods. The goal is to make it easy to read the block itself, since errors there lead to cache bugs. |
||
target = vt.target | ||
node_installed_path = node_paths.node_path(target) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The invalidated block is still quite deeply nested, and thus difficult to read/review. Please fix the factoring in here (mentioned before) before we merge this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated. Please take another look. |
||
with pushd(node_installed_path): | ||
if not vt.valid: | ||
self._run_build_script(target, vt.results_dir, node_installed_path) | ||
|
||
if not target.payload.dev_dependency: | ||
output_dir = self._get_output_dir(target, node_installed_path) | ||
# Make sure that there is output generated. | ||
if not os.path.exists(output_dir): | ||
raise TaskError( | ||
'Target {} has build script {} specified, but did not generate any output ' | ||
'at {}.\n'.format( | ||
target.address.reference(), target.payload.build_script, output_dir)) | ||
absolute_symlink(output_dir, os.path.join(vt.results_dir, target.address.target_name)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These products will only be recorded if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes. When dev_dependency is specified, we don't add them to these two products, so the generated bundle/binary will not contain dev dependencies. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is that something that the bundle/binary tasks should filter instead? |
||
bundleable_js_product[target].add_abs_paths(output_dir, [output_dir]) | ||
runtime_classpath_product.add_for_target(target, [('default', vt.results_dir)]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the PR description for the new flags, as it will be the commit message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.