Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Print Target Definition Using Buildozer #5142

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,9 @@ python_library(
name='meta_rename',
sources=['meta_rename.py']
)


python_library(
name='print_target',
sources=['print_target.py']
)
Original file line number Diff line number Diff line change
Expand Up @@ -72,17 +72,31 @@ def _execute_buildozer_script(self, command):
Buildozer.execute_binary(command, address.spec, binary=self._executable)

@classmethod
def execute_binary(cls, command, spec, binary=None, version='0.6.0.dce8b3c287652cbcaf43c8dd076b3f48c92ab44c'):
def execute_binary(cls, command, spec, binary=None, version='0.6.0.dce8b3c287652cbcaf43c8dd076b3f48c92ab44c', suppress_warnings=False):
binary = binary if binary else BinaryUtil.Factory.create().select_binary('scripts/buildozer', version, 'buildozer')
Buildozer._execute_buildozer_command([binary, command, spec], suppress_warnings, output=False)

Buildozer._execute_buildozer_command([binary, command, spec])
@classmethod
def return_buildozer_output(cls, command, spec, binary=None, version='0.6.0.dce8b3c287652cbcaf43c8dd076b3f48c92ab44c', suppress_warnings=False):
binary = binary if binary else BinaryUtil.Factory.create().select_binary('scripts/buildozer', version, 'buildozer')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see you probably cannot use self._executable because it is an instance variable and does not apply to a classmethod.

The correct way is to make Buildozer a subsystem and --version being an option in that subsystem, so various tasks with buildozer subsystem can use the same version of buildozer. E.g.

self._ivy_subsystem = ivy_subsystem or IvySubsystem.global_instance()
then you can grab the global instance of buildozer subsystem and tell it to run certain commands.

This as it stands still needs some improvement. I understand as far as the class goes it ends tomorrow evening, but we will appreciate that you can follow up the effort if you cannot finish refactoring the subsystem into it by then.

Function wise, I can consider this PR gets the point, but for production code, we have to gate keep a bit more :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the advice and approach recommendation, I will try to implement it this way as soon as possible - and am looking forward to continuing development on pants after the class is over.

return Buildozer._execute_buildozer_command([binary, command, spec], suppress_warnings, output=True)

@classmethod
def _execute_buildozer_command(cls, buildozer_command):
try:
subprocess.check_call(buildozer_command, cwd=get_buildroot())
except subprocess.CalledProcessError as err:
if err.returncode == 3:
logger.warn('{} ... no changes were made'.format(buildozer_command))
else:
raise TaskError('{} ... exited non-zero ({}).'.format(buildozer_command, err.returncode))
def _execute_buildozer_command(cls, buildozer_command, suppress_warnings, output):
if not output:
try:
subprocess.check_call(buildozer_command, cwd=get_buildroot())
except subprocess.CalledProcessError as err:
if err.returncode == 3:
if not suppress_warnings:
logger.warn('{} ... no changes were made'.format(buildozer_command))
else:
raise TaskError('{} ... exited non-zero ({}).'.format(buildozer_command, err.returncode))
else:
try:
subprocess.check_output(buildozer_command, cwd=get_buildroot(), stderr=subprocess.STDOUT)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not get the chance to run the code step by step yet, so cannot tell the reason for stderr=subprocess.STDOUT, but I am guessing this method can be further simplified.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@itsedvard lets coordinate on this as both the print-target and meta-relocate tasks are dependent on this refactoring.

except subprocess.CalledProcessError as err:
if err.returncode == 3:
return err.output
else:
raise TaskError('{} ... exited non-zero ({}).'.format(buildozer_command, err.returncode))
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
# coding=utf-8
# Copyright 2017 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

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

from pants.base.exceptions import TaskError
from pants.task.console_task import ConsoleTask

from pants.contrib.buildrefactor.buildozer import Buildozer


class PrintTarget(ConsoleTask):
"""Print's a specified target if found in the associated build file

line-number: optional flag to print the starting and ending line numbers of the target

Example:
$./pants print-target --line-number testprojects/tests/java/org/pantsbuild/testproject/dummies:passing_target
"""

@classmethod
def register_options(cls, register):
super(PrintTarget, cls).register_options(register)

register('--line-number', help='Prints the starting line number of the named target.', type=bool, default=False)

def __init__(self, *args, **kwargs):
super(PrintTarget, self).__init__(*args, **kwargs)

if len(self.context.target_roots) > 1:
raise TaskError('More than one target specified:\n{}'.format(str(self.context.target_roots)))

self.target = self.context.target_roots[0]
self.options = self.get_options()

def console_output(self, targets):

spec_path = self.target.address.spec

yield('\'{}\' found in BUILD file.\n'.format(self.target.name))

if self.options.line_number:
startline_output = Buildozer.return_buildozer_output(spec = spec_path, command = 'print startline', suppress_warnings=True)
startline_digit = int(filter(str.isdigit, startline_output))

endline_output = Buildozer.return_buildozer_output(spec = spec_path, command = 'print endline', suppress_warnings=True)
endline_digit = int(filter(str.isdigit, endline_output))

yield('Line numbers: {}-{}.\n'.format(startline_digit, endline_digit))

yield('Target definiton:\n\n{}'.format(Buildozer.return_buildozer_output(spec = spec_path, command = 'print rule', suppress_warnings=True)))
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,12 @@
unicode_literals, with_statement)

from pants.goal.task_registrar import TaskRegistrar as task

from pants.contrib.buildrefactor.buildozer import Buildozer
from pants.contrib.buildrefactor.meta_rename import MetaRename
from pants.contrib.buildrefactor.print_target import PrintTarget


def register_goals():
task(name='buildozer', action=Buildozer).install('buildozer')
task(name='meta-rename', action=MetaRename).install('meta-rename')
task(name='print-target', action=PrintTarget).install('print-target')
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,13 @@ python_tests(
]
)

python_tests(
name='print_target_integration',
sources=['test_print_target_integration.py'],
dependencies=[
'tests/python/pants_test:int-test',
]
)

python_tests(
name='meta_rename_integration',
Expand All @@ -53,3 +60,4 @@ python_tests(
'tests/python/pants_test:int-test',
]
)

Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
# coding=utf-8
# Copyright 2017 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

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

from pants_test.pants_run_integration_test import PantsRunIntegrationTest


class PrintTargetIntegrationTest(PantsRunIntegrationTest):
"""Test Peek goal functionality

$./pants test contrib/buildrefactor/tests/python/pants_test/contrib/buildrefactor:print_target_integration
"""

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you also add a negative case here with an invalid target to show what the behavior should be?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to include additional test test_print_invalid_name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Also I think it's probably better to address
#5175 (comment)
#5175 (comment)
here since @itsedvard 's patch probably depends on this, and there are a lot more comments to address on that patch.

Copy link
Author

@denalimarsh denalimarsh Dec 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Responded to your comments on the Meta Relocate PR...

TaskError:
#5175 (comment)

buildozer.py refactoring:
#5175 (comment)

I also updated test_print_target_integration.py with a test_multiple_targets() test for the raised TaskError.

Copy link
Author

@denalimarsh denalimarsh Dec 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had the CI fail with:

ERROR: /home/travis/build/pantsbuild/pants/contrib/buildrefactor/src/python/pants/contrib/buildrefactor/print_target.py Imports are incorrectly sorted.

So I changed the import order in print_target.py so TaskError comes before ConsoleTask, as in buildozer.py and in list_owners.py, an example I referenced: https://github.com/pantsbuild/pants/blob/d30cca1e0ecb9cc0e1b7e2cd0ff6e7e077e62a52/src/python/pants/backend/graph_info/tasks/list_owners.py

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

./pants fmt contrib/buildrefactor/src/python/pants/contrib/buildrefactor:: should fix it for you

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix!

def test_print_name(self):
print_target_print_run = self.run_pants(['print-target',
'testprojects/tests/java/org/pantsbuild/testproject/buildrefactor/x:X'])

self.assertIn('\'X\' found in BUILD file.', print_target_print_run.stdout_data)
self.assertIn('name = "X"', print_target_print_run.stdout_data)

def test_print_invalid_name(self):
print_target_invalid_name_run = self.run_pants(['print-target',
'testprojects/tests/java/org/pantsbuild/testproject/buildrefactor/x:Y'])

self.assertIn('ResolveError: "Y" was not found in namespace', print_target_invalid_name_run.stderr_data)
self.assertIn('Did you mean one of:\n :X', print_target_invalid_name_run.stderr_data)

def test_print_line_number(self):
print_target_line_number_run = self.run_pants(['print-target',
'--line-number',
'testprojects/tests/java/org/pantsbuild/testproject/buildrefactor/x:X'])

self.assertIn('Line numbers: 4-6.', print_target_line_number_run.stdout_data)
self.assertIn('name = "X"', print_target_line_number_run.stdout_data)

def test_multiple_targets(self):
print_target_multiple_targets_run = self.run_pants(['print-target',
'testprojects/tests/java/org/pantsbuild/testproject/buildrefactor/x:X',
'tmp:tmp'])

self.assertIn('FAILURE: More than one target specified:', print_target_multiple_targets_run.stdout_data or
print_target_multiple_targets_run.stderr_data)