Fixes the bug where pants doesn't recognize build targets on top-leve…

…l 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: #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.

Testing done:
Added unit tests to pants_test/commands/test_goal.py; everything passes.

Bugs Fixed 218

Reviewed at https://rbcommons.com/s/twitter/r/551/

src/python/pants/commands/goal.py

@@ -7,6 +7,7 @@
 
 from contextlib import contextmanager
 import inspect
+import logging
 import os
 import sys
 import traceback
@@ -19,6 +20,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
@@ -35,7 +37,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
@@ -44,6 +45,9 @@
 class Goal(Command):
   """Lists installed goals or else executes a named goal."""
 
+  class IntermixedArgumentsError(GoalError):
+    pass
+
   __command__ = 'goal'
   output = None
 
@@ -52,17 +56,42 @@ def parse_args(args):
     goals = OrderedSet()
     specs = OrderedSet()
     explicit_multi = False
+    logger = logging.getLogger(__name__)
+    has_double_dash = u'--' in args
+    goal_names = [phase.name for phase, goal in Phase.all()]
+    if not goal_names:
+      raise GoalError(
+        '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 has_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('-'):
         specs.add(arg) if is_spec(arg) else goals.add(arg)
       elif '--' == arg:
         if specs:
-          raise GoalError('Cannot intermix targets with goals when using --. Targets should '
-                          'appear on the right')
+          raise Goal.IntermixedArgumentsError('Cannot intermix targets with goals when using --. '
+                                              'Targets should appear on the right')
         explicit_multi = True
         del args[i]
         break

tests/python/pants_test/commands/BUILD

@@ -20,7 +20,9 @@ python_tests(
   name = 'test_goal',
   sources = [ 'test_goal.py' ],
   dependencies = [
+    pants('src/python/pants/backend/jvm:plugin'),
     pants('src/python/pants/commands:goal'),
+    pants('src/python/pants/goal:phase'),
     pants('tests/python/pants_test:base_test'),
   ]
 )

tests/python/pants_test/commands/test_goal.py

@@ -7,41 +7,82 @@
 
 import unittest
 
+from pants.backend.jvm.register import register_goals
 from pants.commands.goal import Goal, GoalError
+from pants.goal import GoalError
+from pants.goal.phase import Phase
+from pants_test.base_test import BaseTest
 
 
-class GoalTest(unittest.TestCase):
-  def test_parse_args(self):
-    def assert_result(goals, specs, args):
-      g, s = Goal.parse_args(args)
-      self.assertEquals((goals, specs), (list(g), list(s)))
+class GoalTest(BaseTest):
+
+  def setUp(self):
+    super(GoalTest, self).setUp()
+    # 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 tearDown(self):
+    super(GoalTest, self).tearDown()
+    Phase.clear()
+
+  def assert_result(self, goals, specs, args):
+    g, s = Goal.parse_args(args)
+    self.assertEquals((goals, specs), (list(g), list(s)))
+
+  def test_top_level_dir(self):
+    self.create_dir('topleveldir')
+    self.create_file('topleveldir/BUILD', contents='')
+    self.assert_result(goals=[], specs=['topleveldir'], args=['topleveldir'])
+
+  def test_arg_ambiguity(self):
+    self.create_dir('compile')
+    self.create_file('compile/BUILD', contents='')
+    self.assert_result(goals=['compile'], specs=[], args=['compile'])
+    # Only end up with one 'compile' here, because goals and specs are sets.
+    self.assert_result(goals=['compile'], specs=[], args=['compile', 'compile'])
+    self.assert_result(goals=['compile'], specs=['compile'], args=['compile', '--', 'compile'])
 
-    assert_result(goals=[], specs=[], args=[])
-    assert_result(goals=[], specs=[], args=['--'])
-    assert_result(goals=[], specs=[], args=['-v', '--help'])
+    try:
+      self.assert_result(goals=['compile'], specs=[], args=['compile', 'spec:', '--', 'compile'])
+      self.fail('Expected mixed specs and goals to the left of an explicit multi-goal sep (--) to '
+                'be rejected.')
+    except Goal.IntermixedArgumentsError:
+      pass # expected
+
+    self.assert_result(goals=['bundle', 'compile'], specs=['run'],
+                       args=['bundle', 'compile', '--', 'run'])
+
+  def test_parse_args(self):
+    self.assert_result(goals=[], specs=[], args=[])
+    self.assert_result(goals=[], specs=[], args=['--'])
+    self.assert_result(goals=[], specs=[], args=['-v', '--help'])
 
-    assert_result(goals=['compile'], specs=[], args=['compile', '--log'])
-    assert_result(goals=['compile', 'test'], specs=[], args=['compile', 'test'])
-    assert_result(goals=['compile', 'test'], specs=[], args=['compile', '-v', 'test'])
+    self.assert_result(goals=['compile'], specs=[], args=['compile', '--log'])
+    self.assert_result(goals=['compile', 'test'], specs=[], args=['compile', 'test'])
+    self.assert_result(goals=['compile', 'test'], specs=[], args=['compile', '-v', 'test'])
 
-    assert_result(goals=[], specs=['resolve'], args=['--', 'resolve', '--ivy-open'])
-    assert_result(goals=['test'], specs=['resolve'], args=['test', '--', 'resolve', '--ivy-open'])
+    self.assert_result(goals=[], specs=['resolve'], args=['--', 'resolve', '--ivy-open'])
+    self.assert_result(goals=['test'], specs=['resolve'], args=['test', '--', 'resolve',
+                                                                '--ivy-open'])
 
     try:
       Goal.parse_args(['test', 'lib:all', '--', 'resolve'])
       self.fail('Expected mixed specs and goals to the left of an explicit '
                 'multi-goal sep (--) to be rejected.')
-    except GoalError:
+    except Goal.IntermixedArgumentsError:
       pass # expected
 
     try:
       Goal.parse_args(['resolve', 'lib/all', 'test', '--'])
       self.fail('Expected mixed specs and goals to the left of an explicit '
                 'multi-goal sep (--) to be rejected.')
-    except GoalError:
+    except Goal.IntermixedArgumentsError:
       pass # expected
 
-    assert_result(goals=['test'], specs=['lib:all'], args=['lib:all', '-v', 'test'])
-    assert_result(goals=['test'], specs=['lib/'], args=['-v', 'test', 'lib/'])
-    assert_result(goals=['test'], specs=['lib/io:sound'], args=['test', '-v', 'lib/io:sound'])
-    assert_result(goals=['test'], specs=['lib:all'], args=['-h', 'test', '-v', 'lib:all', '-x'])
+    self.assert_result(goals=['test'], specs=['lib:all'], args=['lib:all', '-v', 'test'])
+    self.assert_result(goals=['test'], specs=['lib/'], args=['-v', 'test', 'lib/'])
+    self.assert_result(goals=['test'], specs=['lib/io:sound'], args=['test', '-v', 'lib/io:sound'])
+    self.assert_result(goals=['test'], specs=['lib:all'], args=['-h', 'test', '-v', 'lib:all',
+                                                                '-x'])