Initial support for `resolve.npm`.

Review Request #2723 - Created Aug. 29, 2015 and submitted

Information
John Sirois
pants
jsirois/issues/2073
2073, 2087
2738, 2737, 2736
f83fa4f...
Reviewers
pants-reviews
stuhood, tejal, zundel

This is pants implementing npm install for node projects modeled in
pants. The driving concern is primary control by pants of Node.js
packages via BUILD files, and this necessitates generating package.json
files under .pants.d/ to describe dependencies (and later more) to
npm so it can resolve both local and remote dependencies. A second
fallout is copying pants controlled node module sourcecode up into
.pants.d/ since node has a documented behavior of resolving against
realpath[1], foiling any attempt to use symlinks.

The basic targets needed to support resolve.npm - aka npm install
are added along with basic tests for the functionality.

[1] https://nodejs.org/api/modules.html#modules_addenda_package_manager_tips

contrib/node/BUILD | 3 ++
contrib/node/examples/3rdparty/node/BUILD | 0
contrib/node/src/python/pants/contrib/node/BUILD | 18 +++++++
contrib/node/src/python/pants/contrib/node/register.py | 26 ++++++++++
contrib/node/src/python/pants/contrib/node/targets/BUILD | 39 ++++++++++++++
contrib/node/src/python/pants/contrib/node/targets/init.py | 0
contrib/node/src/python/pants/contrib/node/targets/node_module.py | 41 +++++++++++++++
contrib/node/src/python/pants/contrib/node/targets/node_remote_module.py | 40 +++++++++++++++
contrib/node/src/python/pants/contrib/node/targets/npm_package.py | 33 ++++++++++++
contrib/node/src/python/pants/contrib/node/tasks/BUILD | 35 +++++++++++++
contrib/node/src/python/pants/contrib/node/tasks/init.py | 0
contrib/node/src/python/pants/contrib/node/tasks/node_task.py | 83 ++++++++++++++++++++++++++++++
contrib/node/src/python/pants/contrib/node/tasks/npm_resolve.py | 140 ++++++++++++++++++++++++++++++++++++++++++++++++++
contrib/node/tests/python/pants_test/contrib/node/targets/BUILD | 28 ++++++++++
contrib/node/tests/python/pants_test/contrib/node/targets/init.py | 0
contrib/node/tests/python/pants_test/contrib/node/targets/test_node_remote_module.py | 31 +++++++++++
contrib/node/tests/python/pants_test/contrib/node/targets/test_npm_package.py | 26 ++++++++++
contrib/node/tests/python/pants_test/contrib/node/tasks/BUILD | 36 +++++++++++++
contrib/node/tests/python/pants_test/contrib/node/tasks/init.py | 0
contrib/node/tests/python/pants_test/contrib/node/tasks/test_node_task.py | 79 ++++++++++++++++++++++++++++
contrib/node/tests/python/pants_test/contrib/node/tasks/test_npm_resolve.py | 122 +++++++++++++++++++++++++++++++++++++++++++
pants.ini | 1 +
22 files changed, 781 insertions(+)

I manually confirmed that with the NpmResolve execution of npm dedupe
commented out, the dedupe portion of the
NpmResolveTest.test_resolve_simple_graph test failed like so:

E     AssertionError: Expected to find exactly 1 de-duped `typ` package, but found these:
E       node_modules/typ/package.json
E       node_modules/util/node_modules/typ/package.json

CI went green here:
https://travis-ci.org/pantsbuild/pants/builds/77852199

John Sirois
John Sirois
Eric Ayers
John Sirois
John Sirois
John Sirois
John Sirois
John Sirois
Review request changed

Status: Closed (submitted)

Stu Hood

Thanks a lot John!

  1. Both items below addressed here: https://rbcommons.com/s/twitter/r/2736/

From a 1:1:1 perspective, this seems like a dangerous thing to expose... can we pre-deprecate it?

  1. Definitely.  This appears to be copypasta plain and simple and we seem to have an awful lot of it in target constructors - unused in the pants repo at any rate.  I'm not sure if Twitter, Square or Foursquare use this in internal plugins ... I'll ping folks and straight up kill if not in the other spots its aped, but follow up and kill it here immediately.
  2. We do have it in our ruby_specs() plugin but I'll be happy to update the code.

  3. Eric agreed here and Stu & Patrick agreed in pantsbuild.slack.com#general so the full fix: https://rbcommons.com/s/twitter/r/2738/

Would be good if this was explicitly abstract... was confused for a minute.

  1. I could mix in AbstractClass - although Target already is one - but it would only be for doc since there are no @abstractmethod/@abstractproperty. As such I'd prefer just documenting in the traditional way and/or adding a type-check in __init__ that raises if == NpmPackage.

    Your call.

  2. Documenting would be fine, thanks.

Loading...