Issue 5559 - include Node.js in ensure_dependencies.py

ensure_dependencies.py

 #!/usr/bin/env python
 
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 import sys
 import os
 import posixpath
 import re
 import io
 import errno
 import logging
 import subprocess
 import urlparse
 import argparse
+import json
 
 from collections import OrderedDict
 from ConfigParser import RawConfigParser
 
 USAGE = '''
 A dependencies file should look like this:
 
   # VCS-specific root URLs for the repositories
   _root = hg:https://hg.adblockplus.org/ git:https://github.com/adblockplus/
   # File to update this script from (optional)
@@ skipping 211 matching lines @@
     return os.path.join(path, *normpath.split(posixpath.sep))
 
 
 def get_repo_type(repo):
     for name, repotype in repo_types.iteritems():
         if repotype.istype(repo):
             return name
     return 'hg'
 
 
+def resolve_npm_dependencies(target, vcs):
+    """Install Node.js production-only dependencies if necessary and desired.
+
+    When the target dependency has additional Node.js dependencies declared
+    and when those are outdated (or not installed), run
+    "npm install --only=production" to resolve the declared dependencies.
+
+    Additionally, make sure that any VCS will ignore the installed files
+    from any dependency.
+    Do nothing when the SKIP_DEPENDENCY_UPDATES environment variable is set.

[Wladimir Palant, 2017/08/24 11:38:12]

SKIP_DEPENDENCY_UPDATES should be handled implicitly if this only runs on
repository update.

[tlucas, 2017/08/24 13:27:54]

Acknowledged.

[tlucas, 2017/08/25 09:55:03]

Done.

+
+    Requires Node.js to be installed locally.
+    """
+    try:
+        with open(os.path.join(target, 'package.json'), 'r') as fp:
+            package_data = json.load(fp)
+
+        # In case a package.json does not exist at all or if there are no
+        # production-dependencies declared, we don't need to run npm and can

[Wladimir Palant, 2017/08/24 11:38:12]

Nit: "production dependencies"

[tlucas, 2017/08/24 13:27:54]

Acknowledged.

[tlucas, 2017/08/25 09:55:03]

Done.

+        # bail out early
+        if not package_data.get('dependencies', False):
+            return
+    except IOError:
+        return
+
+    try:
+        subprocess_kwargs = {
+            'stdout': subprocess.PIPE,
+            'stderr': subprocess.PIPE,
+            'cwd': target,
+        }

[Wladimir Palant, 2017/08/24 11:38:12]

I'd rather have the arguments specified in the actual call rather than in a
separate dictionary.

[tlucas, 2017/08/24 13:27:54]

Acknowledged.

[tlucas, 2017/08/25 09:55:02]

Done.

+
+        if subprocess.call(['npm', 'outdated'], **subprocess_kwargs) == 0:

[Wladimir Palant, 2017/08/24 11:38:12]

Given that `npm outdated` hits the network, I'd rather not call it - just call
`npm install` directly. Besides, this doesn't work correctly anyway, with `npm
outdated` listing outdated development dependencies as well.

[tlucas, 2017/08/24 13:27:54]

What do you think about looking for the desired dependencies in a
"packages-lock.json", created by "npm install"? this way we would not perform
network operations and we would still be able to warn the user about
SKIP_DEPENDENCIES_UPDATES only if an update is necessary, but before the actual
installation would happen (which would be the same behavior as in ensure_repo /
update_repo)

[Wladimir Palant, 2017/08/25 08:10:42]

I don't have a packages-lock.json file. There is probably a reason for that,
which we could figure out. Still, it seems to be very much an implementation
detail that we shouldn't rely on.

IMHO you are overcomplicating. I also tried finding a local approach to
determining whether `npm install` would need to do something - surprisingly,
there doesn't seem to be one. However, that should be fine. The dependencies
cannot change unless the repository state changes. In the
SKIP_DEPENDENCY_UPDATES scenario we already produce a warning due to repository
not being updated, we don't need another. So we can just run `npm install`
blindly whenever changing repository state, this should be good enough and avoid
unnecessary complexity.

[Wladimir Palant, 2017/08/25 08:16:58]

And that reason is an outdated npm version (was obvious once I realized that npm
version and Node.js version aren't identical). This doesn't affect the
conclusions.

[tlucas, 2017/08/25 09:55:02]

Fair enough - Done

+            return
+
+        if SKIP_DEPENDENCY_UPDATES:
+            logging.warning('SKIP_DEPENDENCY_UPDATES environment variable '
+                            'set, not ensuring Node.js dependencies in %s',
+                            target)
+            return
+
+        cmd = ['npm', 'install', '--only=production']
+        npm_process = subprocess.Popen(cmd, **subprocess_kwargs)
+        output, error = npm_process.communicate()

[Wladimir Palant, 2017/08/24 11:38:12]

Better use subprocess.check_output() - this will set up stdout and raise errors
automatically. As to stderr output from npm, we should not suppress it IMHO.

[tlucas, 2017/08/24 13:27:54]

Acknowledged, it should be check_output.


Regarding stderr: npm does not only print errors to stderr (with version 8.x,
installed as described in [1]) - e.g. when first run, it gives the note "npm
notice created a lockfile as package-lock.json. You should commit this file."
via stderr - which imo is not an error)

The point of this was to only show an error, if an actual error happened.
I could change this, but imho this would cause unnecessary confusion for the
users. What do you think about this?

[1] https://nodejs.org/en/download/package-manager/

[Wladimir Palant, 2017/08/25 08:10:42]

npm documentation could definitely be better, but I found that you can add
`--loglevel=warn` parameter. This will hopefully be sufficient to suppress
unnecessary messages.

[tlucas, 2017/08/25 09:55:03]

Yes, this is sufficient (as discussed in irc, status-updates are ok) - Done.

+        logging.info('Node.js: ' + output)

[Wladimir Palant, 2017/08/24 11:38:12]

It's npm output, not Node.js. Besides, I doubt that the module tree produced
here is helpful in any way, I'd rather suppress the output.

[tlucas, 2017/08/24 13:27:54]

Acknowledged.

[tlucas, 2017/08/25 09:55:02]

Done.

+
+        if npm_process.returncode != 0:
+            raise Exception(error)
+
+        # Make sure Node.js related files / folders are ignored by the VCS in
+        # use
+        repo_types[vcs].ignore(
+            os.path.join(target, 'package-lock.json'), target
+        )

[Wladimir Palant, 2017/08/25 08:16:58]

It seems that this file should indeed be committed, so please don't ignore it.
We'll need to add it in both buildtools and adblockpluscore.

[tlucas, 2017/08/25 09:55:03]

Done.

+        repo_types[vcs].ignore(
+            os.path.join(target, 'node_modules'), target
+        )
+    except OSError as e:
+        import errno
+        if e.errno == errno.ENOENT:
+            logging.error('Failed to install Node.js dependencies for %s,'
+                          ' please ensure Node.js is installed.', target)
+        else:
+            raise
+
+
 def ensure_repo(parentrepo, parenttype, target, type, root, sourcename):
     if os.path.exists(target):
         return
 
     if SKIP_DEPENDENCY_UPDATES:
         logging.warning('SKIP_DEPENDENCY_UPDATES environment variable set, '
                         '%s not cloned', target)
         return
 
     postprocess_url = repo_types[type].postprocess_url
@@ skipping 57 matching lines @@
             if key == parenttype or key is None and vcs != '*':
                 vcs = key
         source, rev = merge_seqs(sources.get('*'), sources.get(vcs))
 
         if not (vcs and source and rev):
             logging.warning('No valid source / revision found to create %s' % target)
             continue
 
         ensure_repo(repodir, parenttype, target, vcs, _root.get(vcs, ''), source)
         update_repo(target, vcs, rev)
+        resolve_npm_dependencies(target, vcs)

[Wladimir Palant, 2017/08/24 11:38:12]

This will be called every time, not merely when the repository is updated. `npm
install` requires network access and might be a heavy operation, so we shouldn't
run it unnecessarily. Maybe return a result from ensure_repo() and update_repo()
indicating whether doing something was necessary?

[tlucas, 2017/08/24 13:27:54]

I thought about this too - but this would require those using
ensure_dependencies.py to run 'npm install' manually for the buildtools, if they
updated the buildtools dependency from the current revision to a future
iteration, where a Node.js dependency was already added (e.g. #5560).

When run, ensure_dependencies.py would just update the buildtools-repository and
overwrite itself with the updated version, which would then be restarted. After
restarting, ensure_dependencies.py would figure that buildtools is up-to-date
and hence skip the npm-dependencies.

I agree that this could be a corner case (and that the current implementation is
a rather heavy implementation) - but this would make sure that running
ensure_dependencies.py once results in all desired dependencies being met
immediately, without having to do things manually.

Additionally, if we agree on what i replied to the "npm outdated" comment, this
would also have no network-operation at all (just like ensure_repo /
update_repo)

What do you think about this?

[Wladimir Palant, 2017/08/25 08:10:42]

The way I see it, this wouldn't normally be an issue - the previous version of
ensure_dependencies.py would take care of installing npm dependencies when
updating buildtools repository. It's only a problem when updating from
ensure_dependencies.py without support of npm dependencies to one with such
support. In other words, it's a one-time thing. And it should be solved by
updating buildtools dependency in two steps: first we update
ensure_dependencies.py, then we add package.json to buildtools. We should add
"integration notes" section to issue 5559 description to explain that.

[tlucas, 2017/08/25 09:55:03]

Yes, that sounds reasonable. Done.

         resolve_deps(target, level + 1, self_update=False,
                      overrideroots=overrideroots, skipdependencies=skipdependencies)
 
     if self_update and '_self' in config and '*' in config['_self']:
         source = safe_join(repodir, config['_self']['*'])
         try:
             with io.open(source, 'rb') as handle:
                 sourcedata = handle.read()
         except IOError as e:
             if e.errno != errno.ENOENT:
@@ skipping 36 matching lines @@
     args = parser.parse_args()
 
     if args.quiet:
         logging.disable(logging.INFO)
 
     repos = args.repos
     if not len(repos):
         repos = [os.path.dirname(__file__)]
     for repo in repos:
         resolve_deps(repo)