Print Target Definition Using Buildozer #5142

denalimarsh · 2017-11-29T03:18:29Z

Problem

What if a developer wanted to check a specified directory's BUILD file for a named target? They would have to navigate to the directory's BUILD file and then manually search the file for the target name.

Solution

The solution adds a pants goal called 'print-target' to the buildrefactor module. This allows us to peek a BUILD file for a named target, printing the target's rule if the target's name is found. Additionally, print-target allows a line_number flag which will print the starting line of the named target if it is found in the BUILD file.

In order to accomplish this, a suppress warnings feature was also added to Buildozer.

Print Target is a console task, an upgrade which utilizes method return_buildozer_output() in updated buildozer.pyfrom related the Pants Meta Relocate Using Buildozer Pull Request (#5175).

To Verify:

Example:
$ ./pants print-target
--line-number path/to/directory:target_name

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

This addresses:
#4861

illicitonion

Thanks for putting this together! :)

illicitonion · 2017-11-29T10:22:40Z

contrib/buildrefactor/src/python/pants/contrib/buildrefactor/buildozer.py


  @classmethod
-  def _execute_buildozer_command(cls, buildozer_command):
+  def _execute_buildozer_command(cls, buildozer_command, supress_warnings):


nit: s/supress/suppress/

Updated to suppress_warnings in peek.py and buildozer.py.

illicitonion · 2017-11-29T10:38:38Z

contrib/buildrefactor/src/python/pants/contrib/buildrefactor/peek.py

+# Copyright 2017 Pants project contributors (see CONTRIBUTORS.md).
+# Licensed under the Apache License, Version 2.0 (see LICENSE).
+
+#./pants peek --path=src/scala/org/pantsbuild/zinc/analysis --target-name=zinc-analysis


Remove these comments, or replace them with the valid syntax (./pants peek --line_number=true src/scala/org/pantsbuild/zinc/analysis:zinc-analysis)

(Probably easiest to just remove them; I think CI is failing because they're here too: https://travis-ci.org/pantsbuild/pants/jobs/308864341 )

Removed the comments for CI. Is it relevant to include an example in the description? I noticed some Pants tasks have them but most do not.

illicitonion · 2017-11-29T10:54:03Z

contrib/buildrefactor/src/python/pants/contrib/buildrefactor/peek.py

+
+
+class Peek(Task):
+    """Peek a build file for a specified target


Can you write down here whether the functionality here tells you the literal value in the BUILD file, or tells you the value that pants will actually use, or something else? Each of these options are potentially useful, but noting which one we expect is handy.

For instance, in the following:

dir$ cat BUILD java_library( dependencies = [ "//foo:bar", "//baz:bash", ], ) dir$ ls -1 BUILD Foo.java

there are a bunch of things which Pants will internally do which aren't seen in the BUILD file. Some examples:

The name attribute will be populated with the name of the directory containing the BUILD file. Say the directory is named "dir"; should peek return "dir", "", some special value which denote "missing", some special value which denotes "computed", or an error?

The srcs attribute will automatically be expanded to globs("*.java"). Should peek error / return "" (because it's absent), return globs("*.java") because that's what's in the BUILD file, or call the function globs and return the list of java files which it actually matches (i.e. ["Foo.java"])?

Ordering isn't significant in the dependencies attribute, and Pants internally will sort the dependencies attribute. Should we return the unsorted list (because that's what in the BUILD file), or a sorted version (because that's what Pants will actually use, and it makes comparing two targets easier)?

@wisechengyi I don't know what you're expecting the answers to these questions to be either, or whether you've had this discussion already :)

The current behavior is to print the literal content in BUILD file and not what Pants sees, so just

java_library( dependencies = [ "//foo:bar", "//baz:bash", ], )

which is fine I think.

Totally fine, but worth writing down in the help text :)

baroquebobcat · 2017-11-29T21:05:24Z

contrib/buildrefactor/src/python/pants/contrib/buildrefactor/peek.py

+    def register_options(cls, register):
+        super(Peek, cls).register_options(register)
+
+        register('--line_number', help='Prints the starting line number of the named target.', type=bool, default=False)


nit: specify this as --line-number

Also, you can pass a bool flag as just the flag name, --line-number. You don't need the =true, so it'd be good to update the examples using that syntax.

baroquebobcat · 2017-11-29T21:07:24Z

contrib/buildrefactor/src/python/pants/contrib/buildrefactor/buildozer.py

    binary = binary if binary else BinaryUtil.Factory.create().select_binary('scripts/buildozer', version, 'buildozer')

-    Buildozer._execute_buildozer_command([binary, command, spec])
+    Buildozer._execute_buildozer_command([binary, command, spec], supress_warnings)


s/supress/suppress/

baroquebobcat · 2017-11-29T21:10:54Z

contrib/buildrefactor/src/python/pants/contrib/buildrefactor/peek.py

+
+            if self.options.line_number:
+                print('\n\'{}\' starts on line number:'.format(root.name))
+                Buildozer.execute_binary('print startline', address_spec, suppress_warnings=True)


If the line number were moved above the print name call, would that cause problems? I think that moving it would improve the output.

'foo:target' starts on line number: foo/BUILD:12 my_target(name='target', kwarg=1, kwarg2=1 )

seems better to me than

my_target(name='target', kwarg=1, kwarg2=1 ) 'foo:target' starts on line number: foo/BUILD:12

Updated to incorporate the basics of this proposal - although just the target's name and associated line number is still currently being printed, not the entire rule definition. Depending on the level of complexity we're looking for with the peek functionality, printing the entire rule definition may be possible using the Buildozer command 'print rule' described here: https://github.com/bazelbuild/buildtools/tree/master/buildozer

Pretty printing is made difficult by the Buildozer executable which operates as a black hole, with command 'print name' always printing '\ntarget_name\n' and commands 'print startline' and 'print endline' always printing '\ninteger_value\n'.

baroquebobcat · 2017-11-29T21:11:41Z

contrib/buildrefactor/src/python/pants/contrib/buildrefactor/buildozer.py

    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))
+        if not supress_warnings:


s/supress/suppress/

baroquebobcat · 2017-11-29T21:16:19Z

contrib/buildrefactor/src/python/pants/contrib/buildrefactor/peek.py

+from pants.contrib.buildrefactor.buildozer import Buildozer
+
+
+class Peek(Task):


Could you make this a subclass of ConsoleTask? I think the UX would be better.

A good example of ConsoleTask usage is ListRoots

Yes, I thought the same and tried to implement it as a ConsoleTask, but I have a bug. Here's the current working branch with changes to peek.py and test_peek_integration.py: https://github.com/denalimarsh/pants/tree/peek-console-task

The ConsoleTask version of peek seems to work initially, successfully printing the target's name and line number - but directly afterwards it fails with error message:
for value in self.console_output(targets):
Exception message: 'NoneType' object is not iterable

@denalimarsh consoletask's output should be in the return (or yield in some cases).

pants/src/python/pants/core_tasks/roots.py

Line 17 in d30cca1

yield '{}: {}'.format(src_root.path, all_langs or '*')

, so don't print anything in def console_output, rather, make it a return value.

Exception message: 'NoneType' object is not iterable is because it is expecting a return value

Thanks for the fix! Implemented on branch peek-console-task.

Hm. In the branch, it now returns an empty string. I think if you were to change it so that it doesn't return, but instead yields where it prints, it'll work.

ie

print('\'{}\' found in BUILD file.\n'.format(root_name))

becomes

yield '\'{}\' found in BUILD file.\n'.format(root_name)

If you add the yields, the return will be unnecessary because the method will act like a generator. Does that make sense?

wisechengyi · 2017-11-30T05:18:09Z

contrib/buildrefactor/src/python/pants/contrib/buildrefactor/peek.py

+      print('\n\'{}\' ends on line number:'.format(root.name))
+      Buildozer.execute_binary('print endline', address_spec, suppress_warnings=True)
+    else:
+      Buildozer.execute_binary('print name', address_spec, suppress_warnings=True)


The buildozer arg here should be print rule

$ buildozer 'print rule' examples/tests/java/org/pantsbuild/example/hello/greet:greet junit_tests( dependencies = [ "examples/src/java/org/pantsbuild/example/hello/greet", "examples/src/resources/org/pantsbuild/example/hello", ], )

so the method should be renamed to print_target as well

Implemented as ConsoleTask, updated to 'print rule', and renamed to 'print_target' on branch peek-console-task. If the changes look good I'll either update this PR or open a new 'Print Target' PR.

Updating this one would be good. Thanks.

Also make sure there is no print function left in the code, because all the console output should be the return value.

Updated to use yield instead of print

wisechengyi

Thanks @denalimarsh please see comments below

wisechengyi · 2017-12-06T18:48:45Z

contrib/buildrefactor/src/python/pants/contrib/buildrefactor/buildozer.py

+        raise TaskError('{} ... exited non-zero ({}).'.format(buildozer_command, err.returncode))
+
+  @classmethod
+  def return_buildozer_output(cls, command, spec, binary=None, version='0.6.0.dce8b3c287652cbcaf43c8dd076b3f48c92ab44c', suppress_warnings=False):


Ah I made the same comments with @itsedvard's patch
#5175 (comment)
#5175 (comment)

wisechengyi · 2017-12-06T18:49:21Z

...buildrefactor/tests/python/pants_test/contrib/buildrefactor/test_print_target_integration.py

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


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

Updated to include additional test test_print_invalid_name.

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.

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.

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

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

Thanks for the fix!

wisechengyi · 2017-12-07T02:35:06Z

contrib/buildrefactor/src/python/pants/contrib/buildrefactor/buildozer.py

-    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')


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.

pants/src/python/pants/ivy/bootstrapper.py

Line 65 in 0450732

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 :)

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.

wisechengyi · 2017-12-07T02:37:05Z

contrib/buildrefactor/src/python/pants/contrib/buildrefactor/buildozer.py

+          raise TaskError('{} ... exited non-zero ({}).'.format(buildozer_command, err.returncode))
+    else:
+      try:
+        subprocess.check_output(buildozer_command, cwd=get_buildroot(), stderr=subprocess.STDOUT)


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.

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

This adds the ability to peek a BUILD file for a named target, printing the target name if it is found.Additionally, peek allows a line_number flag which will print the starting line of the named target if it is found in the BUILD file. Also, this adds a suppress warnings feature to Buildozer. Example: $ ./pants peek --line_number=true path/to/directory:target_name To Verify: $ ./pants test contrib/buildrefactor/tests/python/pants_test/contrib/buildrefactor:peek_integration This addresses: pantsbuild#4861 Please enter the commit message for your changes. Lines starting

Eric-Arellano · 2020-05-17T22:17:48Z

Closing due to being stale.

denalimarsh force-pushed the peek-buildozer branch from 4fb4415 to 11da0ca Compare November 29, 2017 08:48

illicitonion reviewed Nov 29, 2017

View reviewed changes

illicitonion requested a review from wisechengyi November 29, 2017 10:57

denalimarsh force-pushed the peek-buildozer branch from 11da0ca to 4d83501 Compare November 29, 2017 19:41

denalimarsh mentioned this pull request Nov 29, 2017

Pants peek goal #4861

Closed

baroquebobcat reviewed Nov 29, 2017

View reviewed changes

denalimarsh force-pushed the peek-buildozer branch from 4d83501 to 293c5cc Compare November 30, 2017 01:55

wisechengyi reviewed Nov 30, 2017

View reviewed changes

denalimarsh force-pushed the peek-buildozer branch from 293c5cc to 4ffe0bc Compare December 6, 2017 03:59

denalimarsh changed the title ~~Peek Target using Buildozer~~ Print Target definition using Buildozer Dec 6, 2017

denalimarsh changed the title ~~Print Target definition using Buildozer~~ Peek Target using Buildozer Dec 6, 2017

denalimarsh changed the title ~~Peek Target using Buildozer~~ Print target definition using Buildozer Dec 6, 2017

denalimarsh changed the title ~~Print target definition using Buildozer~~ Print Target Definition Using Buildozer Dec 6, 2017

wisechengyi reviewed Dec 6, 2017

View reviewed changes

denalimarsh force-pushed the peek-buildozer branch 5 times, most recently from e88bce7 to 1d25d15 Compare December 7, 2017 01:26

wisechengyi requested changes Dec 7, 2017

View reviewed changes

denalimarsh force-pushed the peek-buildozer branch from 1d25d15 to 6d50ec6 Compare December 7, 2017 05:15

denalimarsh force-pushed the peek-buildozer branch from 6d50ec6 to b33d0fd Compare December 7, 2017 23:41

stuhood force-pushed the master branch from b6bb42d to 9e2fdb5 Compare May 11, 2018 23:54

Eric-Arellano closed this May 17, 2020



		class Peek(Task):
		"""Peek a build file for a specified target

		from pants.contrib.buildrefactor.buildozer import Buildozer


		class Peek(Task):


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

Print Target Definition Using Buildozer #5142

Print Target Definition Using Buildozer #5142

Conversation

denalimarsh commented Nov 29, 2017 • edited Loading

Problem

Solution

To Verify:

illicitonion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denalimarsh Nov 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denalimarsh Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denalimarsh Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

wisechengyi Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wisechengyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denalimarsh Dec 6, 2017 • edited Loading

Choose a reason for hiding this comment

denalimarsh Dec 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eric-Arellano commented May 17, 2020

denalimarsh commented Nov 29, 2017 •

edited

Loading

denalimarsh Nov 29, 2017 •

edited

Loading

denalimarsh Nov 30, 2017 •

edited

Loading

denalimarsh Nov 30, 2017 •

edited

Loading

wisechengyi Nov 30, 2017 •

edited

Loading

denalimarsh Dec 6, 2017 •

edited

Loading

denalimarsh Dec 7, 2017 •

edited

Loading