Include Javascript files in JVM binary #4264

Merged
merged 48 commits into from Feb 28, 2017

Conversation

Projects
None yet
2 participants
@UnrememberMe
Contributor

UnrememberMe commented Feb 16, 2017

Problem

node_module is intended to be used as target to manipulate node.js modules. A common practice is to have a JVM based backend server and node based frontend. The FE codes will likely have a transpile step to generate final Javascript codes to be included in the JAR artifact. Currently, there is no support for use node_module as dependencies for jvm targets.
Corresponding to issue #4238

Solution

  • Add a node_build task to support running a single build script.
  • Add three new parameters for node_module target:
    • 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.
    • output_dir: relative path to assets generated by build script. The path will be
      preserved in the created JAR if the target is used as a JVM target dependency.
    • 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.
  • Add build output into runtime_classpath product under %target_name%.
  • Change node_bundle to use node_build output instead of NodePaths output.
    • NodePaths are input product for node_build.

Result

contrib/node/examples/src/java/org/pantsbuild/testproject/jsresources has two examples:

  • jsresources target will generate a JAR file that contains a web-component-button-process/Button.js that is the js file transpiled with webpack.
  • jsresources-with-dependency-artifacts will generate a JAR that contains an additional folder 'web-dependency-test' that is un-transform (include all sources and node_modules folder).

Chris Pesto and others added some commits Sep 11, 2015

Chris Pesto
Roger Jiang
Make sure only support tar archival types since zip does not support …
…undereference symlinks.

Update unit tests to check symlinks.
@stuhood

Looks great! Just a few UX questions.

@@ -0,0 +1,17 @@
+jvm_binary(name = 'jsresources',

This comment has been minimized.

@stuhood

stuhood Feb 16, 2017

Member

This has the same name as the directory, so no need to specify the name (it's the "default" target).

@stuhood

stuhood Feb 16, 2017

Member

This has the same name as the directory, so no need to specify the name (it's the "default" target).

This comment has been minimized.

@UnrememberMe

UnrememberMe Feb 17, 2017

Contributor

ack. Is it recommended not to name "default" target explicitly? I always names default targets so far.

@UnrememberMe

UnrememberMe Feb 17, 2017

Contributor

ack. Is it recommended not to name "default" target explicitly? I always names default targets so far.

This comment has been minimized.

@stuhood

stuhood Feb 20, 2017

Member

Yes, it's recommended not to explicitly add their names.

@stuhood

stuhood Feb 20, 2017

Member

Yes, it's recommended not to explicitly add their names.

+ :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
+ 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

This comment has been minimized.

@stuhood

stuhood Feb 16, 2017

Member

Hm... "artifacts" is a pretty overloaded name. It sounds like what this does is "preserve_dev_dependencies" maybe? Or I'm not understanding.

@stuhood

stuhood Feb 16, 2017

Member

Hm... "artifacts" is a pretty overloaded name. It sounds like what this does is "preserve_dev_dependencies" maybe? Or I'm not understanding.

This comment has been minimized.

@UnrememberMe

UnrememberMe Feb 17, 2017

Contributor

renamed to "dev_dependency" and set default to False.

@UnrememberMe

UnrememberMe Feb 17, 2017

Contributor

renamed to "dev_dependency" and set default to False.

@@ -29,3 +29,32 @@ node_bundle(
name='web-component-button-bundle',
node_module=':web-component-button'
)
+
+# The following target invokes webpack build.

This comment has been minimized.

@stuhood

stuhood Feb 16, 2017

Member

...because of the build_script argument? More comment would be helpful probably.

@stuhood

stuhood Feb 16, 2017

Member

...because of the build_script argument? More comment would be helpful probably.

This comment has been minimized.

@UnrememberMe

UnrememberMe Feb 17, 2017

Contributor

added some comments to clarify different scenarios.

@UnrememberMe

UnrememberMe Feb 17, 2017

Contributor

added some comments to clarify different scenarios.

+ node_module=':web-component-button-processed'
+)
+
+node_module(

This comment has been minimized.

@stuhood

stuhood Feb 16, 2017

Member

Maybe a comment indicating why both this and the above target exist?

Could this target depend on the other one?

@stuhood

stuhood Feb 16, 2017

Member

Maybe a comment indicating why both this and the above target exist?

Could this target depend on the other one?

This comment has been minimized.

@UnrememberMe

UnrememberMe Feb 17, 2017

Contributor

Added a line of comments. This target could be depending on :web-component-button-processed, but it will generate a different output structure.

current BUILD rule =>

JAR / web-component-button-processed-with-dependency-artifacts / Button.js
JAR / web-dependency-test/...

change BUILD rule to

node_module( 
  name = 'module_name',
  dependencies=[':web-component-button-processed', 'web-dependency-test'])

will generate =>

JAR / web-component-button-processed / Button.js
JAR / web-dependency-test/ ...
JAR / module_name / node_modules / web-component-button-processed / %source_files%
JAR / module_name / node_modules / % other_node_modules % / ...

Basically, each module's artifacts will be included at JAR root separately, disregarding the hierarchy.

@UnrememberMe

UnrememberMe Feb 17, 2017

Contributor

Added a line of comments. This target could be depending on :web-component-button-processed, but it will generate a different output structure.

current BUILD rule =>

JAR / web-component-button-processed-with-dependency-artifacts / Button.js
JAR / web-dependency-test/...

change BUILD rule to

node_module( 
  name = 'module_name',
  dependencies=[':web-component-button-processed', 'web-dependency-test'])

will generate =>

JAR / web-component-button-processed / Button.js
JAR / web-dependency-test/ ...
JAR / module_name / node_modules / web-component-button-processed / %source_files%
JAR / module_name / node_modules / % other_node_modules % / ...

Basically, each module's artifacts will be included at JAR root separately, disregarding the hierarchy.

"""
: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
+
+ :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

This comment has been minimized.

@stuhood

stuhood Feb 16, 2017

Member

A longer example would be good here: this all gets rendered to http://www.pantsbuild.org/build_dictionary.html , so it's worthwhile!

@stuhood

stuhood Feb 16, 2017

Member

A longer example would be good here: this all gets rendered to http://www.pantsbuild.org/build_dictionary.html , so it's worthwhile!

This comment has been minimized.

@UnrememberMe

UnrememberMe Feb 17, 2017

Contributor

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.

@UnrememberMe

UnrememberMe Feb 17, 2017

Contributor

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.

+ 'bundleable_js', init_func=lambda: defaultdict(MultipleRootedProducts))
+
+ targets = self.context.targets(predicate=self.is_node_module)
+ with self.invalidated(targets) as invalidation_check:

This comment has been minimized.

@stuhood

stuhood Feb 16, 2017

Member

I think that you want to set invalidate_dependents=True here, which will cause dependencies to invalidate their dependents when they change (yes: it should probably be the default).

@stuhood

stuhood Feb 16, 2017

Member

I think that you want to set invalidate_dependents=True here, which will cause dependencies to invalidate their dependents when they change (yes: it should probably be the default).

This comment has been minimized.

@UnrememberMe

UnrememberMe Feb 17, 2017

Contributor

done.

@UnrememberMe

UnrememberMe Feb 17, 2017

Contributor

done.

+
+ targets = self.context.targets(predicate=self.is_node_module)
+ with self.invalidated(targets) as invalidation_check:
+ for vt in invalidation_check.all_vts:

This comment has been minimized.

@stuhood

stuhood Feb 16, 2017

Member

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.

@stuhood

stuhood Feb 16, 2017

Member

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.

+ build_dir,
+ self._outdir,
+ target.package_name,
+ None,

This comment has been minimized.

@stuhood

stuhood Feb 16, 2017

Member

would be good to pass this arg by-name.

@stuhood

stuhood Feb 16, 2017

Member

would be good to pass this arg by-name.

This comment has been minimized.

@UnrememberMe

UnrememberMe Feb 17, 2017

Contributor

done.

@UnrememberMe

UnrememberMe Feb 17, 2017

Contributor

done.

@@ -21,7 +34,7 @@ python_tests(
'src/python/pants/fs',
'src/python/pants/util:contextutil',
],
- timeout=120,
+ timeout=300,

This comment has been minimized.

@stuhood

stuhood Feb 16, 2017

Member

When one timeout goes up, it's good to tighten a different one. Maybe you can bring each of node_build and node_bundle down to timeout=30?

@stuhood

stuhood Feb 16, 2017

Member

When one timeout goes up, it's good to tighten a different one. Maybe you can bring each of node_build and node_bundle down to timeout=30?

This comment has been minimized.

@UnrememberMe

UnrememberMe Feb 17, 2017

Contributor

done.

@UnrememberMe

UnrememberMe Feb 17, 2017

Contributor

done.

@UnrememberMe

This comment has been minimized.

Show comment
Hide comment
@UnrememberMe

UnrememberMe Feb 17, 2017

Contributor

some of the comments are answered in a previous revision. Please chime in specifically on the flatten of hierarchical dependency structure.

Contributor

UnrememberMe commented Feb 17, 2017

some of the comments are answered in a previous revision. Please chime in specifically on the flatten of hierarchical dependency structure.

@stuhood

The new args make more sense, thanks.

My last blocker is the Task.invalidated block: please split the logic in there out into more methods, and then we should be able to merge.

@@ -0,0 +1,17 @@
+jvm_binary(name = 'jsresources',

This comment has been minimized.

@stuhood

stuhood Feb 20, 2017

Member

Yes, it's recommended not to explicitly add their names.

@stuhood

stuhood Feb 20, 2017

Member

Yes, it's recommended not to explicitly add their names.

@@ -6,5 +6,6 @@ node_module(
'contrib/node/examples/3rdparty/node/null-loader',
'contrib/node/examples/3rdparty/node/style-loader',
'contrib/node/examples/3rdparty/node/webpack',
- ]
+ ],
+ dev_dependency=True,

This comment has been minimized.

@stuhood

stuhood Feb 20, 2017

Member

Please update the PR description for the new flags, as it will be the commit message.

@stuhood

stuhood Feb 20, 2017

Member

Please update the PR description for the new flags, as it will be the commit message.

This comment has been minimized.

@UnrememberMe

UnrememberMe Feb 23, 2017

Contributor

done.

@UnrememberMe

UnrememberMe Feb 23, 2017

Contributor

done.

@@ -25,7 +27,39 @@ node_test(
tags={'integration'},
)
+# Contains non-transpiled codes including all dependencies under node_modules dirctory.

This comment has been minimized.

@stuhood

stuhood Feb 20, 2017

Member

dirctory

@stuhood

stuhood Feb 20, 2017

Member

dirctory

This comment has been minimized.

@UnrememberMe

UnrememberMe Feb 22, 2017

Contributor

done.

@UnrememberMe

UnrememberMe Feb 22, 2017

Contributor

done.

+ build_script='build',
+)
+
+# Contained transpiled codes.

This comment has been minimized.

@stuhood

stuhood Feb 20, 2017

Member

s/Contained/Contains/

@stuhood

stuhood Feb 20, 2017

Member

s/Contained/Contains/

This comment has been minimized.

@UnrememberMe

UnrememberMe Feb 22, 2017

Contributor

done.

@UnrememberMe

UnrememberMe Feb 22, 2017

Contributor

done.

+ def create_target_dirs(self):
+ return True
+
+ def __init__(self, *args, **kwargs):

This comment has been minimized.

@stuhood

stuhood Feb 20, 2017

Member

empty, can remove.

@stuhood

stuhood Feb 20, 2017

Member

empty, can remove.

This comment has been minimized.

@UnrememberMe

UnrememberMe Feb 22, 2017

Contributor

done.

@UnrememberMe

UnrememberMe Feb 22, 2017

Contributor

done.

+ target = vt.target
+ target_address = target.address.reference()
+ node_installed_path = node_paths.node_path(target)
+

This comment has been minimized.

@stuhood

stuhood Feb 20, 2017

Member

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.

@stuhood

stuhood Feb 20, 2017

Member

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.

This comment has been minimized.

@UnrememberMe

UnrememberMe Feb 22, 2017

Contributor

updated. Please take another look.

@UnrememberMe

UnrememberMe Feb 22, 2017

Contributor

updated. Please take another look.

"""
: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
+

This comment has been minimized.

@stuhood

stuhood Feb 20, 2017

Member

These make a lot more sense now, thanks.

One last bit of confusion is the dev_dependencies argument. Is that marking a whole target as a dev_dependency? I guess that would be because the node_tests target doesn't actually own any sources of its own, and thus needs to get them from some other target that has the dev dependency?

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.

@stuhood

stuhood Feb 20, 2017

Member

These make a lot more sense now, thanks.

One last bit of confusion is the dev_dependencies argument. Is that marking a whole target as a dev_dependency? I guess that would be because the node_tests target doesn't actually own any sources of its own, and thus needs to get them from some other target that has the dev dependency?

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.

This comment has been minimized.

@UnrememberMe

UnrememberMe Feb 22, 2017

Contributor

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 contrib/node/examples/src/node/web-component-button. It includes contrib/node/examples/src/node/web-build-tool as a dependency. web-build-tool simply invokes webpack with some customized configuration included. The example is simplified. A more realistic example is a node_module includes a JS tool for asset uploading or template hydration.
I don't think we support this kind of usage in other languages as this is usage pattern is only common for Node developers.

@UnrememberMe

UnrememberMe Feb 22, 2017

Contributor

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 contrib/node/examples/src/node/web-component-button. It includes contrib/node/examples/src/node/web-build-tool as a dependency. web-build-tool simply invokes webpack with some customized configuration included. The example is simplified. A more realistic example is a node_module includes a JS tool for asset uploading or template hydration.
I don't think we support this kind of usage in other languages as this is usage pattern is only common for Node developers.

+ '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))

This comment has been minimized.

@stuhood

stuhood Feb 23, 2017

Member

These products will only be recorded if !dev_dependency... intentional?

@stuhood

stuhood Feb 23, 2017

Member

These products will only be recorded if !dev_dependency... intentional?

This comment has been minimized.

@UnrememberMe

UnrememberMe Feb 23, 2017

Contributor

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.

@UnrememberMe

UnrememberMe Feb 23, 2017

Contributor

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.

This comment has been minimized.

@stuhood

stuhood Feb 23, 2017

Member

Is that something that the bundle/binary tasks should filter instead?

@stuhood

stuhood Feb 23, 2017

Member

Is that something that the bundle/binary tasks should filter instead?

@stuhood

Looks good!

@stuhood stuhood merged commit 8d6b0a0 into pantsbuild:master Feb 28, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

lenucksi added a commit to lenucksi/pants that referenced this pull request Apr 25, 2017

Include Javascript files in JVM binary (#4264)
### Problem
node_module is intended to be used as target to manipulate node.js modules.  A common practice is to have a JVM based backend server and node based frontend. The FE codes will likely have a transpile step to generate final Javascript codes to be included in the JAR artifact.  Currently, there is no support for use node_module as dependencies for jvm targets.  
Corresponding to issue #4238 

### Solution

- Add a node_build task to support running a single build script. 
- Add three new parameters for node_module target:
  - 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.
   - output_dir: relative path to assets generated by build script. The path will be
      preserved in the created JAR if the target is used as a JVM target dependency.
   - 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.
- Add build output into runtime_classpath product under %target_name%.
- Change node_bundle to use node_build output instead of NodePaths output.
  - NodePaths are input product for node_build.

### Result
contrib/node/examples/src/java/org/pantsbuild/testproject/jsresources has two examples:
- jsresources target will generate a JAR file that contains a web-component-button-process/Button.js that is the js file transpiled with webpack.
- jsresources-with-dependency-artifacts will generate a JAR that contains an additional folder 'web-dependency-test' that is un-transform (include all sources and node_modules folder).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment