Merge pull request #3104 from efiop/ssh_dir_out

tests: remove redundant native ssh tests

dvc/remote/ssh/connection.py

@@ -156,14 +156,12 @@ class SSHConnection:
     def _remove_dir(self, path):
         for root, dirs, files in self.walk(path, topdown=False):
             for fname in files:
-                path = posixpath.join(root, fname)
                 with suppress(FileNotFoundError):
-                    self._remove_file(path)
+                    self._remove_file(posixpath.join(root, fname))
 
             for dname in dirs:
-                path = posixpath.join(root, dname)
                 with suppress(FileNotFoundError):
-                    self.sftp.rmdir(dname)
+                    self.sftp.rmdir(posixpath.join(root, dname))
 
         with suppress(FileNotFoundError):
             self.sftp.rmdir(path)

scripts/build-requirements.txt

@@ -0,0 +1,2 @@
+PyInstaller==3.5
+psutil

scripts/build_posix.sh

@@ -95,10 +95,8 @@ install_dependencies()
 	pip install --upgrade pip
 
 	print_info "Installing requirements..."
-	pip install .[all] psutil
-
-	print_info "Installing pyinstaller..."
-	pip install pyinstaller
+	pip install .[all]
+	pip install -r scripts/build-requirements.txt
 }
 
 build_dvc()

scripts/build_windows.cmd

@@ -23,9 +23,8 @@ if %errorlevel% neq 0 (echo Error: Couldn't find Inno Setup compiler. && goto :e
 echo ====== Installing requirements... ======
 echo PKG = "exe" > dvc\utils\build.py
 call pip install .[all] || goto :error
-call pip install psutil || goto :error
+call pip install -r scripts\build-requirements.txt || goto :error
 call dvc pull || goto :error
-call pip install pyinstaller || goto :error
 
 echo ====== Building dvc binary... ======
 call pyinstaller --additional-hooks-dir scripts\hooks dvc/__main__.py --name dvc --specpath build

scripts/hooks/hook-Crypto.py

@@ -1,58 +0,0 @@
-# -----------------------------------------------------------------------------
-# Copyright (c) 2005-2019, PyInstaller Development Team.
-#
-# Distributed under the terms of the GNU General Public License with exception
-# for distributing bootloader.
-#
-# The full license is in the file COPYING.txt, distributed with this software.
-# -----------------------------------------------------------------------------
-"""
-Hook for PyCryptodome library: https://pypi.python.org/pypi/pycryptodome
-
-PyCryptodome is an almost drop-in replacement for the now unmaintained
-PyCrypto library. The two are mutually exclusive as they live under
-the same package ("Crypto").
-
-PyCryptodome distributes dynamic libraries and builds them as if they were
-Python C extensions (even though they are not extensions - as they can't be
-imported by Python). It might sound a bit weird, but this decision is rooted
-in PyPy and its partial and slow support for C extensions. However, this also
-invalidates several of the existing methods used by PyInstaller to decide the
-right files to pull in.
-
-Even though this hook is meant to help with PyCryptodome only, it will be
-triggered also when PyCrypto is installed, so it must be tested with both.
-
-Tested with PyCryptodome 3.5.1, PyCrypto 2.6.1, Python 2.7 & 3.6, Fedora &
-Windows
-"""
-import glob
-import os
-
-from PyInstaller.compat import EXTENSION_SUFFIXES
-from PyInstaller.utils.hooks import get_module_file_attribute
-
-# Include the modules as binaries in a subfolder named like the package.
-# Cryptodome's loader expects to find them inside the package directory for
-# the main module. We cannot use hiddenimports because that would add the
-# modules outside the package.
-
-binaries = []
-binary_module_names = [
-    "Crypto.Math",  # First in the list
-    "Crypto.Cipher",
-    "Crypto.Util",
-    "Crypto.Hash",
-    "Crypto.Protocol",
-]
-
-try:
-    for module_name in binary_module_names:
-        m_dir = os.path.dirname(get_module_file_attribute(module_name))
-        for ext in EXTENSION_SUFFIXES:
-            module_bin = glob.glob(os.path.join(m_dir, "_*%s" % ext))
-            for f in module_bin:
-                binaries.append((f, module_name.replace(".", os.sep)))
-except ImportError:
-    # Do nothing for PyCrypto (Crypto.Math does not exist there)
-    pass

scripts/hooks/hook-aliyunsdkcore.py

@@ -1,3 +0,0 @@
-from PyInstaller.utils.hooks import collect_data_files
-
-datas = collect_data_files("aliyunsdkcore")

setup.py

@@ -99,7 +99,6 @@ all_remotes = gs + s3 + azure + ssh + oss + gdrive + hdfs
 
 # Extra dependecies to run tests
 tests_requirements = [
-    "PyInstaller==3.5",
     "wheel>=0.31.1",
     "pydot>=1.2.4",
     # Test requirements:

tests/func/test_data_cloud.py

@@ -33,7 +33,6 @@ from tests.utils import spy
 from tests.remotes import (
     _should_test_gcp,
     _should_test_hdfs,
-    _should_test_ssh,
     Azure,
     GDrive,
     S3,
@@ -47,7 +46,6 @@ from tests.remotes import (
     get_gcp_url,
     get_hdfs_url,
     get_local_url,
-    get_ssh_url,
     get_ssh_url_mocked,
 )
 
@@ -296,17 +294,6 @@ class TestRemoteLOCAL(TestDataCloudBase):
         self.assertTrue(os.path.isdir(self.dname))
 
 
-class TestRemoteSSH(TestDataCloudBase):
-    def _should_test(self):
-        return _should_test_ssh()
-
-    def _get_url(self):
-        return get_ssh_url()
-
-    def _get_cloud_class(self):
-        return RemoteSSH
-
-
 @pytest.mark.usefixtures("ssh_server")
 class TestRemoteSSHMocked(TestDataCloudBase):
     @pytest.fixture(autouse=True)
@@ -442,18 +429,6 @@ class TestRemoteLOCALCLI(TestDataCloudCLIBase):
         self._test_cloud(TEST_REMOTE)
 
 
-class TestRemoteSSHCLI(TestDataCloudCLIBase):
-    def _should_test(self):
-        return _should_test_ssh()
-
-    def _test(self):
-        url = get_ssh_url()
-
-        self.main(["remote", "add", TEST_REMOTE, url])
-
-        self._test_cloud(TEST_REMOTE)
-
-
 class TestRemoteHDFSCLI(TestDataCloudCLIBase):
     def _should_test(self):
         return _should_test_hdfs()

tests/func/test_repro.py

@@ -38,7 +38,7 @@ from tests.remotes import (
     _should_test_gcp,
     _should_test_hdfs,
     _should_test_ssh,
-    get_ssh_url,
+    get_ssh_url_mocked,
     S3,
     TEST_AWS_REPO_BUCKET,
     TEST_GCP_REPO_BUCKET,
@@ -1611,30 +1611,46 @@ class TestReproDownstream(TestDvc):
         assert evaluation[2].relpath == "E.dvc"
 
 
-def test_ssh_dir_out(tmp_dir, dvc):
-    if not _should_test_ssh():
-        pytest.skip()
-
+@pytest.mark.skipif(
+    os.name == "nt",
+    reason="external output scenario is not supported on Windows",
+)
+def test_ssh_dir_out(tmp_dir, dvc, ssh_server):
     tmp_dir.gen({"foo": "foo content"})
 
     # Set up remote and cache
-    remote_url = get_ssh_url()
-    assert main(["remote", "add", "upstream", remote_url]) == 0
+    user = ssh_server.test_creds["username"]
+    port = ssh_server.port
+    keyfile = ssh_server.test_creds["key_filename"]
 
-    cache_url = get_ssh_url()
+    remote_url = get_ssh_url_mocked(user, port)
+    assert main(["remote", "add", "upstream", remote_url]) == 0
+    assert main(["remote", "modify", "upstream", "keyfile", keyfile]) == 0
+
+    cache_url = get_ssh_url_mocked(user, port)
     assert main(["remote", "add", "sshcache", cache_url]) == 0
     assert main(["config", "cache.ssh", "sshcache"]) == 0
+    assert main(["remote", "modify", "sshcache", "keyfile", keyfile]) == 0
 
     # Recreating to reread configs
     repo = DvcRepo(dvc.root_dir)
 
+    # To avoid "WARNING: UNPROTECTED PRIVATE KEY FILE" from ssh
+    os.chmod(keyfile, 0o600)
+
+    (tmp_dir / "script.py").write_text(
+        "import sys, pathlib\n"
+        "path = pathlib.Path(sys.argv[1])\n"
+        "dir_out = path / 'dir-out'\n"
+        "dir_out.mkdir()\n"
+        "(dir_out / '1.txt').write_text('1')\n"
+        "(dir_out / '2.txt').write_text('2')\n"
+    )
+
     url_info = URLInfo(remote_url)
-    mkdir_cmd = "mkdir dir-out;cd dir-out;echo 1 > 1.txt; echo 2 > 2.txt"
     repo.run(
-        cmd="ssh {netloc} 'cd {path};{cmd}'".format(
-            netloc=url_info.netloc, path=url_info.path, cmd=mkdir_cmd
-        ),
-        outs=[(url_info / "dir-out").url],
+        cmd="python {} {}".format(tmp_dir / "script.py", url_info.path),
+        outs=["remote://upstream/dir-out"],
         deps=["foo"],  # add a fake dep to not consider this a callback
     )
 

tests/unit/remote/ssh/test_connection.py

@@ -35,6 +35,16 @@ def test_makedirs(tmp_path, ssh):
     assert os.path.isdir(path)
 
 
+def test_remove_dir(tmp_path, ssh):
+    dpath = tmp_path / "dir"
+    dpath.mkdir()
+    (dpath / "file").write_text("file")
+    (dpath / "subdir").mkdir()
+    (dpath / "subdir" / "subfile").write_text("subfile")
+    ssh.remove(dpath.absolute().as_posix())
+    assert not dpath.exists()
+
+
 def test_walk(tmp_path, ssh):
     root_path = tmp_path
     dir_path = root_path / "dir"