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

Pants should manage local dependencies defined in package.json for node_module targets #6524

Merged
merged 17 commits into from Oct 10, 2018

Conversation

Projects
None yet
3 participants
@nsaechao
Copy link
Collaborator

nsaechao commented Sep 19, 2018

This is the implementation for #6488.

Problem

Prior to this pull request, a node_module could not depend on another node_module target at the source-level expecting to work. To depend on another node target within the same repo, one needed to build the node package and publish it to an external registry and then depend on it as a third party dependency.

Solution

With this change, node_module targets built using package_manager='yarnpkg' can locally depend on another node_module target built using package_manager='yarnpkg' without an external asset provider.

This is accomplished by:

  • Specifying the source-level dependency within package.json under the dependencies field using the file: specifier with relative pathing to the project directory.
  • Within the BUILD target definition, the node_module target also needs to duplicate the dependencies using the pants target address.
  • The package name of the dependency must equal to the base target name. I.E. The package name 'dep' must match the target address 'src/node/target/dep:dep`.

Outcome

For every source-level dependency within a node_module target, install the dependencies in topological order. After installing each dependency, create a relative symlink under the target's node_modules directory pointing to the dependency's node path. You can see how this will recurse transitively.

Limitations

  • Source-level dependencies is only supported with the Yarnpkg package manager.
  • Dependencies need to be specified in both package.json and BUILD
  • @ is not allowed, so scope is not addressed within this PR. See #6540.
Ny Saechao

@nsaechao nsaechao changed the title [WIP] Pants should manage local dependencies defined in package.json for node_module targets Pants should manage local dependencies defined in package.json for node_module targets Sep 25, 2018

@stuhood
Copy link
Member

stuhood left a comment

Thanks!

@@ -0,0 +1,23 @@
node_module(
sources=rglobs('package.json', 'yarn.lock', 'adder/*', 'add-two/*'),

This comment has been minimized.

@stuhood

stuhood Sep 25, 2018

Member

This being recursive is going to result in this target owning multiple package.json files. If you only wanted to be recursive with respect to adder and add-two, then you'd want to use adder/**, etc (we use git syntax, so a double glob is recursive).

It might be good to add a comment showing what this is demonstrating (something like: "one pants target that owns sources in two subdirectories, and depends on another target for the third").

:param string package_name: A package.json name that is required to be the same as the target name
:param string file_path: Relative filepath from target to the package in the format 'file:<address_path>'
"""
pattern = re.compile('^file:(.*)$')

This comment has been minimized.

@stuhood

stuhood Sep 25, 2018

Member

Should move the compilation of the regex out into a class property, probably.

@@ -35,3 +36,25 @@ def _copy_sources(self, target, results_dir):
dest = os.path.join(results_dir, os.path.relpath(source, source_relative_to))
safe_mkdir(os.path.dirname(dest))
shutil.copyfile(os.path.join(buildroot, source), dest)

def _get_target_from_package_name(self, target, package_name, file_path):
"""Get a target from its dependency tree given a package name and relative file path.

This comment has been minimized.

@stuhood

stuhood Sep 25, 2018

Member

This is not going to operate on the dependency tree: if I'm understanding it correctly, it will find the dep with the matching package name in the direct dependencies of the given target.

:param string file_path: Relative filepath from target to the package in the format 'file:<address_path>'
"""
pattern = re.compile('^file:(.*)$')
address_path = pattern.match(file_path).group(1)

This comment has been minimized.

@stuhood

stuhood Sep 25, 2018

Member

Is it always necessary for these to be relative? Does package.json support having file urls that are relative to the repository root?

Pants somewhat-intentionally does not support relative addresses in BUILD files, because it makes it much, much harder to refactor them.

This comment has been minimized.

@nsaechao

nsaechao Oct 1, 2018

Collaborator

Unfortunately no, the file: specifier is relative to the root of the package.json to be backwards compatible with "yarn" and "npm". Alternatively we could in the future add a new dependency type pantsDependencies as a potential follow up to this implementation which would take a pants target address which would be relative to the build root.

@@ -22,6 +23,17 @@ def atomic_copy(src, dst):
os.rename(tmp_dst.name, dst)


@contextmanager
def safe_temp_edit(filename):
"""Safely modify a file within context that automatically reverts any changes afterwards"""

This comment has been minimized.

@stuhood

stuhood Sep 25, 2018

Member

Mutating files in the buildroot like this is effectively a non-starter. For one thing, if you crash, this will not get cleaned up. But also, this will cause pantsd to churn unnecessarily for file invalidations (and will break in the presence of concurrency).

Is it possible to run the relevant install command with an explicit flag that tells it which package.json file to use? Something like yarn install --package-json=${my_tmp_file}?

This comment has been minimized.

@nsaechao

nsaechao Oct 1, 2018

Collaborator

Unfortunately the path to the package.json is heavily relied on the node package manager tools as the package.json serves as the root location that all other relative paths relies on. I don't think this would be supported any time. The mutation for buildroot would only occur for the node-install command which is used to mutate the yarn.lock file (such as adding a new dependency). Both the package.json and the yarn.lock files are required to be checked in. There is potential case for lossy, as mentioned above a non-backwards compatibile solution could be implemented in the future to better support pants-specific dependencies. Hopefully any catastrophic loss of package.json can be recovered by the code versioning system.

This comment has been minimized.

@stuhood

stuhood Oct 4, 2018

Member

Can you put these caveats in comments both 1) at the callsite for this method, 2) in the method doc to indicate that it is actually mutating the file in place.

Show resolved Hide resolved .../node/src/python/pants/contrib/node/subsystems/resolvers/npm_resolver.py
bin_field = dep.payload.bin_executables
if dep.payload.bin_executables:
bin_dir = os.path.join(node_module_dir, '.bin')
if isinstance(bin_field, dict):

This comment has been minimized.

@stuhood

stuhood Sep 25, 2018

Member

It would be good to move this logic to the target type in order to centralize consumption.

But you might also consider normalizing it at target creation time?

# Symlink each target
dep_path = node_paths.node_path(dep)
node_module_dir = os.path.join(results_dir, 'node_modules')
absolute_symlink(dep_path, os.path.join(node_module_dir, dep.package_name))

This comment has been minimized.

@stuhood

stuhood Sep 25, 2018

Member

I think you want to use relative symlinks rather than absolute symlinks for cases like this... an absolute symlink will break if a git checkout is moved.

Show resolved Hide resolved contrib/node/src/python/pants/contrib/node/targets/node_module.py
@@ -53,7 +59,8 @@ def __init__(
'package_manager': PrimitiveField(package_manager),
'output_dir': PrimitiveField(output_dir),
'dev_dependency': PrimitiveField(dev_dependency),
'style_ignore_path': PrimitiveField(style_ignore_path)
'style_ignore_path': PrimitiveField(style_ignore_path),
'bin_executables': PrimitiveField(bin_executables)

This comment has been minimized.

@stuhood

stuhood Sep 25, 2018

Member

Recommend adding trailing commas to avoid (in future) the extra diff you see here on the previous line.

@baroquebobcat

This comment has been minimized.

Copy link
Contributor

baroquebobcat commented Sep 28, 2018

Looks like the new test_safe_temp_edit is failing in travis.

@nsaechao

This comment has been minimized.

Copy link
Collaborator

nsaechao commented Oct 1, 2018

Looks like the new test_safe_temp_edit is failing in travis.

done

@baroquebobcat
Copy link
Contributor

baroquebobcat left a comment

It's looking pretty good. I've got a number of small comments below.

Show resolved Hide resolved ...src/python/pants/contrib/node/subsystems/resolvers/node_resolver_base.py
Show resolved Hide resolved contrib/node/src/python/pants/contrib/node/targets/node_module.py
@@ -0,0 +1,361 @@
# coding=utf-8
# Copyright 2015 Pants project contributors (see CONTRIBUTORS.md).

This comment has been minimized.

@baroquebobcat

baroquebobcat Oct 2, 2018

Contributor

nit: year

This comment has been minimized.

@nsaechao

nsaechao Oct 8, 2018

Collaborator

Nice catch

def _create_trans_dep(self):
"""Create a transitive dependency target
:param with_trans_dep: If true, assumes that trans_dep is already created and links

This comment has been minimized.

@baroquebobcat

baroquebobcat Oct 2, 2018

Contributor

xx there isn't a param anymore

This comment has been minimized.

@nsaechao

nsaechao Oct 8, 2018

Collaborator

Thanks, removed

dep_node_path = node_paths.node_path(dep)
app_node_path = node_paths.node_path(app)
self.assertIsNotNone(dep_node_path)
self.assertIsNotNone(app_node_path)

This comment has been minimized.

@baroquebobcat

baroquebobcat Oct 2, 2018

Contributor

Could you have a stronger assertion here than just isNotNone?

This comment has been minimized.

@nsaechao

nsaechao Oct 8, 2018

Collaborator

There are stronger assertions down the line to verify the paths are correct

expected = os.path.relpath(app_bin_dep_path, dep_bin_path)
self.assertEqual(relative_path, expected)

def test_resolve_transitive_deps(self):

This comment has been minimized.

@baroquebobcat

baroquebobcat Oct 2, 2018

Contributor

This also invokes the transitive dep dependent project. Could you note that in the test name? Maybe test_resolve_and_run_transitive_deps

script_path = os.path.join(app_node_path, 'index.js')
out = task.node_distribution.node_command(args=[script_path]).check_output()
lines = {line.strip() for line in out.splitlines()}
self.assertIn('2', lines)

This comment has been minimized.

@baroquebobcat

baroquebobcat Oct 2, 2018

Contributor

It took a little doing to figure out where this '2' is supposed to come from. It'd be easier to follow if the transitive dep printed something specific. Or, you could add a comment.

dep = self._create_dep()
app = self._create_app(dep, dep_not_found=True)
with self.assertRaises(TaskError):
self._resolve_target(app)

This comment has been minimized.

@baroquebobcat

baroquebobcat Oct 2, 2018

Contributor

Thanks for the test covering a missing dep! Could you add an assertion about the error message?

script_path = os.path.join(workspace_node_path, 'node_modules', 'app', 'index.js')
out = task.node_distribution.node_command(args=[script_path]).check_output()
lines = {line.strip() for line in out.splitlines()}
self.assertIn('2', lines)

This comment has been minimized.

@baroquebobcat

baroquebobcat Oct 2, 2018

Contributor

This 2 too.

@@ -412,6 +412,7 @@ def relative_symlink(source_path, link_path):
if os.path.lexists(link_path):
os.unlink(link_path)
rel_path = os.path.relpath(source_path, os.path.dirname(link_path))
safe_mkdir_for(link_path)

This comment has been minimized.

@baroquebobcat

baroquebobcat Oct 2, 2018

Contributor

Could you add a test case covering the scenario fixed by adding this line?

@stuhood

stuhood approved these changes Oct 4, 2018

Copy link
Member

stuhood left a comment

Thanks.

@@ -22,6 +23,17 @@ def atomic_copy(src, dst):
os.rename(tmp_dst.name, dst)


@contextmanager
def safe_temp_edit(filename):
"""Safely modify a file within context that automatically reverts any changes afterwards"""

This comment has been minimized.

@stuhood

stuhood Oct 4, 2018

Member

Can you put these caveats in comments both 1) at the callsite for this method, 2) in the method doc to indicate that it is actually mutating the file in place.

Show resolved Hide resolved ...src/python/pants/contrib/node/subsystems/resolvers/node_resolver_base.py
with safe_temp_edit('package.json') as package_json:
with open(package_json, 'r') as package_json_file:
json_data = json.load(package_json_file)
source_deps = { k : v for k,v in json_data.get('dependencies', {}).items() if v.startswith('file:')}

This comment has been minimized.

@stuhood

stuhood Oct 4, 2018

Member

This would still be good to do.

Ny Saechao
@nsaechao

This comment has been minimized.

Copy link
Collaborator

nsaechao commented Oct 9, 2018

Sorry, it looks like I made the updates to the code reviews in my child branch instead of this branch. I cherry-picked the updates, so hopefully we can get a passing CI

@baroquebobcat
Copy link
Contributor

baroquebobcat left a comment

looks good to me!

@baroquebobcat baroquebobcat merged commit b612487 into pantsbuild:master Oct 10, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment