fix(nodeenv): stop -p from reinstalling node that is already there

CHANGES

@@ -12,6 +12,10 @@ Version [unreleased]
 - Added `--isolate-npm` to keep npm cache, userconfig and init-module inside
   the environment `#154 <https://github.com/ekalinin/nodeenv/issues/154>`_
 - Added tests that run the activation scripts in sh, dash, bash, zsh and fish.
+- `-p` no longer reinstalls node when the requested version is already in the
+  virtualenv `#159 <https://github.com/ekalinin/nodeenv/issues/159>`_
+- Repeated `-p` runs no longer duplicate the `predeactivate` hook
+  `#159 <https://github.com/ekalinin/nodeenv/issues/159>`_
 
 Version 1.3.1
 -------------

README.rst

@@ -277,7 +277,8 @@ Basic options
     Lists available node.js versions.
 
 ``-p, --python-virtualenv``
-    Use current python virtualenv.
+    Use current python virtualenv. Running it again with the same node
+    version does not reinstall node; pass ``--force`` to reinstall.
 
 ``-r FILENAME, --requirements=FILENAME``
     Install all the packages listed in the given requirements file.
@@ -286,7 +287,8 @@ Basic options
     Provides an alternative prompt prefix for this environment.
 
 ``--force``
-    Force installation in a pre-existing directory.
+    Force installation in a pre-existing directory, and reinstall node even
+    when the requested version is already installed.
 
 ``--update``
     Install npm packages from file without reinstalling node.

nodeenv.py

@@ -378,6 +378,28 @@ def node_version_from_args(args):
     return parse_version(args.node)
 
 
+def get_installed_node_version(env_dir):
+    """
+    Return version of node installed in env_dir, None if there is none
+    """
+    bin_dir = join(env_dir, 'Scripts' if is_WIN else 'bin')
+    node_bin = join(bin_dir, 'node.exe' if is_WIN else 'node')
+    if not os.path.exists(node_bin):
+        return None
+
+    with open(node_bin, 'rb') as f:
+        # a shim runs the system node, it is not an installed one
+        if f.read(2) == b'#!':
+            return None
+
+    try:
+        out, _ = subprocess.Popen(
+            [node_bin, "--version"], stdout=subprocess.PIPE).communicate()
+        return parse_version(clear_output(out))
+    except (OSError, ValueError):
+        return None
+
+
 def create_logger():
     """
     Create logger for diagnostic
@@ -1219,14 +1241,14 @@ def set_predeactivate_hook(env_dir):
     if is_WIN:
         # Windows: create predeactivate.bat for CMD and
         #           predeactivate.ps1 for PowerShell
-        with open(join(env_dir, 'Scripts', 'predeactivate.bat'), 'a') as hook:
-            hook.write(PREDEACTIVATE_BAT)
-        with open(join(env_dir, 'Scripts', 'predeactivate.ps1'), 'a') as hook:
-            hook.write(PREDEACTIVATE_PS1)
+        writefile(join(env_dir, 'Scripts', 'predeactivate.bat'),
+                  PREDEACTIVATE_BAT, append=True)
+        writefile(join(env_dir, 'Scripts', 'predeactivate.ps1'),
+                  PREDEACTIVATE_PS1, append=True)
     else:
         # Unix: create predeactivate for bash/sh
-        with open(join(env_dir, 'bin', 'predeactivate'), 'a') as hook:
-            hook.write(PREDEACTIVATE_SH)
+        writefile(join(env_dir, 'bin', 'predeactivate'),
+                  PREDEACTIVATE_SH, append=True)
 
 
 def create_environment(env_dir, args):
@@ -1240,12 +1262,16 @@ def create_environment(env_dir, args):
     src_dir = to_utf8(abspath(join(env_dir, 'src')))
     mkdir(src_dir)
 
-    if args.node != "system":
-        install_node(env_dir, src_dir, args)
-    else:
+    if args.node == "system":
         mkdir(join(env_dir, 'bin'))
         mkdir(join(env_dir, 'lib'))
         mkdir(join(env_dir, 'lib', 'node_modules'))
+    elif not args.force and \
+            get_installed_node_version(env_dir) == parse_version(args.node):
+        logger.info(' * Node.js %s is already installed, skipping '
+                    '(use --force to reinstall)', args.node)
+    else:
+        install_node(env_dir, src_dir, args)
     # activate script install must be
     # before npm install, npm use activate
     # for install

tests/nodeenv_test.py

@@ -153,6 +153,83 @@ def test_predeactivate_hook(tmpdir):
         assert 'deactivate_node' in p.read()
 
 
+def test_predeactivate_hook_is_idempotent(tmpdir):
+    if nodeenv.is_WIN:
+        tmpdir.mkdir('Scripts')
+        nodeenv.set_predeactivate_hook(tmpdir.strpath)
+        nodeenv.set_predeactivate_hook(tmpdir.strpath)
+        p_bat = tmpdir.join('Scripts').join('predeactivate.bat')
+        assert p_bat.read() == nodeenv.PREDEACTIVATE_BAT
+        p_ps1 = tmpdir.join('Scripts').join('predeactivate.ps1')
+        assert p_ps1.read() == nodeenv.PREDEACTIVATE_PS1
+    else:
+        tmpdir.mkdir('bin')
+        nodeenv.set_predeactivate_hook(tmpdir.strpath)
+        nodeenv.set_predeactivate_hook(tmpdir.strpath)
+        p = tmpdir.join('bin').join('predeactivate')
+        assert p.read() == nodeenv.PREDEACTIVATE_SH
+
+
+def _node_bin(tmpdir):
+    if nodeenv.is_WIN:
+        return tmpdir.mkdir('Scripts').join('node.exe')
+    return tmpdir.mkdir('bin').join('node')
+
+
+def test_get_installed_node_version_missing(tmpdir):
+    assert nodeenv.get_installed_node_version(str(tmpdir)) is None
+
+
+def test_get_installed_node_version_shim(tmpdir):
+    _node_bin(tmpdir).write(nodeenv.SHIM)
+    assert nodeenv.get_installed_node_version(str(tmpdir)) is None
+
+
+def test_get_installed_node_version_binary(tmpdir):
+    _node_bin(tmpdir).write_binary(b'\x7fELF fake node binary')
+    proc = mock.Mock()
+    proc.communicate.return_value = (b'v26.9.0\n', b'')
+    with mock.patch.object(nodeenv.subprocess, 'Popen', return_value=proc):
+        assert nodeenv.get_installed_node_version(str(tmpdir)) == (26, 9, 0)
+
+
+def _make_opts(extra):
+    with mock.patch.object(sys, 'argv', ['nodeenv'] + extra):
+        return nodeenv.parse_args()
+
+
+def _count_install_node(tmpdir, opts, installed):
+    with mock.patch.object(nodeenv, 'install_node') as install_node, \
+            mock.patch.object(nodeenv, 'install_activate'), \
+            mock.patch.object(nodeenv, 'install_npm'), \
+            mock.patch.object(nodeenv, 'install_npm_win'), \
+            mock.patch.object(nodeenv, 'set_predeactivate_hook'), \
+            mock.patch.object(nodeenv, 'get_installed_node_version',
+                              return_value=installed):
+        nodeenv.create_environment(str(tmpdir), opts)
+    return install_node.call_count
+
+
+def test_create_environment_skips_installed_node(tmpdir):
+    opts = _make_opts(['--node', '26.9.0', '-p'])
+    assert _count_install_node(tmpdir, opts, (26, 9, 0)) == 0
+
+
+def test_create_environment_installs_other_version(tmpdir):
+    opts = _make_opts(['--node', '26.9.0', '-p'])
+    assert _count_install_node(tmpdir, opts, (24, 0, 0)) == 1
+
+
+def test_create_environment_installs_when_absent(tmpdir):
+    opts = _make_opts(['--node', '26.9.0', '-p'])
+    assert _count_install_node(tmpdir, opts, None) == 1
+
+
+def test_create_environment_force_reinstalls_node(tmpdir):
+    opts = _make_opts(['--node', '26.9.0', '-p', '--force'])
+    assert _count_install_node(tmpdir, opts, (26, 9, 0)) == 1
+
+
 def test_mirror_option():
     urls = [('https://npm.taobao.org/mirrors/node',
              'https://npm.taobao.org/mirrors/node/index.json'),