Skip to content
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

Add a node-install goal to Pants for installing node_modules #6367

Merged
merged 21 commits into from Sep 7, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
c98be5a
Add base skeleton code for node-install goal
Aug 17, 2018
27fe727
First iteration for ./pants install-node
Aug 17, 2018
0b4eba4
Add node_install to BUILD file and remove debugger statement
Aug 20, 2018
9b7e628
Turn off read cache invalidation for node_resolve_local and remove un…
Aug 20, 2018
9b92628
NodeResolveLocal should be installed under node-resolve-local rather …
Aug 20, 2018
aea20de
Add handling for node_preinstalled_module_resolver for local node-ins…
Aug 20, 2018
a563089
Switch node server-project example to yarn
Aug 20, 2018
d54b902
Add note about how node will be resolved twice for node-install
Aug 20, 2018
56faa61
Expose additional install parameters for node_modules
Aug 22, 2018
eedbebe
Build an abstract class for node_resolve and node_paths to make disti…
Aug 24, 2018
68f3851
Add docstring explaining NodeInstall task
Aug 24, 2018
df0750a
Merge node-resolve-local with resolve.node by optionally producing No…
Aug 29, 2018
4144bcc
Separate node-install integration tests into its own file
Aug 29, 2018
7cc4dd1
Update notes to reflect why --npm-resolver-force-option-override is a…
Aug 29, 2018
09215e5
Add missing test_node_install_integration.py file
Aug 29, 2018
bca8a91
nop to re-run travis ci
Aug 30, 2018
764e8c3
Remove node-resolve-local tests as the task no longer exists
Aug 30, 2018
554b9e9
Merge branch 'master' into nsaechao/issue-6332_install_goal
Aug 30, 2018
74e04d3
Add docstrings to node resolve and npm resolver
Aug 31, 2018
b4e3942
Remove stale comment
Aug 31, 2018
52d98a2
Update --npm-resolver-production-only option to --npm-resolver-instal…
Sep 6, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions contrib/node/src/python/pants/contrib/node/register.py
Expand Up @@ -5,6 +5,7 @@
from __future__ import absolute_import, division, print_function, unicode_literals

from pants.build_graph.build_file_aliases import BuildFileAliases
from pants.goal.goal import Goal
from pants.goal.task_registrar import TaskRegistrar as task

from pants.contrib.node.subsystems.resolvers.node_preinstalled_module_resolver import \
Expand All @@ -18,8 +19,10 @@
from pants.contrib.node.tasks.javascript_style import JavascriptStyleFmt, JavascriptStyleLint
from pants.contrib.node.tasks.node_build import NodeBuild
from pants.contrib.node.tasks.node_bundle import NodeBundle as NodeBundleTask
from pants.contrib.node.tasks.node_install import NodeInstall
from pants.contrib.node.tasks.node_repl import NodeRepl
from pants.contrib.node.tasks.node_resolve import NodeResolve
from pants.contrib.node.tasks.node_resolve_local import NodeResolveLocal
from pants.contrib.node.tasks.node_run import NodeRun
from pants.contrib.node.tasks.node_test import NodeTest as NodeTestTask

Expand All @@ -37,12 +40,19 @@ def build_file_aliases():


def register_goals():
# Register descriptions.
nsaechao marked this conversation as resolved.
Show resolved Hide resolved
Goal.register('node-install', 'Installs a node_module target.')

# Register tasks.

task(name='node', action=NodeRepl).install('repl')
task(name='node', action=NodeResolve).install('resolve')
task(name='node-install', action=NodeResolveLocal).install('resolve')
task(name='node', action=NodeRun).install('run')
task(name='node', action=NodeBuild).install('compile', first=True)
task(name='node', action=NodeTestTask).install('test')
task(name='node', action=NodeBundleTask).install('bundle')
task(name='node', action=NodeInstall).install('node-install')
nsaechao marked this conversation as resolved.
Show resolved Hide resolved
# Linting
task(name='javascriptstyle', action=JavascriptStyleLint).install('lint')
task(name='javascriptstyle', action=JavascriptStyleFmt).install('fmt')
Expand Down
Expand Up @@ -18,6 +18,7 @@
from pants.contrib.node.subsystems.resolvers.node_resolver_base import NodeResolverBase
from pants.contrib.node.targets.node_preinstalled_module import NodePreinstalledModule
from pants.contrib.node.tasks.node_resolve import NodeResolve
from pants.contrib.node.tasks.node_resolve_local import NodeResolveLocal


class NodePreinstalledModuleResolver(Subsystem, NodeResolverBase):
Expand All @@ -29,8 +30,9 @@ def register_options(cls, register):
help='Timeout the fetch if the connection is idle for longer than this value.')
super(NodePreinstalledModuleResolver, cls).register_options(register)
NodeResolve.register_resolver_for_type(NodePreinstalledModule, cls)
NodeResolveLocal.register_resolver_for_type(NodePreinstalledModule, cls)

def resolve_target(self, node_task, target, results_dir, node_paths):
def resolve_target(self, node_task, target, results_dir, node_paths, resolve_locally=False):
self._copy_sources(target, results_dir)

with temporary_dir() as temp_dir:
Expand Down
Expand Up @@ -15,7 +15,7 @@

class NodeResolverBase(AbstractClass):
@abstractmethod
def resolve_target(self, node_task, target, results_dir, node_paths):
def resolve_target(self, node_task, target, results_dir, node_paths, resolve_locally=False):
"""Resolve a NodePackage target."""

@classmethod
Expand Down
Expand Up @@ -19,6 +19,7 @@
from pants.contrib.node.subsystems.resolvers.node_resolver_base import NodeResolverBase
from pants.contrib.node.targets.node_module import NodeModule
from pants.contrib.node.tasks.node_resolve import NodeResolve
from pants.contrib.node.tasks.node_resolve_local import NodeResolveLocal


class NpmResolver(Subsystem, NodeResolverBase):
Expand All @@ -31,16 +32,20 @@ def register_options(cls, register):
'--install-optional', type=bool, default=False, fingerprint=True,
help='If enabled, install optional dependencies.')
NodeResolve.register_resolver_for_type(NodeModule, cls)
NodeResolveLocal.register_resolver_for_type(NodeModule, cls)

def resolve_target(self, node_task, target, results_dir, node_paths):
self._copy_sources(target, results_dir)
def resolve_target(self, node_task, target, results_dir, node_paths, resolve_locally=False):
if not resolve_locally:
self._copy_sources(target, results_dir)
with pushd(results_dir):
if not os.path.exists('package.json'):
raise TaskError(
'Cannot find package.json. Did you forget to put it in target sources?')
# TODO: remove/remodel the following section when node_module dependency is fleshed out.
package_manager = node_task.get_package_manager(target=target).name
if package_manager == PACKAGE_MANAGER_NPM:
if resolve_locally:
raise TaskError('Resolving node package modules locally is not supported for NPM.')
if os.path.exists('npm-shrinkwrap.json'):
node_task.context.log.info('Found npm-shrinkwrap.json, will not inject package.json')
else:
Expand Down
23 changes: 20 additions & 3 deletions contrib/node/src/python/pants/contrib/node/tasks/BUILD
Expand Up @@ -10,12 +10,15 @@ target(
':node_run',
':node_test',
':javascript_style',
':node_install',
]
)

python_library(
name='node_paths',
sources=['node_paths.py'],
sources=[
'node_paths.py',
'node_paths_local.py'],
dependencies = [
'3rdparty/python:future',
],
Expand Down Expand Up @@ -77,7 +80,9 @@ python_library(

python_library(
name='node_resolve',
sources=['node_resolve.py'],
sources=[
'node_resolve.py',
'node_resolve_local.py'],
dependencies=[
':node_paths',
':node_task',
Expand Down Expand Up @@ -125,4 +130,16 @@ python_library(
'src/python/pants/util:contextutil',
'src/python/pants/util:process_handler',
]
)
)

python_library(
name='node_install',
sources=['node_install.py'],
dependencies=[
':node_paths',
':node_task',
'src/python/pants/base:exceptions',
'src/python/pants/base:workunit',
'src/python/pants/util:contextutil',
]
)
28 changes: 28 additions & 0 deletions contrib/node/src/python/pants/contrib/node/tasks/node_install.py
@@ -0,0 +1,28 @@
# coding=utf-8
# Copyright 2015 Pants project contributors (see CONTRIBUTORS.md).
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2018

# Licensed under the Apache License, Version 2.0 (see LICENSE).

from __future__ import absolute_import, division, print_function, unicode_literals

from pants.contrib.node.tasks.node_paths_local import NodePathsLocal
from pants.contrib.node.tasks.node_task import NodeTask


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

@classmethod
def prepare(cls, options, round_manager):
super(NodeInstall, cls).prepare(options, round_manager)
round_manager.require_data(NodePathsLocal)

@classmethod
def supports_passthru_args(cls):
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to use passthru_args... does one of the parent classes? That would look like ./pants node-install $target -- $passthru_args

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still relevant.

return True

def execute(self):
for target in self.context.target_roots:
if self.is_node_module(target):
node_paths = self.context.products.get_data(NodePathsLocal)
self.context.log.debug('Start installing node_module target: {}'.format(target))
self.context.log.debug('Node_path: {}'.format(node_paths.node_path(target)))
@@ -0,0 +1,11 @@
# 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, print_function, unicode_literals

from pants.contrib.node.tasks.node_paths import NodePaths


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -0,0 +1,54 @@
# 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, print_function, unicode_literals

import os

from pants.base.workunit import WorkUnitLabel

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


class NodeResolveLocal(NodeResolve):
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

"""Resolves node_package targets to source chroots using different registered resolvers."""

@classmethod
def product_types(cls):
return [NodePathsLocal]

@property
def cache_target_dirs(self):
# Do not create a results_dir for this task
return True

def artifact_cache_reads_enabled(self):
# Artifact caching is not necessary for local installation.
# Just depend on the local cache provided by the package manager.
return self._cache_factory.read_cache_available()

def execute(self):
targets = self.context.targets(predicate=self._can_resolve_target)
if not targets:
return

node_paths = self.context.products.get_data(NodePathsLocal, init_func=NodePathsLocal)
# Invalidate all targets for this task, and use invalidated() check
# to build topologically sorted target graph. This is probably not the best way
# to do this, but it works.
self.invalidate()
with self.invalidated(targets,
# This is necessary to ensure that transitive dependencies are installed first
topological_order=True,
invalidate_dependents=True,
silent=True) as invalidation_check:
with self.context.new_workunit(name='node-install', labels=[WorkUnitLabel.MULTITOOL]):
for vt in invalidation_check.all_vts:
target = vt.target
if not vt.valid:
resolver_for_target_type = self._resolver_for_target(target).global_instance()
results_dir = os.path.abspath(target.address.spec_path)
resolver_for_target_type.resolve_target(self, target, results_dir, node_paths, resolve_locally=True)
node_paths.resolved(target, results_dir)