Reviving node bundle goal #4212

Merged
merged 37 commits into from Feb 3, 2017

Conversation

Projects
None yet
3 participants
@UnrememberMe
Contributor

UnrememberMe commented Jan 27, 2017

Problem

node_module does not support bundle goal and thus do not generate any artifacts. Create an archive after resolving node dependencies (effectively npm install) will enable create a deployable .tar.gz file for node_module targets. Some node modules also depend on symlinks in node_modules/.bin to function correctly.

Solution

Reviving a previously reviewed, but unmerged pull request by @chrispesto #2672 #2674
Merge current master head with the revived PQ.
Add dereference = False support to tar archiver.

Result

We should be able to run pants bundle <node_module_target_name> to create a tar.gz file that contains all dependencies. We will preserve symlinks within the generated tar archive file.

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 good! Minor comments.

+
+ @classmethod
+ def product_types(cls):
+ return ['node_bundles']

This comment has been minimized.

@stuhood

stuhood Jan 28, 2017

Member

You're not currently populating this product, but it would be super cool if you did. That would allow other tasks to depend on the bundles you've created to, say, upload them places.

Also, take a look at https://github.com/pantsbuild/pants/blob/master/src/python/pants/backend/jvm/tasks/bundle_create.py#L66 ... creating the deployable_archives product would be neat as well (less specific: accounts for all bundles that pants is capable of producing: jvm, python, etc).

@stuhood

stuhood Jan 28, 2017

Member

You're not currently populating this product, but it would be super cool if you did. That would allow other tasks to depend on the bundles you've created to, say, upload them places.

Also, take a look at https://github.com/pantsbuild/pants/blob/master/src/python/pants/backend/jvm/tasks/bundle_create.py#L66 ... creating the deployable_archives product would be neat as well (less specific: accounts for all bundles that pants is capable of producing: jvm, python, etc).

This comment has been minimized.

@UnrememberMe

UnrememberMe Jan 31, 2017

Contributor

done.

@UnrememberMe

UnrememberMe Jan 31, 2017

Contributor

done.

+ if self.is_node_module(target):
+ build_dir = node_paths.node_path(target)
+ if os.path.islink(build_dir):
+ # Dereference build_dir if it is a symlink. dereference option for tar is set to False.

This comment has been minimized.

@stuhood

stuhood Jan 28, 2017

Member

It should "always" be a symlink, so would probably just eagerly realpath here.

@stuhood

stuhood Jan 28, 2017

Member

It should "always" be a symlink, so would probably just eagerly realpath here.

This comment has been minimized.

@UnrememberMe

UnrememberMe Jan 30, 2017

Contributor

done.

@UnrememberMe

UnrememberMe Jan 30, 2017

Contributor

done.

+ super(NodeBundle, cls).register_options(register)
+ # Some node modules depend on symlink to function. Thus we can only support archival type
+ # that can preserve symlinks.
+ register('--archive', choices=list(archive.TYPE_NAMES_PRESERVE_SYMLINKS),

This comment has been minimized.

@stuhood

stuhood Jan 28, 2017

Member

In the JVM case, we deprecated Task level options and moved them to the targets themselves, because it helped to make the build more reproducible (ie, you don't need to tell someone to use certain command line flags to build your target).

In this case, doing that would require a node_bundle(dependencies=[':x', ':y'], archive='tar') target or something, but that's probably a good idea anyway.

@stuhood

stuhood Jan 28, 2017

Member

In the JVM case, we deprecated Task level options and moved them to the targets themselves, because it helped to make the build more reproducible (ie, you don't need to tell someone to use certain command line flags to build your target).

In this case, doing that would require a node_bundle(dependencies=[':x', ':y'], archive='tar') target or something, but that's probably a good idea anyway.

This comment has been minimized.

@UnrememberMe

UnrememberMe Jan 31, 2017

Contributor

I gave it some thoughts and an additional target node_bundle does not make much sense currently. The suggested node_bundle will always have only one dependency which is a node_module since archive two node modules as sibling directories is not a valid/common scenario. Maybe we just put the archive options in the node_module target itself? Suggestions are welcome.

@UnrememberMe

UnrememberMe Jan 31, 2017

Contributor

I gave it some thoughts and an additional target node_bundle does not make much sense currently. The suggested node_bundle will always have only one dependency which is a node_module since archive two node modules as sibling directories is not a valid/common scenario. Maybe we just put the archive options in the node_module target itself? Suggestions are welcome.

This comment has been minimized.

@stuhood

stuhood Feb 1, 2017

Member

It seems unlikely that every node_module that is defined would also require a node_bundle target, so I'm not sure how high the overhead would be in practice.

Does npm differentiate between binary and library package.jsons? If not, then I think that adding a node_bundle target would make sense, as it would allow you to explicitly indicate which target in your build graph is a root.

But if there is uncertainty here, would bias toward not adding the target argument for now.

@stuhood

stuhood Feb 1, 2017

Member

It seems unlikely that every node_module that is defined would also require a node_bundle target, so I'm not sure how high the overhead would be in practice.

Does npm differentiate between binary and library package.jsons? If not, then I think that adding a node_bundle target would make sense, as it would allow you to explicitly indicate which target in your build graph is a root.

But if there is uncertainty here, would bias toward not adding the target argument for now.

This comment has been minimized.

@UnrememberMe

UnrememberMe Feb 3, 2017

Contributor

add a node_bundle goal to encapsulate the bundle goal.

@UnrememberMe

UnrememberMe Feb 3, 2017

Contributor

add a node_bundle goal to encapsulate the bundle goal.

@wisechengyi

Thanks @UnrememberMe , a few nits as commented.

+ with self._extract_archive('dist/preinstalled-project.tar.gz') as temp_dir:
+ self.assertEquals(
+ set(os.listdir(temp_dir)),
+ set(['src', 'test', 'node_modules', 'package.json'])

This comment has been minimized.

@wisechengyi

wisechengyi Jan 30, 2017

Contributor

What do you think of listing the dir with a variable instead of hardcoding it?

project_dir = 'contrib/node/examples/src/node/preinstalled-project'
...
with .... as temp_dir:
  assertEquals(
  set(os.listdir(temp_dir)),
  set(os.listdir(project_dir))
}
@wisechengyi

wisechengyi Jan 30, 2017

Contributor

What do you think of listing the dir with a variable instead of hardcoding it?

project_dir = 'contrib/node/examples/src/node/preinstalled-project'
...
with .... as temp_dir:
  assertEquals(
  set(os.listdir(temp_dir)),
  set(os.listdir(project_dir))
}

This comment has been minimized.

@UnrememberMe

UnrememberMe Jan 30, 2017

Contributor

done.

@UnrememberMe

UnrememberMe Jan 30, 2017

Contributor

done.

+ self.assertEquals(
+ set(os.listdir(os.path.join(temp_dir, 'preinstalled-project'))),
+ set(['src', 'test', 'node_modules', 'package.json'])
+ )

This comment has been minimized.

@wisechengyi

wisechengyi Jan 30, 2017

Contributor

same as above

@wisechengyi

wisechengyi Jan 30, 2017

Contributor

same as above

This comment has been minimized.

@UnrememberMe

UnrememberMe Jan 30, 2017

Contributor

done.

@UnrememberMe

UnrememberMe Jan 30, 2017

Contributor

done.

+
+ def test_tar_dereference(self):
+
+ def round_trip(archive_format, dereference):

This comment has been minimized.

@wisechengyi

wisechengyi Jan 30, 2017

Contributor

nit: round_trip seems confusing, maybe check_archive_with_flags?

@wisechengyi

wisechengyi Jan 30, 2017

Contributor

nit: round_trip seems confusing, maybe check_archive_with_flags?

This comment has been minimized.

@UnrememberMe

UnrememberMe Jan 30, 2017

Contributor

done.

@UnrememberMe

UnrememberMe Jan 30, 2017

Contributor

done.

This comment has been minimized.

@wisechengyi

wisechengyi Jan 30, 2017

Contributor

btw the changes have not been pushed to github yet.

@wisechengyi

wisechengyi Jan 30, 2017

Contributor

btw the changes have not been pushed to github yet.

@stuhood

stuhood approved these changes Feb 1, 2017

Thanks Roger.

+ super(NodeBundle, cls).register_options(register)
+ # Some node modules depend on symlink to function. Thus we can only support archival type
+ # that can preserve symlinks.
+ register('--archive', choices=list(archive.TYPE_NAMES_PRESERVE_SYMLINKS),

This comment has been minimized.

@stuhood

stuhood Feb 1, 2017

Member

It seems unlikely that every node_module that is defined would also require a node_bundle target, so I'm not sure how high the overhead would be in practice.

Does npm differentiate between binary and library package.jsons? If not, then I think that adding a node_bundle target would make sense, as it would allow you to explicitly indicate which target in your build graph is a root.

But if there is uncertainty here, would bias toward not adding the target argument for now.

@stuhood

stuhood Feb 1, 2017

Member

It seems unlikely that every node_module that is defined would also require a node_bundle target, so I'm not sure how high the overhead would be in practice.

Does npm differentiate between binary and library package.jsons? If not, then I think that adding a node_bundle target would make sense, as it would allow you to explicitly indicate which target in your build graph is a root.

But if there is uncertainty here, would bias toward not adding the target argument for now.

@@ -1,6 +1,56 @@
# Copyright 2015 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).
+target(

This comment has been minimized.

@stuhood

stuhood Feb 1, 2017

Member

Use target tags, not aliases: can delete these three.

Instead, mark integration tests tags={'integration'} (see example further down in this file). Then you will be able to run tests with:

# Only integration
./pants --tag='integration' test contrib/node::
# Only unit
./pants --tag='-integration' test contrib/node::
# Everything
./pants test contrib/node::
@stuhood

stuhood Feb 1, 2017

Member

Use target tags, not aliases: can delete these three.

Instead, mark integration tests tags={'integration'} (see example further down in this file). Then you will be able to run tests with:

# Only integration
./pants --tag='integration' test contrib/node::
# Only unit
./pants --tag='-integration' test contrib/node::
# Everything
./pants test contrib/node::

This comment has been minimized.

@UnrememberMe

UnrememberMe Feb 3, 2017

Contributor

done.

@UnrememberMe

UnrememberMe Feb 3, 2017

Contributor

done.

@@ -37,6 +38,7 @@ def register_goals():
task(name='node', action=NodeResolve).install('resolve')
task(name='node', action=NodeRun).install('run')
task(name='node', action=NodeTestTask).install('test')
+ task(name='node', action=NodeBundle).install('bundle')

This comment has been minimized.

@stuhood

stuhood Feb 3, 2017

Member

To register your new target type, you'd need to include it in the dict of targets={..} above.

The fact that it isn't actually registered and your integration tests are passing would seem to indicate that usage of the target is optional. Is that intentional?

@stuhood

stuhood Feb 3, 2017

Member

To register your new target type, you'd need to include it in the dict of targets={..} above.

The fact that it isn't actually registered and your integration tests are passing would seem to indicate that usage of the target is optional. Is that intentional?

This comment has been minimized.

@UnrememberMe

UnrememberMe Feb 3, 2017

Contributor

I missed a commit. Sorry about that.

@UnrememberMe

UnrememberMe Feb 3, 2017

Contributor

I missed a commit. Sorry about that.

@stuhood

stuhood approved these changes Feb 3, 2017

Thanks Roger.

@@ -1,4 +1,5 @@
node_module(
+ name='web-component-button',

This comment has been minimized.

@stuhood

stuhood Feb 3, 2017

Member

Just FYI: by not explicitly declaring a name, this was the default target for this directory. That doesn't mean it doesn't have a name: just that its name is equal to the directory name.

As an example, spelling it out explicitly is fine though.

@stuhood

stuhood Feb 3, 2017

Member

Just FYI: by not explicitly declaring a name, this was the default target for this directory. That doesn't mean it doesn't have a name: just that its name is equal to the directory name.

As an example, spelling it out explicitly is fine though.

@stuhood stuhood merged commit e3d4f1b into pantsbuild:master Feb 3, 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

Add a node bundle goal (#4212)
### Problem 
node_module does not support bundle goal and thus do not generate any artifacts.  Create an archive after resolving node dependencies (effectively `npm install`) will enable create a deployable .tar.gz file for node_module targets.  Some node modules also depend on symlinks in node_modules/.bin to function correctly.

### Solution
Reviving a previously reviewed, but unmerged pull request by @chrispesto  #2672 #2674
Merge current master head with the revived PQ.
Add dereference = False support to tar archiver.

### Result
We should be able to run `./pants bundle <node_bundle_target_name>` to create a tar.gz file that contains all dependencies. We will preserve symlinks within the generated tar archive file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment