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 44 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(name = 'jsresources', | ||
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 |
---|---|---|
|
@@ -5,7 +5,7 @@ node_module( | |
'contrib/node/examples/3rdparty/node/mocha', | ||
'contrib/node/examples/3rdparty/node/react', | ||
'contrib/node/examples/src/node/web-build-tool', | ||
] | ||
], | ||
) | ||
|
||
node_test( | ||
|
@@ -29,3 +29,32 @@ node_bundle( | |
name='web-component-button-bundle', | ||
node_module=':web-component-button' | ||
) | ||
|
||
# The following target invokes webpack build. | ||
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. ...because of the 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 some comments to clarify different scenarios. |
||
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', | ||
) | ||
|
||
node_bundle( | ||
name='web-component-button-processed-bundle', | ||
node_module=':web-component-button-processed' | ||
) | ||
|
||
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,6 @@ | ||
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,28 @@ | |
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', | ||
preserve_artifacts=True, 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. | ||
: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 preserve_artifacts: boolean value. Default is True. If a node_module is used as parts | ||
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. Hm... "artifacts" is a pretty overloaded name. It sounds like what this does is "preserve_dev_dependencies" maybe? Or I'm not understanding. 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. renamed to "dev_dependency" and set default to False. |
||
of devDependencies and thus should not be included in the final artifact, set this value to | ||
False. | ||
""" | ||
# 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 +36,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), | ||
'preserve_artifacts': PrimitiveField(preserve_artifacts), | ||
}) | ||
|
||
super(NodeModule, self).__init__(address=address, payload=payload, **kwargs) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,89 @@ | ||
# 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 __init__(self, *args, **kwargs): | ||
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. empty, can remove. 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. done. |
||
super(NodeBuild, self).__init__(*args, **kwargs) | ||
|
||
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) as invalidation_check: | ||
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. I think that you want to set 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. done. |
||
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 | ||
target_address = target.address.reference() | ||
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 target.payload.build_script: | ||
if not vt.valid: | ||
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]) | ||
if result != 0: | ||
raise TaskError( | ||
'Failed to run build for {}:\n\t{} failed with exit code {}'.format( | ||
target_address, npm_build_command, result)) | ||
|
||
if target.payload.preserve_artifacts: | ||
output_dir = os.path.join(node_installed_path, target.payload.output_dir) | ||
if os.path.exists(output_dir): | ||
bundleable_js_product[target].add_abs_paths(output_dir, [output_dir]) | ||
else: | ||
raise TaskError( | ||
'Target {} has build script {} specified, but did not generate any output ' | ||
'at {}.\n'.format(target_address, npm_build_command, output_dir)) | ||
else: | ||
if target.payload.preserve_artifacts: | ||
bundleable_js_product[target].add_abs_paths(node_installed_path, [node_installed_path]) | ||
output_dir = node_installed_path | ||
|
||
if target.payload.preserve_artifacts: | ||
if not vt.valid: | ||
# Resources included in a JAR file will be under %target_name% | ||
absolute_symlink(output_dir, os.path.join(vt.results_dir, target.address.target_name)) | ||
self.context.log.debug('adding {} for target {} to runtime classpath'.format( | ||
vt.results_dir, target_address)) | ||
runtime_classpath_product.add_for_target(target, [('default', vt.results_dir)]) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,7 +11,6 @@ | |
from pants.fs import archive | ||
from pants.util import dirutil | ||
|
||
from pants.contrib.node.tasks.node_paths import NodePaths | ||
from pants.contrib.node.tasks.node_task import NodeTask | ||
|
||
|
||
|
@@ -25,31 +24,35 @@ def product_types(cls): | |
@classmethod | ||
def prepare(cls, options, round_manager): | ||
super(NodeBundle, cls).prepare(options, round_manager) | ||
round_manager.require_data(NodePaths) | ||
round_manager.require_data('bundleable_js') | ||
|
||
def __init__(self, *args, **kwargs): | ||
super(NodeBundle, self).__init__(*args, **kwargs) | ||
self._outdir = self.get_options().pants_distdir | ||
|
||
def execute(self): | ||
node_paths = self.context.products.get_data(NodePaths) | ||
bundleable_js = self.context.products.get_data('bundleable_js') | ||
bundle_archive_product = self.context.products.get('deployable_archives') | ||
dirutil.safe_mkdir(self._outdir) # Make sure dist dir is present. | ||
|
||
for target in self.context.target_roots: | ||
if self.is_node_bundle(target): | ||
archiver = archive.archiver(target.payload.archive) | ||
# build_dir is a symlink. Since dereference option for tar is set to False, we need to | ||
# dereference manually to archive the linked build dir. | ||
build_dir = os.path.realpath(node_paths.node_path(target.node_module)) | ||
self.context.log.debug('archiving %s' % build_dir) | ||
archivepath = archiver.create( | ||
build_dir, | ||
self._outdir, | ||
target.package_name, | ||
None, | ||
dereference=False | ||
) | ||
bundle_archive_product.add( | ||
target, os.path.dirname(archivepath)).append(os.path.basename(archivepath)) | ||
self.context.log.info('created {}'.format(os.path.relpath(archivepath, get_buildroot()))) | ||
|
||
for _, abs_paths in bundleable_js[target.node_module].abs_paths(): | ||
for abs_path in abs_paths: | ||
# build_dir is a symlink. Since dereference option for tar is set to False, we need to | ||
# dereference manually to archive the linked build dir. | ||
build_dir = os.path.realpath(abs_path) | ||
self.context.log.debug('archiving {}'.format(build_dir)) | ||
archivepath = archiver.create( | ||
build_dir, | ||
self._outdir, | ||
target.package_name, | ||
None, | ||
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. would be good to pass this arg by-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. done. |
||
dereference=False | ||
) | ||
bundle_archive_product.add( | ||
target, os.path.dirname(archivepath)).append(os.path.basename(archivepath)) | ||
self.context.log.info( | ||
'created {}'.format(os.path.relpath(archivepath, get_buildroot()))) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,19 @@ | ||
# Copyright 2015 Pants project contributors (see CONTRIBUTORS.md). | ||
# Licensed under the Apache License, Version 2.0 (see LICENSE). | ||
|
||
python_tests( | ||
name='node_build', | ||
sources=['test_node_build.py'], | ||
dependencies=[ | ||
'contrib/node/src/python/pants/contrib/node/targets:node_module', | ||
'contrib/node/src/python/pants/contrib/node/tasks:node_build', | ||
'src/python/pants/build_graph', | ||
'src/python/pants/util:contextutil', | ||
'src/python/pants/util:dirutil', | ||
'tests/python/pants_test/tasks:task_test_base', | ||
], | ||
) | ||
|
||
python_tests( | ||
name='node_bundle', | ||
sources=['test_node_bundle.py'], | ||
|
@@ -21,7 +34,7 @@ python_tests( | |
'src/python/pants/fs', | ||
'src/python/pants/util:contextutil', | ||
], | ||
timeout=120, | ||
timeout=300, | ||
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 one timeout goes up, it's good to tighten a different one. Maybe you can bring each of 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. done. |
||
tags={'integration'}, | ||
) | ||
|
||
|
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.
This has the same name as the directory, so no need to specify the name (it's the "default" target).
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.
ack. Is it recommended not to name "default" target explicitly? I always names default targets so far.
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.
Yes, it's recommended not to explicitly add their names.