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 17, 2014
1 parent cff740c commit 4f0bbc9
Show file tree
Hide file tree
Showing 4 changed files with 274 additions and 23 deletions.
37 changes: 33 additions & 4 deletions src/python/pants/commands/goal.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

from contextlib import contextmanager
import inspect
import logging
import os
import sys
import traceback
Expand All @@ -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
Expand All @@ -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
Expand All @@ -44,6 +45,9 @@
class Goal(Command):
"""Lists installed goals or else executes a named goal."""

class IntermixedArgumentsError(GoalError):
pass

__command__ = 'goal'
output = None

Expand All @@ -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
Expand Down
2 changes: 2 additions & 0 deletions tests/python/pants_test/commands/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
]
)
Expand Down
79 changes: 60 additions & 19 deletions tests/python/pants_test/commands/test_goal.py
Original file line number Diff line number Diff line change
Expand Up @@ -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'])
179 changes: 179 additions & 0 deletions tests/python/pants_test/test_binary_util.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,179 @@
# coding=utf-8
# Copyright 2014 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

from __future__ import (nested_scopes, generators, division, absolute_import, with_statement,
print_function, unicode_literals)

import os

from pants.base.config import Config
from pants.base.exceptions import TaskError
from pants.binary_util import select_binary_stream, select_binary_base_path
from pants_test.base_test import BaseTest


class BinaryUtilTest(BaseTest):
"""Tests binary_util's pants_support_baseurls handling."""

class LambdaReader(object):
"""Class which pretends to be an input stream, but is actually a lambda function."""

def __init__(self, func):
self._func = func

def __call__(self):
return self._func()

def read(self):
return self()

def __enter__(self, a=None, b=None, c=None, d=None):
return self

def __exit__(self, a=None, b=None, c=None, d=None):
pass

class MapReader(object):
"""Class which pretends to be a url stream opener, but is actually a dictionary."""

def __init__(self, read_map):
self._map = read_map

def __call__(self, key):
if not key in self._map:
raise IOError("404: Virtual URL '{key}' does not exist.".format(key=key))
value = self._map[key]
return BinaryUtilTest.LambdaReader(lambda: value)

def keys(self):
return self._map.keys()

def __getitem__(self, key):
return self._map[key] # Vanilla internal map access (without lambda shenanigans).

def setUp(self):
super(BinaryUtilTest, self).setUp()

def config_urls(self, urls=None, legacy=None):
"""Generates the contents of a configuration file."""
def clean_config(txt):
return '\n'.join((line.strip() for line in txt.split('\n')))

if legacy:
legacy = '\npants_support_baseurl: {url}'.format(url=legacy)
else:
legacy = ''

if urls:
urls = '\npants_support_baseurls = {urls}'.format(urls=urls)
else:
urls = ''

return self.config(overrides=clean_config('[DEFAULT]{urls}{legacy}'.format(urls=urls,
legacy=legacy)))

@classmethod
def _fake_base(cls, name):
return 'fake-url-{name}'.format(name=name)

@classmethod
def _fake_url(cls, binaries, base, binary_key):
base_path, version, name = binaries[binary_key]
return '{base}/{binary}'.format(base=base,
binary=select_binary_base_path(base_path, version, name))

def _seens_test(self, binaries, bases, reader, config=None):
unseen = filter(lambda item: item.startswith('SEEN '), (reader[key] for key in reader.keys()))
if not config:
config = self.config_urls(bases)
for key in binaries:
base_path, version, name = binaries[key]
with select_binary_stream(base_path,
version,
name,
config=config,
url_opener=reader) as stream:
self.assertEqual(stream(), 'SEEN ' + key.upper())
unseen.remove(stream())
self.assertEqual(0, len(unseen)) # Make sure we've seen all the SEENs.

def test_legacy_support(self):
"""Tests legacy support for old pants.ini."""
fake_base, fake_url = self._fake_base, self._fake_url
binaries = {
'protoc': ('bin/protobuf', '2.4.1', 'protoc',),
'ivy': ('bin/ivy', '4.3.7', 'ivy',),
'bash': ('bin/bash', '4.4.3', 'bash',),
}
bases = [fake_base('apple'), fake_base('orange'), fake_base('banana'),]
reader = self.MapReader({
fake_url(binaries, bases[0], 'protoc'): 'SEEN PROTOC',
fake_url(binaries, bases[0], 'ivy'): 'SEEN IVY',
fake_url(binaries, bases[0], 'bash'): 'SEEN BASH',
})
config = self.config_urls(legacy=bases[0])
self._seens_test(binaries, bases, reader, config=config)

def test_mixed_support(self):
"""Tests support for mixed legacy and new style build support in pants.ini."""
fake_base, fake_url = self._fake_base, self._fake_url
binaries = {
'protoc': ('bin/protobuf', '2.4.1', 'protoc',),
'ivy': ('bin/ivy', '4.3.7', 'ivy',),
'bash': ('bin/bash', '4.4.3', 'bash',),
}
bases = [fake_base('apple'), fake_base('orange'), fake_base('banana'),]
reader = self.MapReader({
fake_url(binaries, bases[0], 'protoc'): 'SEEN PROTOC',
fake_url(binaries, bases[1], 'ivy'): 'SEEN IVY',
fake_url(binaries, bases[2], 'bash'): 'SEEN BASH',
})
config = self.config_urls(urls=bases[1:], legacy=bases[0])
self._seens_test(binaries, bases, reader, config=config)

def test_nobases(self):
"""Tests exception handling if build support urls are improperly specified."""
try:
with select_binary_stream('bin/foo', '4.4.3', 'foo', self.config_urls()) as stream:
self.assertTrue(False) # We should get an exception before we reach here.
except TaskError as e:
self.assertTrue(e.message.find(' defined ') > 0) # Check if we got the right error message.

def test_support_url_multi(self):
"""Tests to make sure existing base urls function as expected."""
config = self.config_urls([
'BLATANTLY INVALID URL',
'https://pantsbuild.github.io/binaries/reasonably-invalid-url',
'https://pantsbuild.github.io/binaries/build-support',
'https://pantsbuild.github.io/binaries/build-support', # Test duplicate entry handling.
'https://pantsbuild.github.io/binaries/another-invalid-url',
])
binaries = [
('bin/protobuf', '2.4.1', 'protoc',),
]
for base_path, version, name in binaries:
one = 0
with select_binary_stream(base_path, version, name, config=config) as stream:
stream()
one += 1
self.assertEqual(one, 1)

def test_support_url_fallback(self):
"""Tests fallback behavior with multiple support baseurls."""
fake_base, fake_url = self._fake_base, self._fake_url
binaries = {
'protoc': ('bin/protobuf', '2.4.1', 'protoc',),
'ivy': ('bin/ivy', '4.3.7', 'ivy',),
'bash': ('bin/bash', '4.4.3', 'bash',),
}
bases = [fake_base('apple'), fake_base('orange'), fake_base('banana'),]
reader = self.MapReader({
fake_url(binaries, bases[0], 'protoc'): 'SEEN PROTOC',
fake_url(binaries, bases[0], 'ivy'): 'SEEN IVY',
fake_url(binaries, bases[1], 'bash'): 'SEEN BASH',
fake_url(binaries, bases[1], 'protoc'): 'UNSEEN PROTOC 1',
fake_url(binaries, bases[2], 'protoc'): 'UNSEEN PROTOC 2',
fake_url(binaries, bases[2], 'ivy'): 'UNSEEN IVY 2',
})
self._seens_test(binaries, bases, reader)

0 comments on commit 4f0bbc9

Please sign in to comment.