Skip to content

Commit

Permalink
Fixes the bug where pants doesn't recognize build targets on top-leve…
Browse files Browse the repository at this point in the history
…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: pantsbuild#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.
  • Loading branch information
gmalmquist committed Jun 13, 2014
1 parent f3d1cd6 commit 35756d1
Show file tree
Hide file tree
Showing 4 changed files with 47 additions and 2 deletions.
30 changes: 28 additions & 2 deletions src/python/pants/commands/goal.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

from contextlib import contextmanager
import inspect
import logging
import os
import sys
import traceback
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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('-'):
Expand Down
12 changes: 12 additions & 0 deletions tests/BUILD
Original file line number Diff line number Diff line change
@@ -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',
],
)
1 change: 1 addition & 0 deletions tests/python/pants_test/commands/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
]
Expand Down
6 changes: 6 additions & 0 deletions tests/python/pants_test/commands/test_goal.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)))
Expand Down

0 comments on commit 35756d1

Please sign in to comment.