From 35756d1534c9e72c841f663b84b32b84efca7fb0 Mon Sep 17 00:00:00 2001 From: Garrett Malmquist Date: Thu, 12 Jun 2014 18:26:19 -0400 Subject: [PATCH] Fixes the bug where pants doesn't recognize build targets on top-level directories. The previous argument parser naively simply checked whether a command-line argument contained a slash (/) or a colon (:). This is a reasonable assumption most of the time, but in the case where a BUILD file is located only one directory deep, (eg, buildroot/folder/BUILD), this makes it impossible to run pants on the target simply by calling (eg) ./pants goal bundle folder. See github issue for example: https://github.com/pantsbuild/pants/pull/159 This fixes it by actually checking to see if a phase with goals is defined by the command-line argument. It still checks before that whether the argument contains a slash or a colon, because that if an argument has those characters it can't mean a goal. But an argument not having those characters doesn't mean it is a goal. In a completely ambiguous situation (eg, given a command argument 'bundle', and there is a valid BUILD file at bundle/BUILD), we err on the side of the goal, and print out a warning. This makes sense, because it allows the user to disambiguate the situation using the '--' syntax. --- src/python/pants/commands/goal.py | 30 +++++++++++++++++-- tests/BUILD | 12 ++++++++ tests/python/pants_test/commands/BUILD | 1 + tests/python/pants_test/commands/test_goal.py | 6 ++++ 4 files changed, 47 insertions(+), 2 deletions(-) create mode 100644 tests/BUILD diff --git a/src/python/pants/commands/goal.py b/src/python/pants/commands/goal.py index 947c782e39f2..234c937b393e 100644 --- a/src/python/pants/commands/goal.py +++ b/src/python/pants/commands/goal.py @@ -6,6 +6,7 @@ from contextlib import contextmanager import inspect +import logging import os import sys import traceback @@ -18,6 +19,7 @@ from pants.backend.core.tasks.console_task import ConsoleTask from pants.backend.core.tasks.task import Task +from pants.backend.jvm.tasks.nailgun_task import NailgunTask # XXX(pl) from pants.base.address import BuildFileAddress, parse_spec from pants.base.build_environment import get_buildroot from pants.base.build_file import BuildFile @@ -34,7 +36,6 @@ from pants.goal.help import print_help from pants.goal.initialize_reporting import update_reporting from pants.goal.option_helpers import add_global_options -from pants.backend.jvm.tasks.nailgun_task import NailgunTask # XXX(pl) StringIO = Compatibility.StringIO @@ -51,9 +52,34 @@ def parse_args(args): goals = OrderedSet() specs = OrderedSet() explicit_multi = False + logger = logging.getLogger(__name__) + double_dash = u'--' in args + goal_names = [phase.name for phase, goal in Phase.all()] + if not goal_names: + raise RuntimeError( + 'Arguments cannot be parsed before the list of goals from Phase.all() is populated.') def is_spec(spec): - return os.sep in spec or ':' in spec + if os.sep in spec or ':' in spec: + return True # Definitely not a goal. + if not (spec in goal_names): + return True # Definitely not a (known) goal. + if double_dash: + # This means that we're parsing the half of the expression before a --, so assume it's a + # goal without warning. + return False + # Here, it's possible we have a goal and target with the same name. For now, always give + # priority to the goal, but give a warning if they might have meant the target (if the BUILD + # file exists). + try: + BuildFile(get_buildroot(), spec) + msg = (' Command-line argument "{spec}" is ambiguous, and was assumed to be a goal.' + ' If this is incorrect, disambiguate it with the "--" argument to separate goals' + ' from targets.') + logger.warning(msg.format(spec=spec)) + except IOError: pass # Awesome, it's unambiguous. + return False + for i, arg in enumerate(args): if not arg.startswith('-'): diff --git a/tests/BUILD b/tests/BUILD new file mode 100644 index 000000000000..043b2aaa7b30 --- /dev/null +++ b/tests/BUILD @@ -0,0 +1,12 @@ +# Copyright 2014 Pants project contributors (see CONTRIBUTORS.md). +# Licensed under the Apache License, Version 2.0 (see LICENSE). + +# Yes, this is a weird place for this BUILD file. It's here to test if pants can handle targets that +# are only 1 directory deep, and thus might not have a ':' or '/' on the spec path. (which goal.py +# previously has been bad about.) +java_library(name='tests', + sources=[], + dependencies=[ + 'tests/java/com/pants/examples/hello/greet', + ], +) diff --git a/tests/python/pants_test/commands/BUILD b/tests/python/pants_test/commands/BUILD index 84357d0ecd69..7a1c570e14ef 100644 --- a/tests/python/pants_test/commands/BUILD +++ b/tests/python/pants_test/commands/BUILD @@ -20,6 +20,7 @@ python_tests( name = 'test_goal', sources = [ 'test_goal.py' ], dependencies = [ + pants('src/python/pants/backend/jvm:plugin'), pants('src/python/pants/commands:goal'), pants('tests/python/pants_test:base_test'), ] diff --git a/tests/python/pants_test/commands/test_goal.py b/tests/python/pants_test/commands/test_goal.py index adca1711a91a..5be939b1e94a 100644 --- a/tests/python/pants_test/commands/test_goal.py +++ b/tests/python/pants_test/commands/test_goal.py @@ -6,11 +6,17 @@ import unittest +from pants.backend.jvm.register import register_goals from pants.commands.goal import Goal, GoalError class GoalTest(unittest.TestCase): def test_parse_args(self): + # Have to load in goals, because parse_args requires Phase.all() now. + # Fortunately, we only have to load in the goals we're actually using below, so the goals in the + # core register are sufficient. + register_goals() + def assert_result(goals, specs, args): g, s = Goal.parse_args(args) self.assertEquals((goals, specs), (list(g), list(s)))