From 7757c85d4f03619e1e8fe13086aaf84c61de1550 Mon Sep 17 00:00:00 2001 From: Jacob Beck Date: Mon, 5 Nov 2018 09:57:27 -0700 Subject: [PATCH 1/9] add some helpful exceptions about command running --- dbt/exceptions.py | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/dbt/exceptions.py b/dbt/exceptions.py index 3deb674e6bc..96a40f978e3 100644 --- a/dbt/exceptions.py +++ b/dbt/exceptions.py @@ -165,6 +165,45 @@ class FailedToConnectException(DatabaseException): pass +class CommandError(RuntimeException): + def __init__(self, cwd, cmd, message='Error running command'): + super(CommandError, self).__init__(message) + self.cwd = cwd + self.cmd = cmd + self.args = (cwd, cmd, message) + + def __str__(self): + if len(self.cmd) == 0: + return '{}: No arguments given'.format(self.msg) + return '{}: "{}"'.format(self.msg, self.cmd[0]) + + +class ExecutableError(CommandError): + def __init__(self, cwd, cmd, message): + super(ExecutableError, self).__init__(cwd, cmd, message) + + +class WorkingDirectoryError(CommandError): + def __init__(self, cwd, cmd, message): + super(WorkingDirectoryError, self).__init__(cwd, cmd, message) + + def __str__(self): + return '{}: "{}"'.format(self.msg, self.cwd) + + +class CommandResultError(CommandError): + def __init__(self, cwd, cmd, returncode, stdout, stderr, + message='Got a non-zero returncode'): + super(CommandResultError, self).__init__(cwd, cmd, message) + self.returncode = returncode + self.stdout = stdout + self.stderr = stderr + self.args = (cwd, cmd, returncode, stdout, stderr, message) + + def __str__(self): + return '{} running: {}'.format(self.msg, self.cmd) + + def raise_compiler_error(msg, node=None): raise CompilationException(msg, node) From 59b6f78c7111637a6fadcc8bf703408594ba31ae Mon Sep 17 00:00:00 2001 From: Jacob Beck Date: Mon, 5 Nov 2018 11:12:55 -0700 Subject: [PATCH 2/9] Raise an exception on rc!=0 in run_cmd, raise more specific exceptions about what went wrong on error --- dbt/clients/git.py | 31 +++++++++++------- dbt/clients/system.py | 76 +++++++++++++++++++++++++++++++++++++++---- 2 files changed, 90 insertions(+), 17 deletions(-) diff --git a/dbt/clients/git.py b/dbt/clients/git.py index 0acb72f45bf..73543f1e862 100644 --- a/dbt/clients/git.py +++ b/dbt/clients/git.py @@ -26,10 +26,7 @@ def list_tags(cwd): return tags -def checkout(cwd, repo, branch=None): - if branch is None: - branch = 'master' - +def _checkout(cwd, repo, branch): logger.debug(' Checking out branch {}.'.format(branch)) run_cmd(cwd, ['git', 'remote', 'set-branches', 'origin', branch]) @@ -44,12 +41,17 @@ def checkout(cwd, repo, branch=None): spec = 'origin/{}'.format(branch) out, err = run_cmd(cwd, ['git', 'reset', '--hard', spec]) - stderr = err.decode('utf-8').strip() + return out, err + - if stderr.startswith('fatal:'): +def checkout(cwd, repo, branch=None): + if branch is None: + branch = 'master' + try: + return _checkout(cwd, repo, branch) + except dbt.exceptions.CommandResultError as exc: + stderr = exc.stderr.decode('utf-8').strip() dbt.exceptions.bad_package_spec(repo, branch, stderr) - else: - return out, err def get_current_sha(cwd): @@ -64,9 +66,16 @@ def remove_remote(cwd): def clone_and_checkout(repo, cwd, dirname=None, remove_git_dir=False, branch=None): - _, err = clone(repo, cwd, dirname=dirname, remove_git_dir=remove_git_dir) - exists = re.match("fatal: destination path '(.+)' already exists", - err.decode('utf-8')) + exists = None + try: + _, err = clone(repo, cwd, dirname=dirname, + remove_git_dir=remove_git_dir) + except dbt.exceptions.CommandResultError as exc: + err = exc.stderr.decode('utf-8') + exists = re.match("fatal: destination path '(.+)' already exists", err) + if not exists: # something else is wrong, raise it + dbt.exceptions.bad_package_spec(repo, branch, err) + directory = None start_sha = None if exists: diff --git a/dbt/clients/system.py b/dbt/clients/system.py index 3ee2eee36e0..d38aea57d58 100644 --- a/dbt/clients/system.py +++ b/dbt/clients/system.py @@ -185,19 +185,83 @@ def open_dir_cmd(): return 'xdg-open' +def _handle_cwd_error(exc, cwd, cmd): + if exc.errno == errno.ENOENT: + message = 'Directory does not exist' + elif exc.errno == errno.EACCES: + message = 'Current user cannot access directory, check permissions' + elif exc.errno == errno.ENOTDIR: + message = 'Not a directory' + else: + message = 'Unknown OSError: {} - cwd'.format(str(exc)) + raise dbt.exceptions.WorkingDirectoryError(cwd, cmd, message) + + +def _handle_cmd_error(exc, cwd, cmd): + if exc.errno == errno.ENOENT: + message = "Could not find command, ensure it is in the user's PATH" + elif exc.errno == errno.EACCES: + message = 'User does not have permissions for this command' + else: + message = 'Unknown OSError: {} - cmd'.format(str(exc)) + raise dbt.exceptions.ExecutableError(cwd, cmd, message) + + +def _interpret_oserror(exc, cwd, cmd): + """Interpret an OSError exc and raise the appropriate dbt exception. + + Some things that could happen to trigger an OSError: + - cwd could not exist + - exc.errno == ENOENT + - exc.filename == cwd + - cwd could have permissions that prevent the current user moving to it + - exc.errno == EACCESS + - exc.filename == cwd + - cwd could exist but not be a directory + - exc.errno == ENOTDIR + - exc.filename == cwd + - cmd[0] could not exist + - exc.errno == ENOENT + - exc.filename == None(?) + - cmd[0] could exist but have permissions that prevents the current + user from executing it (executable bit not set for the user) + - exc.errno == EACCES + - exc.filename == None(?) + """ + if len(cmd) == 0: + raise dbt.exceptions.CommandError(cwd, cmd) + + # both of these functions raise unconditionally + if getattr(exc, 'filename', None) == cwd: + _handle_cwd_error(exc, cwd, cmd) + else: + _handle_cmd_error(exc, cwd, cmd) + + def run_cmd(cwd, cmd): logger.debug('Executing "{}"'.format(' '.join(cmd))) - proc = subprocess.Popen( - cmd, - cwd=cwd, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE) + if len(cmd) == 0: + raise dbt.exceptions.CommandError(cwd, cmd) - out, err = proc.communicate() + try: + proc = subprocess.Popen( + cmd, + cwd=cwd, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE) + + out, err = proc.communicate() + except OSError as exc: + _interpret_oserror(exc, cwd, cmd) logger.debug('STDOUT: "{}"'.format(out)) logger.debug('STDERR: "{}"'.format(err)) + if proc.returncode != 0: + logger.debug('command return code={}'.format(proc.returncode)) + raise dbt.exceptions.CommandResultError(cwd, cmd, proc.returncode, + out, err) + return out, err From ec466067b2f93675a029896a8d2d2d5b4034c492 Mon Sep 17 00:00:00 2001 From: Jacob Beck Date: Mon, 5 Nov 2018 11:13:14 -0700 Subject: [PATCH 3/9] Tests, fixup ci --- .circleci/config.yml | 53 +++++--------------- .dockerignore | 1 + Dockerfile | 10 ++-- dbt/clients/system.py | 46 ++++++++++++++--- test/integration/base.py | 2 +- test/setup_db.sh | 24 +++++++++ test/unit/test_system_client.py | 87 +++++++++++++++++++++++++++++++++ tox.ini | 20 ++++---- 8 files changed, 179 insertions(+), 64 deletions(-) create mode 100644 .dockerignore diff --git a/.circleci/config.yml b/.circleci/config.yml index 11186999dec..a1cfe63757c 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -1,111 +1,82 @@ version: 2 jobs: unit: - docker: &py36 - - image: python:3.6 + docker: &containers + - image: fishtownjacob/test-container - image: postgres + name: database environment: &pgenv POSTGRES_USER: "root" POSTGRES_PASSWORD: "password" POSTGRES_DB: "dbt" steps: - checkout - - run: apt-get update && apt-get install -y python-dev python3-dev postgresql - - run: echo 127.0.0.1 database | tee -a /etc/hosts - - run: pip install virtualenvwrapper tox - run: &setupdb name: Setup postgres command: bash test/setup_db.sh environment: - PGHOST: 127.0.0.1 + PGHOST: database PGUSER: root PGPASSWORD: password PGDATABASE: postgres - run: tox -e pep8,unit-py27,unit-py36 integration-postgres-py36: - docker: *py36 + docker: *containers steps: - checkout - - run: apt-get update && apt-get install -y python3-dev postgresql - - run: echo 127.0.0.1 database | tee -a /etc/hosts - - run: pip install virtualenvwrapper tox - run: *setupdb - run: name: Run tests command: tox -e integration-postgres-py36 integration-snowflake-py36: - docker: *py36 + docker: *containers steps: - checkout - - run: apt-get update && apt-get install -y python3-dev postgresql - - run: echo 127.0.0.1 database | tee -a /etc/hosts - - run: pip install virtualenvwrapper tox - run: name: Run tests command: tox -e integration-snowflake-py36 no_output_timeout: 1h integration-redshift-py36: - docker: *py36 + docker: *containers steps: - checkout - - run: apt-get update && apt-get install -y python3-dev postgresql - - run: echo 127.0.0.1 database | tee -a /etc/hosts - - run: pip install virtualenvwrapper tox - run: name: Run tests command: tox -e integration-redshift-py36 integration-bigquery-py36: - docker: *py36 + docker: *containers steps: - checkout - - run: apt-get update && apt-get install -y python3-dev postgresql - - run: echo 127.0.0.1 database | tee -a /etc/hosts - - run: pip install virtualenvwrapper tox - run: name: Run tests command: tox -e integration-bigquery-py36 integration-postgres-py27: - docker: &py27 - - image: python:2.7 - - image: postgres - environment: *pgenv + docker: *containers steps: - checkout - - run: apt-get update && apt-get install -y python-dev postgresql - - run: echo 127.0.0.1 database | tee -a /etc/hosts - - run: pip install virtualenvwrapper tox - run: *setupdb - run: name: Run tests command: tox -e integration-postgres-py27 integration-snowflake-py27: - docker: *py27 + docker: *containers steps: - checkout - - run: apt-get update && apt-get install -y python-dev postgresql - - run: echo 127.0.0.1 database | tee -a /etc/hosts - - run: pip install virtualenvwrapper tox - run: name: Run tests command: tox -e integration-snowflake-py27 no_output_timeout: 1h integration-redshift-py27: - docker: *py27 + docker: *containers steps: - checkout - - run: apt-get update && apt-get install -y python-dev postgresql - - run: echo 127.0.0.1 database | tee -a /etc/hosts - - run: pip install virtualenvwrapper tox - run: name: Run tests command: tox -e integration-redshift-py27 integration-bigquery-py27: - docker: *py27 + docker: *containers steps: - checkout - - run: apt-get update && apt-get install -y python-dev postgresql - - run: echo 127.0.0.1 database | tee -a /etc/hosts - - run: pip install virtualenvwrapper tox - run: name: Run tests command: tox -e integration-bigquery-py27 diff --git a/.dockerignore b/.dockerignore new file mode 100644 index 00000000000..72e8ffc0db8 --- /dev/null +++ b/.dockerignore @@ -0,0 +1 @@ +* diff --git a/Dockerfile b/Dockerfile index 410823589f3..874decacdf3 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,14 +1,16 @@ FROM python:3.6 -RUN apt-get update - -RUN apt-get install -y python-pip netcat -RUN apt-get install -y python-dev python3-dev +# do these on one line so changes trigger apt-get update +RUN apt-get update && \ + apt-get install -y python-pip netcat python-dev python3-dev postgresql RUN pip install pip --upgrade RUN pip install virtualenv RUN pip install virtualenvwrapper RUN pip install tox +RUN useradd -mU dbt_test_user +USER dbt_test_user + WORKDIR /usr/src/app RUN cd /usr/src/app diff --git a/dbt/clients/system.py b/dbt/clients/system.py index d38aea57d58..71a2c2d5c39 100644 --- a/dbt/clients/system.py +++ b/dbt/clients/system.py @@ -185,7 +185,7 @@ def open_dir_cmd(): return 'xdg-open' -def _handle_cwd_error(exc, cwd, cmd): +def _handle_posix_cwd_error(exc, cwd, cmd): if exc.errno == errno.ENOENT: message = 'Directory does not exist' elif exc.errno == errno.EACCES: @@ -197,7 +197,7 @@ def _handle_cwd_error(exc, cwd, cmd): raise dbt.exceptions.WorkingDirectoryError(cwd, cmd, message) -def _handle_cmd_error(exc, cwd, cmd): +def _handle_posix_cmd_error(exc, cwd, cmd): if exc.errno == errno.ENOENT: message = "Could not find command, ensure it is in the user's PATH" elif exc.errno == errno.EACCES: @@ -207,8 +207,8 @@ def _handle_cmd_error(exc, cwd, cmd): raise dbt.exceptions.ExecutableError(cwd, cmd, message) -def _interpret_oserror(exc, cwd, cmd): - """Interpret an OSError exc and raise the appropriate dbt exception. +def _handle_posix_error(exc, cwd, cmd): + """OSError handling for posix systems. Some things that could happen to trigger an OSError: - cwd could not exist @@ -227,15 +227,45 @@ def _interpret_oserror(exc, cwd, cmd): user from executing it (executable bit not set for the user) - exc.errno == EACCES - exc.filename == None(?) + """ + if getattr(exc, 'filename', None) == cwd: + _handle_posix_cwd_error(exc, cwd, cmd) + else: + _handle_posix_cmd_error(exc, cwd, cmd) + + +def _handle_windows_error(exc, cwd, cmd): + cls = dbt.exceptions.CommandError + if exc.errno == errno.ENOENT: + message = ("Could not find command, ensure it is in the user's PATH " + "and that the user has permissions to run it") + cls = dbt.exceptions.ExecutableError + elif exc.errno == errno.ENOTDIR: + message = ('Unable to cd: path does not exist, user does not have' + ' permissions, or not a directory') + cls = dbt.exceptions.WorkingDirectoryError + else: + message = 'Unknown error: {}'.format(str(exc)) + raise cls(cwd, cmd, message) + + +def _interpret_oserror(exc, cwd, cmd): + """Interpret an OSError exc and raise the appropriate dbt exception. + """ if len(cmd) == 0: raise dbt.exceptions.CommandError(cwd, cmd) - # both of these functions raise unconditionally - if getattr(exc, 'filename', None) == cwd: - _handle_cwd_error(exc, cwd, cmd) + # all of these functions raise unconditionally + if os.name == 'nt': + _handle_windows_error(exc, cwd, cmd) else: - _handle_cmd_error(exc, cwd, cmd) + _handle_posix_error(exc, cwd, cmd) + + # this should not be reachable, raise _something_ at least! + raise dbt.exceptions.InternalException( + 'Unhandled exception in _interpret_oserror: {}'.format(exc) + ) def run_cmd(cwd, cmd): diff --git a/test/integration/base.py b/test/integration/base.py index b1cd4704f92..019efc50b4b 100644 --- a/test/integration/base.py +++ b/test/integration/base.py @@ -21,7 +21,7 @@ DBT_CONFIG_DIR = os.path.abspath( - os.path.expanduser(os.environ.get("DBT_CONFIG_DIR", '/root/.dbt')) + os.path.expanduser(os.environ.get("DBT_CONFIG_DIR", '/home/dbt_test_user/.dbt')) ) DBT_PROFILES = os.path.join(DBT_CONFIG_DIR, 'profiles.yml') diff --git a/test/setup_db.sh b/test/setup_db.sh index d373b7195a0..65cfa2a61ac 100644 --- a/test/setup_db.sh +++ b/test/setup_db.sh @@ -1,11 +1,35 @@ #!/bin/bash set -x +env | grep '^PG' + # If you want to run this script for your own postgresql (run with # docker-compose) it will look like this: # PGHOST=127.0.0.1 PGUSER=root PGPASSWORD=password PGDATABASE=postgres \ # bash test/setup.sh PGUSER="${PGUSER:-postgres}" +export PGUSER +PGPORT="${PGPORT:-5432}" +export PGPORT +PGHOST="${PGHOST:-localhost}" + +function connect_circle() { + # try to handle circleci/docker oddness + let rc=1 + while [[ $rc -eq 1 ]]; do + nc -z ${PGHOST} ${PGPORT} + let rc=$? + done + if [[ $rc -ne 0 ]]; then + echo "Fatal: Could not connect to $PGHOST" + exit 1 + fi +} + +# appveyor doesn't have 'nc', but it also doesn't have these issues +if [[ -n $CIRCLECI ]]; then + connect_circle +fi createdb dbt psql -c "CREATE ROLE root WITH PASSWORD 'password';" diff --git a/test/unit/test_system_client.py b/test/unit/test_system_client.py index 2e87e7f5b50..282b957df63 100644 --- a/test/unit/test_system_client.py +++ b/test/unit/test_system_client.py @@ -1,6 +1,12 @@ import os +import shutil +import stat +import sys import unittest +from tempfile import mkdtemp +from dbt.exceptions import ExecutableError, WorkingDirectoryError, \ + CommandResultError import dbt.clients.system if os.name == 'nt': @@ -45,3 +51,84 @@ def test__make_file_with_overwrite(self): self.assertTrue(written) self.assertEqual(self.get_profile_text(), 'NEW_TEXT') + + +class TestRunCmd(unittest.TestCase): + """Test `run_cmd`. + + Don't mock out subprocess, in order to expose any OS-level differences. + """ + not_a_file = 'zzzbbfasdfasdfsdaq' + def setUp(self): + self.tempdir = mkdtemp() + self.run_dir = os.path.join(self.tempdir, 'run_dir') + self.does_not_exist = os.path.join(self.tempdir, 'does_not_exist') + self.empty_file = os.path.join(self.tempdir, 'empty_file') + if os.name == 'nt': + self.exists_cmd = ['cmd', '/C', 'echo', 'hello'] + else: + self.exists_cmd = ['echo', 'hello'] + + + os.mkdir(self.run_dir) + with open(self.empty_file, 'w') as fp: + pass # "touch" + + def tearDown(self): + shutil.rmtree(self.tempdir) + + def test__executable_does_not_exist(self): + with self.assertRaises(ExecutableError) as exc: + dbt.clients.system.run_cmd(self.run_dir, [self.does_not_exist]) + + msg = str(exc.exception).lower() + + self.assertIn('path', msg) + self.assertIn('could not find', msg) + self.assertIn(self.does_not_exist.lower(), msg) + + def test__not_exe(self): + with self.assertRaises(ExecutableError) as exc: + dbt.clients.system.run_cmd(self.run_dir, [self.empty_file]) + + msg = str(exc.exception).lower() + self.assertIn('permissions', msg) + self.assertIn(self.empty_file.lower(), msg) + + def test__cwd_does_not_exist(self): + with self.assertRaises(WorkingDirectoryError) as exc: + dbt.clients.system.run_cmd(self.does_not_exist, self.exists_cmd) + msg = str(exc.exception).lower() + self.assertIn('does not exist', msg) + self.assertIn(self.does_not_exist.lower(), msg) + + def test__cwd_not_directory(self): + with self.assertRaises(WorkingDirectoryError) as exc: + dbt.clients.system.run_cmd(self.empty_file, self.exists_cmd) + + msg = str(exc.exception).lower() + self.assertIn('not a directory', msg) + self.assertIn(self.empty_file.lower(), msg) + + def test__cwd_no_permissions(self): + # it would be nice to add a windows test. Possible path to that is via + # `psexec` (to get SYSTEM privs), use `icacls` to set permissions on + # the directory for the test user. I'm pretty sure windows users can't + # create files that they themselves cannot access. + if os.name == 'nt': + return + + # read-only -> cannot cd to it + os.chmod(self.run_dir, stat.S_IRUSR) + + with self.assertRaises(WorkingDirectoryError) as exc: + dbt.clients.system.run_cmd(self.run_dir, self.exists_cmd) + + msg = str(exc.exception).lower() + self.assertIn('permissions', msg) + self.assertIn(self.run_dir.lower(), msg) + + def test__ok(self): + out, err = dbt.clients.system.run_cmd(self.run_dir, self.exists_cmd) + self.assertEqual(out.strip(), b'hello') + self.assertEqual(err.strip(), b'') diff --git a/tox.ini b/tox.ini index 357ca429403..8e3b0243ac4 100644 --- a/tox.ini +++ b/tox.ini @@ -27,7 +27,7 @@ deps = basepython = python2.7 passenv = * setenv = - HOME=/root/ + HOME=/home/dbt_test_user commands = /bin/bash -c '{envpython} $(which nosetests) -v -a type=postgres {posargs} --with-coverage --cover-branches --cover-html --cover-html-dir=htmlcov test/integration/*' deps = -r{toxinidir}/requirements.txt @@ -37,7 +37,7 @@ deps = basepython = python2.7 passenv = * setenv = - HOME=/root/ + HOME=/home/dbt_test_user commands = /bin/bash -c '{envpython} $(which nosetests) -v -a type=snowflake {posargs} --with-coverage --cover-branches --cover-html --cover-html-dir=htmlcov test/integration/*' deps = -r{toxinidir}/requirements.txt @@ -47,7 +47,7 @@ deps = basepython = python2.7 passenv = * setenv = - HOME=/root/ + HOME=/home/dbt_test_user commands = /bin/bash -c '{envpython} $(which nosetests) -v -a type=bigquery {posargs} --with-coverage --cover-branches --cover-html --cover-html-dir=htmlcov test/integration/*' deps = -r{toxinidir}/requirements.txt @@ -57,7 +57,7 @@ deps = basepython = python2.7 passenv = * setenv = - HOME=/root/ + HOME=/home/dbt_test_user commands = /bin/bash -c '{envpython} $(which nosetests) -v -a type=redshift {posargs} --with-coverage --cover-branches --cover-html --cover-html-dir=htmlcov test/integration/*' deps = -r{toxinidir}/requirements.txt @@ -67,7 +67,7 @@ deps = basepython = python3.6 passenv = * setenv = - HOME=/root/ + HOME=/home/dbt_test_user commands = /bin/bash -c '{envpython} $(which nosetests) -v -a type=postgres --with-coverage --cover-branches --cover-html --cover-html-dir=htmlcov {posargs} test/integration/*' deps = -r{toxinidir}/requirements.txt @@ -77,7 +77,7 @@ deps = basepython = python3.6 passenv = * setenv = - HOME=/root/ + HOME=/home/dbt_test_user commands = /bin/bash -c '{envpython} $(which nosetests) -v -a type=snowflake {posargs} --with-coverage --cover-branches --cover-html --cover-html-dir=htmlcov test/integration/*' deps = -r{toxinidir}/requirements.txt @@ -87,7 +87,7 @@ deps = basepython = python3.6 passenv = * setenv = - HOME=/root/ + HOME=/home/dbt_test_user commands = /bin/bash -c '{envpython} $(which nosetests) -v -a type=bigquery {posargs} --with-coverage --cover-branches --cover-html --cover-html-dir=htmlcov test/integration/*' deps = -r{toxinidir}/requirements.txt @@ -97,7 +97,7 @@ deps = basepython = python3.6 passenv = * setenv = - HOME=/root/ + HOME=/home/dbt_test_user commands = /bin/bash -c '{envpython} $(which nosetests) -v -a type=redshift {posargs} --with-coverage --cover-branches --cover-html --cover-html-dir=htmlcov test/integration/*' deps = -r{toxinidir}/requirements.txt @@ -107,7 +107,7 @@ deps = basepython = python2.7 passenv = * setenv = - HOME=/root/ + HOME=/home/dbt_test_user commands = /bin/bash -c '{envpython} $(which nosetests) -v {posargs}' deps = -r{toxinidir}/requirements.txt @@ -117,7 +117,7 @@ deps = basepython = python3.6 passenv = * setenv = - HOME=/root/ + HOME=/home/dbt_test_user commands = /bin/bash -c '{envpython} $(which nosetests) -v {posargs}' deps = -r{toxinidir}/requirements.txt From e866caa900512ba88aaf4cd7675936f20b546812 Mon Sep 17 00:00:00 2001 From: Jacob Beck Date: Mon, 5 Nov 2018 14:52:32 -0700 Subject: [PATCH 4/9] add support for cross-drive moves --- dbt/clients/system.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbt/clients/system.py b/dbt/clients/system.py index 71a2c2d5c39..e633d53924f 100644 --- a/dbt/clients/system.py +++ b/dbt/clients/system.py @@ -311,7 +311,7 @@ def rename(from_path, to_path, force=False): else: rmdir(to_path) - os.rename(from_path, to_path) + shutil.move(from_path, to_path) def untar_package(tar_path, dest_dir, rename_to=None): From 531d7c687ec094ce56871c08d17cd68ca4a47bfd Mon Sep 17 00:00:00 2001 From: Jacob Beck Date: Mon, 5 Nov 2018 15:11:03 -0700 Subject: [PATCH 5/9] use environment variables or per-run temp directories to assign the temporary downloads directory --- dbt/clients/git.py | 4 ++-- dbt/task/deps.py | 34 ++++++++++++++++++++++++++++++++-- 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/dbt/clients/git.py b/dbt/clients/git.py index 73543f1e862..bd682410f0c 100644 --- a/dbt/clients/git.py +++ b/dbt/clients/git.py @@ -51,7 +51,7 @@ def checkout(cwd, repo, branch=None): return _checkout(cwd, repo, branch) except dbt.exceptions.CommandResultError as exc: stderr = exc.stderr.decode('utf-8').strip() - dbt.exceptions.bad_package_spec(repo, branch, stderr) + dbt.exceptions.bad_package_spec(repo, branch, stderr) def get_current_sha(cwd): @@ -74,7 +74,7 @@ def clone_and_checkout(repo, cwd, dirname=None, remove_git_dir=False, err = exc.stderr.decode('utf-8') exists = re.match("fatal: destination path '(.+)' already exists", err) if not exists: # something else is wrong, raise it - dbt.exceptions.bad_package_spec(repo, branch, err) + raise directory = None start_sha = None diff --git a/dbt/task/deps.py b/dbt/task/deps.py index 9f5daba2dde..78e5b356798 100644 --- a/dbt/task/deps.py +++ b/dbt/task/deps.py @@ -21,7 +21,24 @@ from dbt.task.base_task import BaseTask -DOWNLOADS_PATH = os.path.join(tempfile.gettempdir(), "dbt-downloads") +DOWNLOADS_PATH = None +REMOVE_DOWNLOADS = False + + +def _initialize_downloads(): + global DOWNLOADS_PATH, REMOVE_DOWNLOADS + # the user might have set an environment variable. Set it to None, and do + # not remove it when finished. + if DOWNLOADS_PATH is None: + DOWNLOADS_PATH = os.environ.get('DBT_DOWNLOADS_DIR', None) + REMOVE_DOWNLOADS = False + # if we are making a per-run temp directory, remove it at the end of + # successful runs + if DOWNLOADS_PATH is None: + DOWNLOADS_PATH = tempfile.mkdtemp(prefix='dbt-downloads-') + REMOVE_DOWNLOADS = True + + dbt.clients.system.make_directory(DOWNLOADS_PATH) class Package(APIObject): @@ -396,6 +413,16 @@ def _read_packages(project_yaml): class DepsTask(BaseTask): + def __init__(self, args, config=None): + super(DepsTask, self).__init__(args=args, config=config) + self._downloads_path = None + + @property + def downloads_path(self): + if self._downloads_path is None: + self._downloads_path = tempfile.mkdtemp(prefix='dbt-downloads') + return self._downloads_path + def _check_for_duplicate_project_names(self, final_deps): seen = set() for _, package in final_deps.items(): @@ -421,7 +448,7 @@ def track_package_install(self, package_name, source_type, version): def run(self): dbt.clients.system.make_directory(self.config.modules_path) - dbt.clients.system.make_directory(DOWNLOADS_PATH) + _initialize_downloads() packages = self.config.packages.packages if not packages: @@ -451,3 +478,6 @@ def run(self): package_name=package.name, source_type=package.source_type(), version=package.version_name()) + + if REMOVE_DOWNLOADS: + shutil.rmtree(DOWNLOADS_PATH) From 412b165dc9d97facbedd97f7a1cbe8b1d1d07fc3 Mon Sep 17 00:00:00 2001 From: Jacob Beck Date: Tue, 6 Nov 2018 16:18:27 -0700 Subject: [PATCH 6/9] fix issue where we incorrectly logged stack traces --- dbt/logger.py | 3 +++ dbt/main.py | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/dbt/logger.py b/dbt/logger.py index 11f19ec0b07..e60270b5536 100644 --- a/dbt/logger.py +++ b/dbt/logger.py @@ -112,4 +112,7 @@ def initialize_logger(debug_mode=False, path=None): initialized = True +def logger_initialized(): + return initialized + GLOBAL_LOGGER = logger diff --git a/dbt/main.py b/dbt/main.py index 891edb6dde7..3a4676c7059 100644 --- a/dbt/main.py +++ b/dbt/main.py @@ -1,5 +1,5 @@ from dbt.logger import initialize_logger, GLOBAL_LOGGER as logger, \ - initialized as logger_initialized + logger_initialized import argparse import os.path @@ -91,7 +91,7 @@ def main(args=None): logger.info("Encountered an error:") logger.info(str(e)) - if logger_initialized: + if logger_initialized(): logger.debug(traceback.format_exc()) elif not isinstance(e, RuntimeException): # if it did not come from dbt proper and the logger is not From 37738430949af4119d5b9e40654e8f6cb7f578be Mon Sep 17 00:00:00 2001 From: Jacob Beck Date: Tue, 13 Nov 2018 08:02:08 -0700 Subject: [PATCH 7/9] in deps, when "git" is not in the path, link users to help docs --- dbt/task/deps.py | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/dbt/task/deps.py b/dbt/task/deps.py index 78e5b356798..1b17e0d5b90 100644 --- a/dbt/task/deps.py +++ b/dbt/task/deps.py @@ -7,6 +7,7 @@ import dbt.utils import dbt.deprecations +import dbt.exceptions import dbt.clients.git import dbt.clients.system import dbt.clients.registry as registry @@ -247,9 +248,18 @@ def _checkout(self, project): if len(self.version) != 1: dbt.exceptions.raise_dependency_error( 'Cannot checkout repository until the version is pinned.') - dir_ = dbt.clients.git.clone_and_checkout( - self.git, DOWNLOADS_PATH, branch=self.version[0], - dirname=self._checkout_name) + try: + dir_ = dbt.clients.git.clone_and_checkout( + self.git, DOWNLOADS_PATH, branch=self.version[0], + dirname=self._checkout_name) + except dbt.exceptions.ExecutableError as exc: + if exc.cmd and exc.cmd[0] == 'git': + logger.error( + 'Make sure git is installed on your machine. More ' + 'information: ' + 'https://docs.getdbt.com/docs/package-management' + ) + raise return os.path.join(DOWNLOADS_PATH, dir_) def _fetch_metadata(self, project): From d35e549dbf06211b74aa2ca81306cf8b69c029a4 Mon Sep 17 00:00:00 2001 From: Jacob Beck Date: Wed, 14 Nov 2018 10:37:23 -0700 Subject: [PATCH 8/9] Handle cross-drive windows permissions issues by undoing git's readonly settings --- dbt/clients/system.py | 65 ++++++++++++++++++++++++++++++++++++++++++- dbt/task/deps.py | 4 +-- 2 files changed, 66 insertions(+), 3 deletions(-) diff --git a/dbt/clients/system.py b/dbt/clients/system.py index e633d53924f..8973f54781f 100644 --- a/dbt/clients/system.py +++ b/dbt/clients/system.py @@ -215,7 +215,7 @@ def _handle_posix_error(exc, cwd, cmd): - exc.errno == ENOENT - exc.filename == cwd - cwd could have permissions that prevent the current user moving to it - - exc.errno == EACCESS + - exc.errno == EACCES - exc.filename == cwd - cwd could exist but not be a directory - exc.errno == ENOTDIR @@ -323,3 +323,66 @@ def untar_package(tar_path, dest_dir, rename_to=None): downloaded_path = os.path.join(dest_dir, tar_dir_name) desired_path = os.path.join(dest_dir, rename_to) dbt.clients.system.rename(downloaded_path, desired_path, force=True) + + +def chmod_and_retry(func, path, exc_info): + """Define an error handler to pass to shutil.rmtree. + On Windows, when a file is marked read-only as git likes to do, rmtree will + fail. To handle that, on errors try to make the file writable. + We want to retry most operations here, but listdir is one that we know will + be useless. + """ + if func is os.listdir or os.name != 'nt': + raise + os.chmod(path, stat.S_IREAD | stat.S_IWRITE) + # on error,this will raise. + func(path) + + +def _absnorm(path): + return os.path.normcase(os.path.abspath(path)) + + +def move(src, dst): + """A re-implementation of shutil.move that properly removes the source + directory on windows when it has read-only files in it and the move is + between two drives. + + This is almost identical to the real shutil.move, except it uses our rmtree + and skips handling non-windows OSes since the existing one works ok there. + """ + if os.name != 'nt': + return shutil.move(src, dst) + + if os.path.isdir(dst): + if _absnorm(src) == _absnorm(dst): + os.rename(src, dst) + return + + dst = os.path.join(dst, os.path.basename(src.rstrip('/\\'))) + if os.path.exists(dst): + raise EnvironmentError("Path '{}' already exists".format(dst)) + + try: + os.rename(src, dst) + except OSError: + # probably different drives + if os.path.isdir(src): + if _absnorm(dst+'\\').startswith(_absnorm(src+'\\')): + # dst is inside src + raise EnvironmentError( + "Cannot move a directory '{}' into itself '{}'" + .format(src, dst) + ) + shutil.copytree(src, dst, symlinks=True) + rmtree(src) + else: + shutil.copy2(src, dst) + os.unlink(src) + + +def rmtree(path): + """Recursively remove path. On permissions errors on windows, try to remove + the read-only flag and try again. + """ + return shutil.rmtree(path, onerror=chmod_and_retry) diff --git a/dbt/task/deps.py b/dbt/task/deps.py index 1b17e0d5b90..034faee8cda 100644 --- a/dbt/task/deps.py +++ b/dbt/task/deps.py @@ -273,7 +273,7 @@ def install(self, project): dbt.clients.system.remove_file(dest_path) else: dbt.clients.system.rmdir(dest_path) - shutil.move(self._checkout(project), dest_path) + dbt.clients.system.move(self._checkout(project), dest_path) class LocalPackage(Package): @@ -490,4 +490,4 @@ def run(self): version=package.version_name()) if REMOVE_DOWNLOADS: - shutil.rmtree(DOWNLOADS_PATH) + dbt.clients.system.rmtree(DOWNLOADS_PATH) From 8840996a3086dde9453ac5e3ba18a38168fe941a Mon Sep 17 00:00:00 2001 From: Jacob Beck Date: Wed, 14 Nov 2018 11:30:17 -0700 Subject: [PATCH 9/9] user feedback: log download directory in dbt deps --- dbt/task/deps.py | 1 + 1 file changed, 1 insertion(+) diff --git a/dbt/task/deps.py b/dbt/task/deps.py index 034faee8cda..9e7b5f3b4ee 100644 --- a/dbt/task/deps.py +++ b/dbt/task/deps.py @@ -40,6 +40,7 @@ def _initialize_downloads(): REMOVE_DOWNLOADS = True dbt.clients.system.make_directory(DOWNLOADS_PATH) + logger.debug("Set downloads directory='{}'".format(DOWNLOADS_PATH)) class Package(APIObject):