diff --git a/dvc/command/run.py b/dvc/command/run.py index 8222cbcda..d16c9f7a2 100644 --- a/dvc/command/run.py +++ b/dvc/command/run.py @@ -1,6 +1,5 @@ from __future__ import unicode_literals -import os import argparse import dvc.logger as logger @@ -38,6 +37,7 @@ class CmdRun(CmdBase): deps=self.args.deps, fname=self.args.file, cwd=self.args.cwd, + wdir=self.args.wdir, no_exec=self.args.no_exec, overwrite=overwrite, ignore_build_cache=self.args.ignore_build_cache, @@ -128,11 +128,13 @@ def add_parser(subparsers, parent_parser): "a stage filename.", ) run_parser.add_argument( - "-c", - "--cwd", - default=os.path.curdir, - help="Directory within your repo to run your command and place " - "stage file in.", + "-c", "--cwd", default=None, help="Deprecated, use -w and -f instead." + ) + run_parser.add_argument( + "-w", + "--wdir", + default=None, + help="Directory within your repo to run your command in.", ) run_parser.add_argument( "--no-exec", diff --git a/dvc/exceptions.py b/dvc/exceptions.py index eff0ae227..b31eef8b5 100644 --- a/dvc/exceptions.py +++ b/dvc/exceptions.py @@ -67,9 +67,9 @@ class OutputNotFoundError(DvcException): ) -class WorkingDirectoryAsOutputError(DvcException): +class StagePathAsOutputError(DvcException): """Thrown if directory that stage is going to be saved in is specified as - an output of anothe stage. + an output of another stage. Args: cwd (str): path to the directory. @@ -77,16 +77,16 @@ class WorkingDirectoryAsOutputError(DvcException): output. """ - def __init__(self, cwd, fname): - assert isinstance(cwd, str) or isinstance(cwd, builtin_str) + def __init__(self, wdir, fname): + assert isinstance(wdir, str) or isinstance(wdir, builtin_str) assert isinstance(fname, str) or isinstance(fname, builtin_str) msg = ( "current working directory '{cwd}' is specified as an output in" " '{fname}'. Use another CWD to prevent any data removal.".format( - cwd=cwd, fname=fname + cwd=wdir, fname=fname ) ) - super(WorkingDirectoryAsOutputError, self).__init__(msg) + super(StagePathAsOutputError, self).__init__(msg) class CircularDependencyError(DvcException): diff --git a/dvc/output/local.py b/dvc/output/local.py index c330e09ea..cb501be69 100644 --- a/dvc/output/local.py +++ b/dvc/output/local.py @@ -28,7 +28,7 @@ class OutputLOCAL(OutputBase): if not os.path.isabs(p): p = self.remote.to_ospath(p) - p = os.path.join(stage.cwd, p) + p = os.path.join(stage.wdir, p) p = os.path.abspath(os.path.normpath(p)) self.path_info = {"scheme": "local", "path": p} @@ -58,7 +58,7 @@ class OutputLOCAL(OutputBase): ret = super(OutputLOCAL, self).dumpd() if self.is_local: path = self.remote.unixpath( - os.path.relpath(self.path, self.stage.cwd) + os.path.relpath(self.path, self.stage.wdir) ) else: path = self.url diff --git a/dvc/repo/__init__.py b/dvc/repo/__init__.py index 025bfd2ef..176bda17b 100644 --- a/dvc/repo/__init__.py +++ b/dvc/repo/__init__.py @@ -294,7 +294,7 @@ class Repo(object): import networkx as nx from dvc.exceptions import ( OutputDuplicationError, - WorkingDirectoryAsOutputError, + StagePathAsOutputError, ) G = nx.DiGraph() @@ -314,15 +314,10 @@ class Repo(object): outs.append(out) for stage in stages: + path_dir = os.path.dirname(stage.path) + os.sep for out in outs: - overlaps = stage.cwd == out.path or stage.cwd.startswith( - out.path + os.sep - ) - - if overlaps: - raise WorkingDirectoryAsOutputError( - stage.cwd, stage.relpath - ) + if path_dir.startswith(out.path + os.sep): + raise StagePathAsOutputError(stage.wdir, stage.relpath) # collect the whole DAG for stage in stages: diff --git a/dvc/repo/move.py b/dvc/repo/move.py index 39a4ad9c9..c186fbc1c 100644 --- a/dvc/repo/move.py +++ b/dvc/repo/move.py @@ -30,7 +30,7 @@ def move(self, from_path, to_path): import dvc.output as Output from dvc.stage import Stage - from_out = Output.loads_from(Stage(self, cwd=os.curdir), [from_path])[0] + from_out = Output.loads_from(Stage(self), [from_path])[0] to_path = _expand_target_path(from_path, to_path) @@ -52,7 +52,7 @@ def move(self, from_path, to_path): os.path.basename(to_path) + Stage.STAGE_FILE_SUFFIX, ) - stage.cwd = os.path.abspath( + stage.wdir = os.path.abspath( os.path.join(os.curdir, os.path.dirname(to_path)) ) diff --git a/dvc/repo/run.py b/dvc/repo/run.py index b782655cf..d2bfffc96 100644 --- a/dvc/repo/run.py +++ b/dvc/repo/run.py @@ -1,4 +1,4 @@ -import os +from __future__ import unicode_literals def run( @@ -10,7 +10,8 @@ def run( metrics=None, metrics_no_cache=None, fname=None, - cwd=os.curdir, + cwd=None, + wdir=None, no_exec=False, overwrite=False, ignore_build_cache=False, @@ -36,6 +37,7 @@ def run( fname=fname, cmd=cmd, cwd=cwd, + wdir=wdir, outs=outs, outs_no_cache=outs_no_cache, metrics=metrics, diff --git a/dvc/stage.py b/dvc/stage.py index feac49ffe..89cf7e850 100644 --- a/dvc/stage.py +++ b/dvc/stage.py @@ -61,10 +61,16 @@ class StageFileBadNameError(DvcException): super(StageFileBadNameError, self).__init__(msg) -class StageBadCwdError(DvcException): - def __init__(self, cwd): - msg = "stage cwd '{}' is outside of the current dvc repo" - super(StageBadCwdError, self).__init__(msg.format(cwd)) +class StagePathOutsideError(DvcException): + def __init__(self, path): + msg = "stage working or file path '{}' is outside of dvc repo" + super(StagePathOutsideError, self).__init__(msg.format(path)) + + +class StagePathNotFoundError(DvcException): + def __init__(self, path): + msg = "stage working or file path '{}' does not exist" + super(StagePathNotFoundError, self).__init__(msg.format(path)) class StageCommitError(DvcException): @@ -102,6 +108,7 @@ class Stage(object): PARAM_MD5 = "md5" PARAM_CMD = "cmd" + PARAM_WDIR = "wdir" PARAM_DEPS = "deps" PARAM_OUTS = "outs" PARAM_LOCKED = "locked" @@ -109,6 +116,7 @@ class Stage(object): SCHEMA = { Optional(PARAM_MD5): Or(str, None), Optional(PARAM_CMD): Or(str, None), + Optional(PARAM_WDIR): Or(str, None), Optional(PARAM_DEPS): Or(And(list, Schema([dependency.SCHEMA])), None), Optional(PARAM_OUTS): Or(And(list, Schema([output.SCHEMA])), None), Optional(PARAM_LOCKED): bool, @@ -119,7 +127,7 @@ class Stage(object): repo, path=None, cmd=None, - cwd=os.curdir, + wdir=os.curdir, deps=None, outs=None, md5=None, @@ -133,7 +141,7 @@ class Stage(object): self.repo = repo self.path = path self.cmd = cmd - self.cwd = cwd + self.wdir = wdir self.outs = outs self.deps = deps self.md5 = md5 @@ -290,12 +298,12 @@ class Stage(object): raise StageFileFormatError(fname, exc) @classmethod - def _stage_fname_cwd(cls, fname, cwd, outs, add): - if fname and cwd: - return (fname, cwd) + def _stage_fname(cls, fname, outs, add): + if fname: + return fname if not outs: - return (cls.STAGE_FILE, cwd if cwd else os.getcwd()) + return cls.STAGE_FILE out = outs[0] if out.scheme == "local": @@ -303,20 +311,23 @@ class Stage(object): else: path = posixpath - if not fname: - fname = path.basename(out.path) + cls.STAGE_FILE_SUFFIX + fname = path.basename(out.path) + cls.STAGE_FILE_SUFFIX - if not cwd or (add and out.is_local): - cwd = path.dirname(out.path) + if add and out.is_local: + fname = path.join(path.dirname(out.path), fname) - return (fname, cwd) + return fname @staticmethod - def _check_inside_repo(repo, cwd): + def _check_stage_path(repo, path): assert repo is not None - proj_dir = os.path.realpath(repo.root_dir) - if not os.path.realpath(cwd).startswith(proj_dir): - raise StageBadCwdError(cwd) + + if not os.path.exists(os.path.realpath(path)): + raise StagePathNotFoundError(path) + + proj_dir = os.path.realpath(repo.root_dir) + os.path.sep + if not (os.path.realpath(path) + os.path.sep).startswith(proj_dir): + raise StagePathOutsideError(path) @property def is_cached(self): @@ -359,14 +370,14 @@ class Stage(object): metrics=None, metrics_no_cache=None, fname=None, - cwd=os.curdir, + cwd=None, + wdir=None, locked=False, add=False, overwrite=True, ignore_build_cache=False, remove_outs=False, ): - if outs is None: outs = [] if deps is None: @@ -378,7 +389,20 @@ class Stage(object): if metrics_no_cache is None: metrics_no_cache = [] - stage = Stage(repo=repo, cwd=cwd, cmd=cmd, locked=locked) + # Backward compatibility for `cwd` option + if wdir is None and cwd is not None: + if fname is not None and os.path.basename(fname) != fname: + raise StageFileBadNameError( + "stage file name '{fname}' may not contain subdirectories" + " if '-c|--cwd' (deprecated) is specified. Use '-w|--wdir'" + " along with '-f' to specify stage file path and working" + " directory.".format(fname=fname) + ) + wdir = cwd + else: + wdir = os.curdir if wdir is None else wdir + + stage = Stage(repo=repo, wdir=wdir, cmd=cmd, locked=locked) stage.outs = output.loads_from(stage, outs, use_cache=True) stage.outs += output.loads_from( @@ -393,22 +417,18 @@ class Stage(object): stage._check_circular_dependency() stage._check_duplicated_arguments() - if fname is not None and os.path.basename(fname) != fname: - raise StageFileBadNameError( - "stage file name '{fname}' should not contain subdirectories." - " Use '-c|--cwd' to change location of the stage file.".format( - fname=fname - ) - ) + fname = Stage._stage_fname(fname, stage.outs, add=add) + wdir = os.path.abspath(wdir) - fname, cwd = Stage._stage_fname_cwd(fname, cwd, stage.outs, add=add) + if cwd is not None: + path = os.path.join(wdir, fname) + else: + path = os.path.abspath(fname) - Stage._check_inside_repo(repo, cwd) + Stage._check_stage_path(repo, wdir) + Stage._check_stage_path(repo, os.path.dirname(path)) - cwd = os.path.abspath(cwd) - path = os.path.join(cwd, fname) - - stage.cwd = cwd + stage.wdir = wdir stage.path = path # NOTE: remove outs before we check build cache @@ -463,11 +483,16 @@ class Stage(object): d = yaml.safe_load(fd) or {} Stage.validate(d, fname=os.path.relpath(fname)) + path = os.path.abspath(fname) stage = Stage( repo=repo, - path=os.path.abspath(fname), - cwd=os.path.dirname(os.path.abspath(fname)), + path=path, + wdir=os.path.abspath( + os.path.join( + os.path.dirname(path), d.get(Stage.PARAM_WDIR, ".") + ) + ), cmd=d.get(Stage.PARAM_CMD), md5=d.get(Stage.PARAM_MD5), locked=d.get(Stage.PARAM_LOCKED, False), @@ -484,6 +509,7 @@ class Stage(object): for key, value in { Stage.PARAM_MD5: self.md5, Stage.PARAM_CMD: self.cmd, + Stage.PARAM_WDIR: self.wdir, Stage.PARAM_LOCKED: self.locked, Stage.PARAM_DEPS: [d.dumpd() for d in self.deps], Stage.PARAM_OUTS: [o.dumpd() for o in self.outs], @@ -501,9 +527,13 @@ class Stage(object): file=os.path.relpath(fname) ) ) + d = self.dumpd() + d[Stage.PARAM_WDIR] = os.path.relpath( + self.wdir, os.path.dirname(fname) + ) with open(fname, "w") as fd: - yaml.safe_dump(self.dumpd(), fd, default_flow_style=False) + yaml.safe_dump(d, fd, default_flow_style=False) self.repo.files_to_git_add.append(os.path.relpath(fname)) @@ -516,6 +546,13 @@ class Stage(object): if self.PARAM_MD5 in d.keys(): del d[self.PARAM_MD5] + # Ignore the wdir default value. In this case stage file w/o + # wdir has the same md5 as a file with the default value specified. + # It's important for backward compatibility with pipelines that + # didn't have WDIR in their stage files. + if d.get(self.PARAM_WDIR) == ".": + del d[self.PARAM_WDIR] + # NOTE: excluding parameters that don't affect the state of the # pipeline. Not excluding `OutputLOCAL.PARAM_CACHE`, because if # it has changed, we might not have that output in our cache. @@ -614,7 +651,7 @@ class Stage(object): p = subprocess.Popen( self.cmd, - cwd=self.cwd, + cwd=self.wdir, shell=True, env=fix_env(os.environ), executable=executable, diff --git a/tests/test_repro.py b/tests/test_repro.py index 421fe13fe..b3b38ac7b 100644 --- a/tests/test_repro.py +++ b/tests/test_repro.py @@ -23,7 +23,7 @@ from dvc.stage import Stage, StageFileDoesNotExistError from dvc.system import System from dvc.exceptions import ( CyclicGraphError, - WorkingDirectoryAsOutputError, + StagePathAsOutputError, ReproductionError, ) @@ -121,7 +121,7 @@ class TestReproWorkingDirectoryAsOutput(TestDvc): yaml.safe_dump(stage_dump, fd, default_flow_style=False) - with self.assertRaises(WorkingDirectoryAsOutputError): + with self.assertRaises(StagePathAsOutputError): self.dvc.reproduce(faulty_stage_path) def test_nested(self): @@ -170,7 +170,7 @@ class TestReproWorkingDirectoryAsOutput(TestDvc): Stage.load(self.dvc, error_stage_path), ] - with self.assertRaises(WorkingDirectoryAsOutputError): + with self.assertRaises(StagePathAsOutputError): self.dvc.reproduce(error_stage_path) def test_similar_paths(self): @@ -196,8 +196,8 @@ class TestReproWorkingDirectoryAsOutput(TestDvc): try: self.dvc.reproduce(stage) - except WorkingDirectoryAsOutputError: - self.fail("should not raise WorkingDirectoryAsOutputError") + except StagePathAsOutputError: + self.fail("should not raise StagePathAsOutputError") class TestReproDepUnderDir(TestDvc): diff --git a/tests/test_run.py b/tests/test_run.py index 98eaa1f75..ab1b820a7 100644 --- a/tests/test_run.py +++ b/tests/test_run.py @@ -1,4 +1,6 @@ import os +import uuid + import mock import shutil import filecmp @@ -7,15 +9,15 @@ import subprocess from dvc.main import main from dvc.utils import file_md5 from dvc.system import System -from dvc.stage import Stage +from dvc.stage import Stage, StagePathNotFoundError from dvc.stage import StageFileBadNameError, MissingDep -from dvc.stage import StageBadCwdError, StageFileAlreadyExistsError +from dvc.stage import StagePathOutsideError, StageFileAlreadyExistsError from dvc.exceptions import ( OutputDuplicationError, CircularDependencyError, CyclicGraphError, ArgumentDuplicationError, - WorkingDirectoryAsOutputError, + StagePathAsOutputError, ) from tests.basic_env import TestDvc @@ -189,11 +191,11 @@ class TestRunDuplicatedArguments(TestDvc): ) -class TestRunWorkingDirectoryAsOutput(TestDvc): - def test(self): +class TestRunStageInsideOutput(TestDvc): + def test_cwd(self): self.dvc.run(cmd="", deps=[], outs=[self.DATA_DIR]) - with self.assertRaises(WorkingDirectoryAsOutputError): + with self.assertRaises(StagePathAsOutputError): self.dvc.run( cmd="", cwd=self.DATA_DIR, @@ -201,12 +203,73 @@ class TestRunWorkingDirectoryAsOutput(TestDvc): fname="inside-cwd.dvc", ) + def test_file_name(self): + self.dvc.run(cmd="", deps=[], outs=[self.DATA_DIR]) + + with self.assertRaises(StagePathAsOutputError): + self.dvc.run( + cmd="", + outs=[self.FOO], + fname=os.path.join(self.DATA_DIR, "inside-cwd.dvc"), + ) + class TestRunBadCwd(TestDvc): def test(self): - with self.assertRaises(StageBadCwdError): + with self.assertRaises(StagePathOutsideError): self.dvc.run(cmd="", cwd=self.mkdtemp()) + def test_same_prefix(self): + with self.assertRaises(StagePathOutsideError): + path = "{}-{}".format(self._root_dir, uuid.uuid4()) + os.mkdir(path) + self.dvc.run(cmd="", cwd=path) + + +class TestRunBadWdir(TestDvc): + def test(self): + with self.assertRaises(StagePathOutsideError): + self.dvc.run(cmd="", wdir=self.mkdtemp()) + + def test_same_prefix(self): + with self.assertRaises(StagePathOutsideError): + path = "{}-{}".format(self._root_dir, uuid.uuid4()) + os.mkdir(path) + self.dvc.run(cmd="", wdir=path) + + def test_not_found(self): + with self.assertRaises(StagePathNotFoundError): + path = os.path.join(self._root_dir, str(uuid.uuid4())) + self.dvc.run(cmd="", wdir=path) + + +class TestRunBadName(TestDvc): + def test(self): + with self.assertRaises(StagePathOutsideError): + self.dvc.run( + cmd="", + fname=os.path.join( + self.mkdtemp(), self.FOO + Stage.STAGE_FILE_SUFFIX + ), + ) + + def test_same_prefix(self): + with self.assertRaises(StagePathOutsideError): + path = "{}-{}".format(self._root_dir, uuid.uuid4()) + os.mkdir(path) + self.dvc.run( + cmd="", + fname=os.path.join(path, self.FOO + Stage.STAGE_FILE_SUFFIX), + ) + + def test_not_found(self): + with self.assertRaises(StagePathNotFoundError): + path = os.path.join(self._root_dir, str(uuid.uuid4())) + self.dvc.run( + cmd="", + fname=os.path.join(path, self.FOO + Stage.STAGE_FILE_SUFFIX), + ) + class TestCmdRun(TestDvc): def test_run(self):