Add a node-install goal to Pants for installing node_modules by nsaechao · Pull Request #6367 · pantsbuild/pants

nsaechao · 2018-08-18T00:00:54Z

Problem

Adding node-install is the first step to support javascript source-level dependencies. See #6332.
node-install is a gap-fill for setting up the development environment similar to the environment that is produced by the working dir in .pants.d. This goal should resolve all third party dependencies in addition to the source deps and then be consumable by NPM/Yarn as if the developer has ran "npm install".

Solution

Add a bare-bones node-install goal that will use the target-specific package manager to run the install task in the target directory.
Currently, only node_modules with the 'yarn' package manager will be supported for the node-install goal.

Result

A new node-install goal is added.
The node-install goal will be able to take node_module targets as input.
When executing the node-install goal, walk down the dependency graph and install each node_module target into the target root.

The NodeResolve task now exposes new optional product NodePathsLocal product. The existing NodePaths product remains the same. The NodePathsLocal provides similar mapping of targets and PATHS but installs in the same directory as the target definition rather than in the hidden pants working environment. Despite being seemingly similar, both products are exclusive and don't share any work done. See review discussion for more details.

Example

src/node/target/package.json
"dependencies": {
  "example": "0.0.0"
}

./pants node-install src/node/target:target

output:
src/node/target/node_modules

nsaechao · 2018-08-18T00:05:53Z

@stuhood, @baroquebobcat: This is an initial WIP ticket for adding the node-install task. I would greatly appreciate early feedback on the design if you have the time...

The main focus so far is how the new goal is added and how the product types are specified. Things I still have not added are testing and checks against non-source node_modules.

…used shrinkwap for yarn node modules

…than resolve

…talls

stuhood

Thanks Ny.

I think both the class docs and the PR description need more info on what node-install is, how it is used, etc. A link to the issue you opened would be good too.

stuhood · 2018-08-20T23:05:59Z

+
+
+class NodePathsLocal(NodePaths):
+  """Maps Node package targets to their local NODE_PATH chroot."""


The difference between NodePathsLocal and NodePaths needs more docs/comments... possibly in both the parent class and the subclass. It would be good to discuss why they are not the same class.

And in general, it's not a great idea to subclass concrete classes, as it makes the interactions between the parent and child harder to reason about.

stuhood · 2018-08-20T23:08:17Z

+from pants.contrib.node.tasks.node_resolve import NodeResolve
+
+
+class NodeResolveLocal(NodeResolve):


Ditto subclassing concrete classes: rather than subclassing a concrete class, you should almost always prefer to extract an abstract base class with two (in this case) subclasses. That helps you to define the API, and clarifies why the classes actually need to be different (basically: it's exactly the abstract members).

…nction more clear

nsaechao · 2018-08-24T18:22:20Z

I am having some difficulties in isolating the task execution order. The current result is that the "resolve" task execution is executed before the "node-resolve-local" task. This causes the node-install goal to install the node_packages twice. Once in the .pants.d dir and another in the source target dir. The underlying issue is that node-install consumes products from resolve.node (like node/npm/yarn distributions).

stuhood · 2018-08-27T17:50:17Z

I am having some difficulties in isolating the task execution order. The current result is that the "resolve" task execution is executed before the "node-resolve-local" task. This causes the node-install goal to install the node_packages twice. Once in the .pants.d dir and another in the source target dir. The underlying issue is that node-install consumes products from resolve.node (like node/npm/yarn distributions).

The design doc and PR description don't really explain why node-resolve-local is a separate task from node-resolve. I know we had discussed this potentially being two optional products exposed by the same task... is that worth exploring? Happy to talk offline about this, or to discuss it on #6332.

…dePathsLocal

… thing and that it is an ADVANCED option

stuhood

Thanks!

stuhood · 2018-08-30T21:47:30Z

+              resolver_for_target_type = self._resolver_for_target(target).global_instance()
+              resolver_for_target_type.resolve_target(self, target, vt.results_dir, node_paths)
+            node_paths.resolved(target, vt.results_dir)
+    if self.context.products.is_required_data(NodePathsLocal):


Are there any cases in which this should represent a symlink into .pants.d rather than always double installing things?

By exposing two products and treating them both as optional, we effectively removed the "double installation" problem.

Except when they've both been requested?

It's not critical, it's just unfortunate that ./pants node-install can never (re)use the work done by the private copy.

stuhood · 2018-08-30T21:48:56Z

+                                   topological_order=True):
+    """Returns an invalidation_check with all targets invalidated.
+    """
+    # Generate a cache_key that never matches to ensure work is always done.


If you're going to do this, then you don't actually need to use an invalidation check.

This usage of the invalidation check is incorrect anyway though I think... it's meant to be used as context manager, such that when the context exits, targets are marked valid.

That's true, I admit this is a hack job to re-use topological sort for targets. I can copy-paste the topological sort and it should make more sense.

stuhood · 2018-08-30T21:50:45Z

+                                                  resolve_locally=True,
+                                                  force=True,
+                                                  install_optional=True,
+                                                  frozen_lockfile=False)


frozen_lockfile=False is really fishy... is the default behaviour of yarn install to ignore the lockfile...?

--frozen-lockfile=False is the default behavior of yarn install. We turn on frozen-lockfile for .pants.d installs because we want them to atomic. Outside of .pants.d, it is meaningful to produce a new lockfile such as the case of adding a new dependency.

Ok. It makes some sense to align with Yarn's default. But this is still a pretty wacky default, IMO. It might be worth diverging from the default to be a bit more sane, but up to you.

nsaechao · 2018-08-31T22:09:09Z

Thanks @stuhood for the reviews, I've expanded the documentations and got rid of the awkward invalidation check and replaced it with topological sort which is what I only intended to use.

benjyw · 2018-09-04T20:38:18Z

This looks fine as far as my limited understanding permits, but to clarify, what is the difference between what the new task does and what the existing NodeResolve does?

stuhood

Thanks, no blockers that I see. One last round of feedback, and then will merge on green CI, unless Benjy has followup questions/feedback.

stuhood · 2018-09-04T21:17:40Z

@@ -4,9 +4,8 @@ Hello!

 You can also run one of the following command to greet with name:


s/run one of the/run the/

stuhood · 2018-09-04T21:20:28Z

      '--install-optional', type=bool, default=False, fingerprint=True,
      help='If enabled, install optional dependencies.')
+    register(
+      '--production-only', type=bool, default=False, fingerprint=True,


There is an --install-optional flag, which implies that maybe this should be --install-production-only? Alternatively, could rename that flag to --optional and mark the old one deprecated? Example: https://github.com/pantsbuild/pants/blob/master/testprojects/src/python/plugins/dummy_options/tasks/dummy_options.py#L20-L21

stuhood · 2018-09-04T21:23:43Z

+
+
+class NodeInstall(NodeTask):
+  """Installs a node_module target into the source directory


The first line of a task doc is the one that shows up in ./pants help $task, so replacing the word "source" with something more meaningful would be good. Example:

$ ./pants help list list options: Lists all targets matching the target specs. ..

stuhood · 2018-09-04T21:24:45Z

+    round_manager.require_data(NodePathsLocal)
+
+  @classmethod
+  def supports_passthru_args(cls):


This is still relevant.

stuhood · 2018-09-04T21:25:47Z

+
+
+class NodePathsLocal(NodePathsBase):
+  """Maps Node package targets to their local NODE_PATH chroot.


The "local" bit is I think still not clear, as evidenced by Benjy's question.

Maybe you could have some/all of these docstrings refer to the docstring of NodeResolve, which you expanded?

stuhood · 2018-09-04T21:25:59Z

+class NodePathsLocal(NodePathsBase):
+  """Maps Node package targets to their local NODE_PATH chroot.
+
+  NodePathsLocal is exposed as a product type for node-resolve-local.


Still relevant.

stuhood · 2018-09-04T21:27:13Z

+  pants working directory.
+
+  NodePathsLocal is similar to NodePaths with the difference that the resolved location
+  is local to the target source root.


"local to" doesn't have much meaning... could just say "next to", or refer explicitly to "the directory that the target is defined in".

stuhood · 2018-09-04T21:30:27Z

+              resolver_for_target_type = self._resolver_for_target(target).global_instance()
+              resolver_for_target_type.resolve_target(self, target, vt.results_dir, node_paths)
+            node_paths.resolved(target, vt.results_dir)
+    if self.context.products.is_required_data(NodePathsLocal):


Except when they've both been requested?

It's not critical, it's just unfortunate that ./pants node-install can never (re)use the work done by the private copy.

stuhood · 2018-09-04T21:32:29Z

+                                                  resolve_locally=True,
+                                                  force=True,
+                                                  install_optional=True,
+                                                  frozen_lockfile=False)


Ok. It makes some sense to align with Yarn's default. But this is still a pretty wacky default, IMO. It might be worth diverging from the default to be a bit more sane, but up to you.

…l-production and improve docstrings

nsaechao · 2018-09-06T01:05:50Z

@benjyw

This looks fine as far as my limited understanding permits, but to clarify, what is the difference between what the new task does and what the existing NodeResolve does?

Previously NodeResolve only exposed a single product NodePaths that contains a mapping of targets and PATHS to chroots that has all third party dependencies "installed" into the virtual pants working directory. With this change, the NodeResolve task exposes both the NodePaths and NodePathsLocal products as optional. The existing NodePaths product remains the same. The NodePathsLocal provides similar mapping of targets and PATHS but installs in the same directory as the target definition.

The stick is that, both products are exclusive and don't share any work done.

nsaechao · 2018-09-06T01:08:24Z

@stuhood: After some more thought and experimentation, setting --force=True by default doesn't make sense. My main concern would be that Yarn copies file dependencies rather than installs so it would not pick up local changes. This concern was unfounded as I will be wiring the install, bypassing yarn's interpretation of file:/ deps. I've removed the --force flag from node-install.

benjyw

OK, all seems reasonable as far as I can tell (which, admittedly, isn't very far...)

stuhood

Thanks! Will merge on green CI.

stuhood · 2018-09-07T18:14:44Z

Will merge this as soon as #6469 goes in. Thanks!

Ny Saechao added 2 commits August 17, 2018 10:20

Add base skeleton code for node-install goal

c98be5a

First iteration for ./pants install-node

27fe727

Ny Saechao added 6 commits August 20, 2018 10:41

Add node_install to BUILD file and remove debugger statement

0b4eba4

Turn off read cache invalidation for node_resolve_local and remove un…

9b7e628

…used shrinkwap for yarn node modules

NodeResolveLocal should be installed under node-resolve-local rather …

9b92628

…than resolve

Add handling for node_preinstalled_module_resolver for local node-ins…

aea20de

…talls

Switch node server-project example to yarn

a563089

Add note about how node will be resolved twice for node-install

d54b902

stuhood reviewed Aug 20, 2018

View reviewed changes

Ny Saechao added 3 commits August 22, 2018 13:08

Expose additional install parameters for node_modules

56faa61

Build an abstract class for node_resolve and node_paths to make disti…

eedbebe

…nction more clear

Add docstring explaining NodeInstall task

68f3851

nsaechao changed the title ~~[WIP] Add a node-install goal to Pants for installing node_modules~~ Add a node-install goal to Pants for installing node_modules Aug 24, 2018

nsaechao mentioned this pull request Aug 27, 2018

Pants should support Node.js source-level dependencies #6332

Closed

Ny Saechao added 7 commits August 29, 2018 15:35

Merge node-resolve-local with resolve.node by optionally producing No…

df0750a

…dePathsLocal

Separate node-install integration tests into its own file

4144bcc

Update notes to reflect why --npm-resolver-force-option-override is a…

7cc4dd1

… thing and that it is an ADVANCED option

Add missing test_node_install_integration.py file

09215e5

nop to re-run travis ci

bca8a91

Remove node-resolve-local tests as the task no longer exists

764e8c3

Merge branch 'master' into nsaechao/issue-6332_install_goal

554b9e9

stuhood reviewed Aug 30, 2018

View reviewed changes

stuhood requested review from baroquebobcat and jsirois August 30, 2018 21:52

Ny Saechao added 2 commits August 31, 2018 15:04

Add docstrings to node resolve and npm resolver

74e04d3

Remove stale comment

b4e3942

stuhood requested review from benjyw and removed request for jsirois September 4, 2018 18:08

stuhood approved these changes Sep 4, 2018

View reviewed changes

Update --npm-resolver-production-only option to --npm-resolver-instal…

52d98a2

…l-production and improve docstrings

benjyw approved these changes Sep 6, 2018

View reviewed changes

stuhood approved these changes Sep 7, 2018

View reviewed changes

stuhood merged commit 371504e into pantsbuild:master Sep 7, 2018



		class NodePathsLocal(NodePaths):
		"""Maps Node package targets to their local NODE_PATH chroot."""

		from pants.contrib.node.tasks.node_resolve import NodeResolve


		class NodeResolveLocal(NodeResolve):

		@@ -4,9 +4,8 @@ Hello!

		You can also run one of the following command to greet with name:



		class NodeInstall(NodeTask):
		"""Installs a node_module target into the source directory



		class NodePathsLocal(NodePathsBase):
		"""Maps Node package targets to their local NODE_PATH chroot.

Uh oh!

Conversation

nsaechao commented Aug 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Result

Uh oh!

nsaechao commented Aug 18, 2018

Uh oh!

stuhood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nsaechao commented Aug 24, 2018

Uh oh!

stuhood commented Aug 27, 2018

Uh oh!

stuhood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nsaechao commented Aug 31, 2018

Uh oh!

benjyw commented Sep 4, 2018

Uh oh!

stuhood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nsaechao commented Sep 6, 2018

Uh oh!

nsaechao commented Sep 6, 2018

Uh oh!

benjyw left a comment

Choose a reason for hiding this comment

Uh oh!

stuhood left a comment

nsaechao commented Aug 18, 2018 •

edited

Loading