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

Remodel of node subsystem #5698

Merged
merged 15 commits into from Apr 23, 2018

Conversation

Projects
None yet
4 participants
@UnrememberMe
Copy link
Contributor

UnrememberMe commented Apr 13, 2018

Problem

There are two package managers that are currently supported by Pants, npm and yarn. The codes to invoke those two package manager are scattered in various task and usually invokes a pattern of codes looks like:

  • get the package manager defined at target level;
  • if not defined, use subsystem default package manager;
  • if package manager is npm, generate npm command line;
  • otherwise if package manager is yarn, generate yarn command line;
  • run the generated command in task context.

Solution

Remodel node subsystem code to provide better package manager support. PackageManagers are used to remove package manager selection logic from task level. It also isolates npm/yarn specific codes into separate classes that is easier to understand.

Minimal work has go into eslint javascript style task. Further remodeling will be required.

Result

There should be no end user impact, while the codes should be more readable and easier to add future package manager related functionalities. It should also be easier to add more tools for node if necessary.

@@ -44,18 +47,14 @@ def subsystem_dependencies(cls):
def register_options(cls, register):
super(NodeDistribution, cls).register_options(register)
register('--supportdir', advanced=True, default='bin/node',
removal_version='1.7.0.dev0', removal_hint='No longer supported.',
removal_version='1.8.0.dev0', removal_hint='No longer supported.',

This comment has been minimized.

@benjyw

benjyw Apr 13, 2018

Contributor

Note that you can just kill these deprecated options entirely, since we're already on 1.7.0 dev releases.

This comment has been minimized.

@UnrememberMe

UnrememberMe Apr 16, 2018

Contributor

done.

@cosmicexplorer
Copy link
Contributor

cosmicexplorer left a comment

I love node and I'm going to make another pr to add coffeescript support after this one is merged.

"""
kwargs = kwargs.copy()
env = kwargs.pop('env', os.environ).copy()
env['PATH'] = os.path.pathsep.join(self.extra_paths + [env.get('PATH', '')])

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Apr 14, 2018

Contributor

I added a method get_joined_path() to contextutil.py in #5490. That method might be a bit messy (and you might be able to rewrite it much better), but it would be great to have path handling methods in a single place so they can be shared across different parts of pants and continually improved. If you could dump this logic in a method in contextutil.py (or maybe process_handler.py?) and call it here, that would be cool, but if you don't want to in this pr I can do that myself in a new pr.

This comment has been minimized.

@UnrememberMe

UnrememberMe Apr 17, 2018

Contributor

done.

VALID_PACKAGE_MANAGERS = [PACKAGE_MANAGER_NPM, PACKAGE_MANAGER_YARNPKG, PACKAGE_MANAGER_YARNPKG_ALIAS]


# TODO: Change to enum type when migrated to Python 3.4+

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Apr 14, 2018

Contributor

This is a useful comment, I'm not familiar enough with python 3 to know where all the migration points are so this is helpful.

package_manager_args.append(script_name)
if script_args:
package_manager_args.append('--')
package_manager_args.extend(script_args)

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Apr 14, 2018

Contributor

Even adding arguments like this could be useful to add to util/ at some point. I feel like having some common framework to share manipulation of argument lists for invoking pants-owned binaries would be great -- a method adding '--' and the positional args could be used to wrap invocations across many different binaries without tedious error-prone argument list creation. You can see where this would have helped with something like #4958.

Just a thought -- no action needed here in this PR.

This comment has been minimized.

@benjyw

benjyw Apr 17, 2018

Contributor

Maybe just a TODO to consider it...

This comment has been minimized.

@UnrememberMe

UnrememberMe Apr 18, 2018

Contributor

done.


from pants.contrib.node.subsystems.package_managers import (
PackageInstallationTypeOption, PackageInstallationVersionOption,
PackageManagerNpm, PackageManagerYarnpkg)

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Apr 14, 2018

Contributor

I've been just making my python import lines really long, this style is much better and I will definitely do this from now on.

self, target=None, package_manager=None,
package=None, type_option=None, version_option=None,
node_paths=None, workunit_name=None, workunit_labels=None):
"""Add an additional package using requested package_manager."""

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Apr 14, 2018

Contributor

Would it be helpful for this to take a **kwargs argument to forward to either the .add_package() or ._execute_command() calls below? I see that node_paths is unused and I feel like this many keyword args gets unwieldy. Named options shared between different methods is also something that could be encapsulated in a pants.util.objects.datatype subclass. Just wanted your thoughts on whether that would be a useful change, no action needed here.

This comment has been minimized.

@UnrememberMe

UnrememberMe Apr 17, 2018

Contributor

Good catch. node_paths should be passed to the callee. There seems to have some further remodeling opportunity here to consolidate duplicated codes in add_package/install_modules/run_scripts etc. I think I will leave the codes as is for now if you don't object.

Roger Jiang added some commits Apr 16, 2018

Roger Jiang
Address integrtion tests failure for node_test.
Bring back accidentally deleted code.  For node_test, we will get the first (and only) node_module dependency, and
use the package_manager defined on the node_module.

Also address some review feedbacks.
Roger Jiang
Roger Jiang
Roger Jiang
@benjyw
Copy link
Contributor

benjyw left a comment

This is a great improvement, and really well documented, thanks! I just have some grammar nits and a couple of small comments.

@@ -2,6 +2,12 @@
# Licensed under the Apache License, Version 2.0 (see LICENSE).

python_library(
sources=[

This comment has been minimized.

@benjyw

benjyw Apr 17, 2018

Contributor

Why enumerate the sources explicitly?

This comment has been minimized.

@UnrememberMe

UnrememberMe Apr 18, 2018

Contributor

It's an old habit. In my previous projects, it was recommended to explicitly list all sources and strongly discouraged usage of glob/rglob or equivalent.
I don't know what the Pants norm is. Do we have a recommendation on this topic?

This comment has been minimized.

@benjyw

benjyw Apr 18, 2018

Contributor

I think the less boilerplate the better. We don't list sources unless we have to. They default to a glob over the appropriate file suffix, which is almost always what you want anyway.

This comment has been minimized.

@UnrememberMe

UnrememberMe Apr 19, 2018

Contributor

done.

"""
return [self.executable] + (self.args or [])

def _prepare_env(self, kwargs):

This comment has been minimized.

@benjyw

benjyw Apr 17, 2018

Contributor

This can be a @staticmethod.

This comment has been minimized.

@UnrememberMe

UnrememberMe Apr 18, 2018

Contributor

it uses self.extra_paths.

This comment has been minimized.

@benjyw

benjyw Apr 18, 2018

Contributor

Ooops, never mind.



def command_gen(tool_installations, tool_executable, args=None, node_paths=None):
"""Generate a Command object with requires tools installed and paths setup.

This comment has been minimized.

@benjyw

benjyw Apr 17, 2018

Contributor

Grammar nits:

s/requires/required/
s/setup/set up/.

This comment has been minimized.

@UnrememberMe

UnrememberMe Apr 18, 2018

Contributor

done.

logger = logging.getLogger(__name__)


class Command(namedtuple('Command', ['executable', 'args', 'extra_paths'])):

This comment has been minimized.

@benjyw

benjyw Apr 17, 2018

Contributor

There's almost nothing node-specific about this class, right? And it could be generally useful. Maybe add a # TODO: Consider generalizing this into pants.util. comment?

This comment has been minimized.

@UnrememberMe

UnrememberMe Apr 18, 2018

Contributor

done.

package_manager_args.append(script_name)
if script_args:
package_manager_args.append('--')
package_manager_args.extend(script_args)

This comment has been minimized.

@benjyw

benjyw Apr 17, 2018

Contributor

Maybe just a TODO to consider it...

@@ -38,7 +38,7 @@ def supports_passthru_args(cls):
def _run_node_distribution_command(self, command, workunit):
"""Overrides NodeTask._run_node_distribution_command.
This is what execute_npm ultimately uses to run the NodeDistribution.Command.
This is what ultimately used to run the Command.

This comment has been minimized.

@benjyw

benjyw Apr 17, 2018

Contributor

Nit:
s/ultimately/is ultimately/

This comment has been minimized.

@UnrememberMe

UnrememberMe Apr 18, 2018

Contributor

done.

Roger Jiang
@benjyw

benjyw approved these changes Apr 18, 2018

@@ -2,6 +2,12 @@
# Licensed under the Apache License, Version 2.0 (see LICENSE).

python_library(
sources=[

This comment has been minimized.

@benjyw

benjyw Apr 18, 2018

Contributor

I think the less boilerplate the better. We don't list sources unless we have to. They default to a glob over the appropriate file suffix, which is almost always what you want anyway.

"""
return [self.executable] + (self.args or [])

def _prepare_env(self, kwargs):

This comment has been minimized.

@benjyw

benjyw Apr 18, 2018

Contributor

Ooops, never mind.

removal_version='1.7.0.dev0',
removal_hint='Use --version in scope yarnpkg-distribution',
help='Yarnpkg version to use.')

register('--package-manager', advanced=True, default='npm', fingerprint=True,

This comment has been minimized.

@nsaechao

nsaechao Apr 20, 2018

Collaborator

nit: PACKAGE_MANAGER_NPM instead of 'npm'

self.name = name
self.tool_installations = tool_installations

def _get_installation_args(self, install_optional, production_only, force):

This comment has been minimized.

@nsaechao

nsaechao Apr 20, 2018

Collaborator

should we consider defining a **kwargs option here as well?

This comment has been minimized.

@nsaechao

nsaechao Apr 20, 2018

Collaborator

for yarn, I can see something like --pure-lockfile being a usecase for this.

@benjyw benjyw merged commit 2b15199 into pantsbuild:master Apr 23, 2018

1 check passed

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

@UnrememberMe UnrememberMe deleted the UnrememberMe:UnrememberMe/node_system_limited_remodel branch Apr 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment