From 8154c89c3f5e306bf6a9368ae26faa2ac4d09eed Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Fri, 10 Jan 2020 20:53:59 +0200 Subject: [PATCH 1/6] tests: remove redundant native ssh tests We already have mocked ones, which are more reliable as they use their own paramiko server instead of relying on the sshd running on the test machine. Native tests are also quite flaky and fail sometimes due to network issues. --- tests/func/test_data_cloud.py | 25 ------------------------- 1 file changed, 25 deletions(-) diff --git a/tests/func/test_data_cloud.py b/tests/func/test_data_cloud.py index e69ffa9587..4270050633 100644 --- a/tests/func/test_data_cloud.py +++ b/tests/func/test_data_cloud.py @@ -33,7 +33,6 @@ from tests.remotes import ( _should_test_gcp, _should_test_hdfs, - _should_test_ssh, Azure, GDrive, S3, @@ -47,7 +46,6 @@ get_gcp_url, get_hdfs_url, get_local_url, - get_ssh_url, get_ssh_url_mocked, ) @@ -296,17 +294,6 @@ def test(self): 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 @@ def _test(self): 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() From f6a00f35ae0ea8a77a77a49f8e027eb269a9ea5d Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Sat, 11 Jan 2020 01:41:19 +0200 Subject: [PATCH 2/6] ssh: fix bug in remove() --- dvc/remote/ssh/connection.py | 6 ++---- tests/unit/remote/ssh/test_connection.py | 10 ++++++++++ 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/dvc/remote/ssh/connection.py b/dvc/remote/ssh/connection.py index 674bfc2c3b..ccfa5d7366 100644 --- a/dvc/remote/ssh/connection.py +++ b/dvc/remote/ssh/connection.py @@ -156,14 +156,12 @@ def _remove_file(self, path): 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) diff --git a/tests/unit/remote/ssh/test_connection.py b/tests/unit/remote/ssh/test_connection.py index 2b329b4241..3f79d9a49d 100644 --- a/tests/unit/remote/ssh/test_connection.py +++ b/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" From 2a9d19f11673f9b369fcc8625c5a951766b8cc9a Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Fri, 10 Jan 2020 23:29:38 +0200 Subject: [PATCH 3/6] tests: use mocked ssh --- tests/func/test_repro.py | 40 ++++++++++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 12 deletions(-) diff --git a/tests/func/test_repro.py b/tests/func/test_repro.py index 4d96457438..90450b4eb0 100644 --- a/tests/func/test_repro.py +++ b/tests/func/test_repro.py @@ -38,7 +38,7 @@ _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 @@ def test(self): 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() + user = ssh_server.test_creds["username"] + port = ssh_server.port + keyfile = ssh_server.test_creds["key_filename"] + + 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() + 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 ) From 795e4ce6b878c69f4b203a6e91a99cfeeabaab44 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Sat, 11 Jan 2020 04:30:43 +0200 Subject: [PATCH 4/6] pyinstaller: delete oss hook It is already available in the standard hooks. --- scripts/hooks/hook-aliyunsdkcore.py | 3 --- 1 file changed, 3 deletions(-) delete mode 100644 scripts/hooks/hook-aliyunsdkcore.py diff --git a/scripts/hooks/hook-aliyunsdkcore.py b/scripts/hooks/hook-aliyunsdkcore.py deleted file mode 100644 index 64f096d445..0000000000 --- a/scripts/hooks/hook-aliyunsdkcore.py +++ /dev/null @@ -1,3 +0,0 @@ -from PyInstaller.utils.hooks import collect_data_files - -datas = collect_data_files("aliyunsdkcore") From c064865c80f5ecdec66dcd2156219d482ab756b9 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Sat, 11 Jan 2020 03:53:17 +0200 Subject: [PATCH 5/6] build: use build-requirements.txt --- scripts/build-requirements.txt | 2 ++ scripts/build_posix.sh | 6 ++---- scripts/build_windows.cmd | 3 +-- setup.py | 1 - 4 files changed, 5 insertions(+), 7 deletions(-) create mode 100644 scripts/build-requirements.txt diff --git a/scripts/build-requirements.txt b/scripts/build-requirements.txt new file mode 100644 index 0000000000..45ad33c4d4 --- /dev/null +++ b/scripts/build-requirements.txt @@ -0,0 +1,2 @@ +PyInstaller==3.5 +psutil diff --git a/scripts/build_posix.sh b/scripts/build_posix.sh index cfe1bf611f..c5e9bb89f2 100755 --- a/scripts/build_posix.sh +++ b/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() diff --git a/scripts/build_windows.cmd b/scripts/build_windows.cmd index 90c513c83c..feca849b4d 100755 --- a/scripts/build_windows.cmd +++ b/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 diff --git a/setup.py b/setup.py index 9c86e09035..035253aaaf 100644 --- a/setup.py +++ b/setup.py @@ -99,7 +99,6 @@ def run(self): # Extra dependecies to run tests tests_requirements = [ - "PyInstaller==3.5", "wheel>=0.31.1", "pydot>=1.2.4", # Test requirements: From bc619f1c4148fb037d567146a728b06c5169d234 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Sat, 11 Jan 2020 04:47:27 +0200 Subject: [PATCH 6/6] pyinstaller: remove Crypto hook --- scripts/hooks/hook-Crypto.py | 58 ------------------------------------ 1 file changed, 58 deletions(-) delete mode 100644 scripts/hooks/hook-Crypto.py diff --git a/scripts/hooks/hook-Crypto.py b/scripts/hooks/hook-Crypto.py deleted file mode 100644 index 1be95a592d..0000000000 --- a/scripts/hooks/hook-Crypto.py +++ /dev/null @@ -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