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

Add `buildrefactor` to `contrib` and buildozer goal #4921

Merged
merged 4 commits into from Oct 18, 2017

Conversation

Projects
None yet
3 participants
@15Dkatz
Copy link
Contributor

15Dkatz commented Oct 1, 2017

@wisechengyi @baroquebobcat @ity

Problem

Directly editing and manipulating files is somewhat manual.

Solution

Rather than an overwrite-file solution, we can use Bazel's open source buildozer tool. This code change adds support for removing and adding dependencies with the buildozer binary.

Behavior:

  1. ./pants buildozer --add-dependencies=<dependencies>
    will add the dependency to the context's relative BUILD file.

    Example: ./pants buildozer --add-dependencies='a/b b/c' //tmp:tmp

  2. ./pants buildozer --remove-dependencies=<dependencies>
    will remove the dependency from the context's BUILD file.

    Example: ./pants buildozer --remove-dependencies='a/b b/c' //tmp:tmp

The buildozer binary will be created and hosted under pantsbuild/binaries:
pantsbuild/binaries#42

Original buildozer code:
https://github.com/bazelbuild/buildtools/tree/master/buildozer

To Verify

Run examples:
./pants buildozer --add-dependencies='a/b e/d' //tmp:tmp
./pants buildozer --remove-dependencies=a/b/c //tmp:tmp
(Note: this assumes a tmp/ directory with a valid BUILD file exists in the root of pants)

Run the tests:
./pants test contrib/buildrefactor/tests/python/pants_test/contrib/buildrefactor:buildozer

Follow Ups:

  • Add a custom --command option for full buildozer interaction.
  • Use this tool to refactor target-rename: #4882
  • Hack buildozer to recognize targets that have no name field in the BUILD file.
from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
unicode_literals, with_statement)

import subprocess

This comment has been minimized.

@kwlzn

kwlzn Oct 1, 2017

Member

this should use from pants.util.process_handler import subprocess instead of the stdlib variant

from pants.task.task import Task


class Buildozer(Task):

This comment has been minimized.

@kwlzn

kwlzn Oct 1, 2017

Member

src/python/pants/backend/python pertains to the python language support in pants, so it probably isn't the best place for this code.

how about sticking this into a new contrib plugin (e.g. pantsbuild.pants.contrib.buildozer)? or worst case a new dir in src/python/pants/backend/....

This comment has been minimized.

@wisechengyi

wisechengyi Oct 2, 2017

Contributor

+1 on contrib


def execute_buildozer_script(self, command):
buildozer_command = [
BinaryUtil.Factory.create().select_script('scripts/buildozer', self.options.version, 'buildozer'),

This comment has been minimized.

@kwlzn

kwlzn Oct 1, 2017

Member

how about calling select_script() once in __init__ and then storing that as e.g. self._executable for reuse here?

This comment has been minimized.

@wisechengyi

wisechengyi Oct 3, 2017

Contributor

Related to pantsbuild/binaries#42, it seems that the buildozer go binary isn't platform independent, i.e. the binary in that PR only works on mac, so it is not a script anymore.

Therefore it would be more appropriate to use pants.binaries.binary_util.BinaryUtil#select_binary since it will factor the platform name into the download path.

illicitonion added a commit to twitter/pants that referenced this pull request Oct 2, 2017

Error if the wrong subprocess is imported
kwlzn flagged this as an error in
pantsbuild#4921 - rather than relying on
manual review to catch the problem in the future, give a pre-commit
error with a suggested replacement.

Example error on failure:

```
$ git commit -a
* Checking native_engine_version: verified: 5d5266579286db1f1fa254f383571b3c5031b97b
* Checking packages
* Checking headers
* Checking for banned imports
Found forbidden imports. Instead of `import subprocess` you should `from pants.util.process_handler import subprocess`. Bad files:
src/python/pants/backend/codegen/protobuf/java/protobuf_gen.py
```

kwlzn added a commit that referenced this pull request Oct 2, 2017

Error if the wrong subprocess is imported (#4922)
@kwlzn flagged this as an error in
#4921 - rather than relying on
manual review to catch the problem in the future, give a pre-commit
error with a suggested replacement.

Example error on failure:

$ git commit -a
* Checking native_engine_version: verified: 5d5266579286db1f1fa254f383571b3c5031b97b
* Checking packages
* Checking headers
* Checking for banned imports
Found forbidden imports. Instead of `import subprocess` you should `from pants.util.process_handler import subprocess`. Bad files:
src/python/pants/backend/codegen/protobuf/java/protobuf_gen.py
@wisechengyi
Copy link
Contributor

wisechengyi left a comment

Test is broken, seems unrelated to binaries util as I already configured it to be a local path.

./pants test tests/python/pants_test/backend/python/buildozer/:
...
dozer.xml 
                     ==================== FAILURES ====================
                     _______ BuildozerTest.test_add_dependency ________
                     
                     self = <pants_test.backend.python.buildozer.test_buildozer.BuildozerTest testMethod=test_add_dependency>
                     
                         def test_add_dependency(self):
                           mock_dependency = '/a/b/c'
                           build_file = self.build_root + '/b/BUILD'
                         
                           self._clean_build_file(build_file)
                     >     self._test_buildozer_execution({ 'add': mock_dependency, 'location': '//b:b' })
                     
                     .pants.d/pyprep/sources/66b5df6fdd7e680f5857397234a11c434fd1e5dd/pants_test/backend/python/buildozer/test_buildozer.py:37: 
                     _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
                     .pants.d/pyprep/sources/66b5df6fdd7e680f5857397234a11c434fd1e5dd/pants_test/backend/python/buildozer/test_buildozer.py:50: in _test_buildozer_execution
                         self.create_task(self.context(target_roots=self.targets)).execute()
                     src/python/pants/backend/python/buildozer/buildozer.py:42: in execute
                         self.add_dependency()
                     src/python/pants/backend/python/buildozer/buildozer.py:48: in add_dependency
                         self.execute_buildozer_script('add dependencies ' + self.options.add)
                     src/python/pants/backend/python/buildozer/buildozer.py:55: in execute_buildozer_script
                         BinaryUtil.Factory.create().select_script('scripts/buildozer', self.options.version, 'buildozer'),
                     .pants.d/pyprep/sources/66b5df6fdd7e680f5857397234a11c434fd1e5dd/pants/binaries/binary_util.py:67: in create
                         options.binaries_baseurls,
                     _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
                     
                     self = <pants_test.option.util.fakes._FakeOptionValues object at 0x10702fbd0>
                     key = 'binaries_baseurls'
                     
                         def __getattr__(self, key):
                           try:
                             value = self._option_values[key]
                           except KeyError:
                             # Instead of letting KeyError raise here, re-raise an AttributeError to not break getattr().
                     >       raise AttributeError(key)
                     E       AttributeError: binaries_baseurls
                     
                     .pants.d/pyprep/sources/66b5df6fdd7e680f5857397234a11c434fd1e5dd/pants_test/option/util/fakes.py:35: AttributeError
register('--version', advanced=True, fingerprint=True, default='0.4.5', help='Version of buildozer.')
register('--add', type=str, advanced=True, default=None, help='The dependency to add')
register('--remove', type=str, advanced=True, default=None, help='The dependency to remove')
register('--location', type=str, advanced=True, default=None, help='The target location')

This comment has been minimized.

@wisechengyi

wisechengyi Oct 3, 2017

Contributor

As mentioned in the meeting, we can just use self.context.target_roots instead of --location, e.g.

./pants buildozer --add=abc examples/tests/java/org/pantsbuild/example/hello/greet:greet
@classmethod
def register_options(cls, register):
register('--version', advanced=True, fingerprint=True, default='0.4.5', help='Version of buildozer.')
register('--add', type=str, advanced=True, default=None, help='The dependency to add')

This comment has been minimized.

@wisechengyi

wisechengyi Oct 3, 2017

Contributor

This should be a basic option for buildozer, so advanced=False for all options in this task.

Name can be more descriptive, like --add-dependencies as it can specify multiple dependencies separated by space; same for removal. You can put an example in the help message.

self.remove_dependency()

def add_dependency(self):
self.execute_buildozer_script('add dependencies ' + self.options.add)

This comment has been minimized.

@wisechengyi

wisechengyi Oct 3, 2017

Contributor

the conventional way is to use format, e.g. 'add dependencies {}'.format(self.options.add), same for other instances.

@wisechengyi wisechengyi referenced this pull request Oct 3, 2017

Merged

Add buildozer 0.4.5 #42

@15Dkatz 15Dkatz force-pushed the 15Dkatz:buildozer-goal branch 2 times, most recently from 9e8cbbc to 348623b Oct 4, 2017

@15Dkatz 15Dkatz changed the title Add buildozer goal Add buildozer goal to contrib Oct 4, 2017

@15Dkatz 15Dkatz changed the title Add buildozer goal to contrib Add `buildozer` to `contrib` Oct 4, 2017

@15Dkatz

This comment has been minimized.

Copy link
Contributor

15Dkatz commented Oct 4, 2017

💇‍♂️
@kwlzn @wisechengyi
Moved buildozer to the contrib path as a plugin for pants. Also now using pant's subprocess rather than the stdlib subprocess.

To get the tests to pass while the binary is still being accepted:

  1. buildozer.py line 44 needs to be commented out or deleted:
    #self._executable = BinaryUtil.Factory.create().select_binary('scripts/buildozer', self.options.version, 'buildozer')

  2. And then line 60 needs to replace self._executable with a hardcoded path to the buildozer binary for example /Users/davidkatz/buildozer:
    buildozer_command = ['/Users/davidkatz/buildozer', command, '//{}:{}'.format(self.address._spec_path, self.address._target_name)]

@wisechengyi
Copy link
Contributor

wisechengyi left a comment

Thanks @15Dkatz. Looks mostly good!

nit: could you rename the module to buildrefactor, i.e. contrib/buildrefactor/src/python/...? as buildozer will be one of the goals, and meta-rename later will be another goal under this module.

  • hooking up pants binary as you mentioned in the comment.
super(Buildozer, self).__init__(*args, **kwargs)

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

This comment has been minimized.

@wisechengyi

wisechengyi Oct 4, 2017

Contributor
  1. We want to run buildozer against all specified target roots, not just the first one. buildozer allows your to append targets as well.
buildozer 'add dependencies 456' examples/tests/java/org/pantsbuild/example/hello/greet:greet examples/tests/java/org/pantsbuild/example/useproto:proto
  1. self.address as a instance variable seems unnecessary because it is only used by one other function. so it would be more clear to pass it as an argument to _execute_buildozer_script or just have _execute_buildozer_script to do the deeds.
"""Enables interaction with the Buildozer Go binary
Behavior:
1. `./pants buildozer --add-dependencies=<dependencies> --location=<directory>`

This comment has been minimized.

@wisechengyi

wisechengyi Oct 4, 2017

Contributor

remove --location in doc

@wisechengyi

This comment has been minimized.

Copy link
Contributor

wisechengyi commented Oct 4, 2017

Also let me clarify the process a bit. Once you have the select_binary working locally

  1. push the changes here
  2. update pantsbuild/binaries#42

Then I will go verify the binaries with this PR locally. If all works well, I will merge pantsbuild/binaries#42, sync it with s3, then trigger a rebuild for this PR, which then should work and be merged.

@15Dkatz 15Dkatz changed the title Add `buildozer` to `contrib` Add `buildrefactor` to `contrib` Oct 5, 2017

@15Dkatz 15Dkatz changed the title Add `buildrefactor` to `contrib` Add `buildrefactor` to `contrib` and buildozer goal Oct 5, 2017

@15Dkatz 15Dkatz force-pushed the 15Dkatz:buildozer-goal branch from a83b09c to 0bad0e2 Oct 5, 2017

@15Dkatz

This comment has been minimized.

Copy link
Contributor

15Dkatz commented Oct 5, 2017

💇‍♂️ @wisechengyi
Renamed the plugin to buildrefactor. Also now supporting and testing multiple dependency addition and removal with the buildozer goal.

@michaelttran will update pantsbuild/binaries#42 to support the different mac versions as we discussed.

@wisechengyi

This comment has been minimized.

Copy link
Contributor

wisechengyi commented Oct 5, 2017

sounds good! for ci to pass after pantsbuild binaries got merged, a linux version is also needed, which can be found at https://github.com/bazelbuild/buildtools/releases as well

@wisechengyi

This comment has been minimized.

Copy link
Contributor

wisechengyi commented Oct 10, 2017

Related to pantsbuild/binaries#42, please also bring up the test up to date with the binaryutil subsystem, so something like below should pass locally.

PANTS_BINARIES_BASEURLS="['file:///Users/yic/workspace/binaries/build-support/']"  ./pants test contrib/buildrefactor/tests/python/pants_test/contrib/buildrefactor:buildozer

15Dkatz added some commits Sep 30, 2017

Add buildozer goal
This adds a Pants Task that wraps around the buildozer binary.

Behavior:
1. `./pants buildozer --add=<dependency> --location=<directory>`
    will add the dependency to the location's relative BUILD file.
2. `./pants buildozer --remove=<dependency> --location=<directory>`
    will remove the dependency from the location's relative BUILD file.

Note that buildozer assumes that BUILD files contain a name field for the target.

Resources:
The buildozer binary will be created and hosted under pantsbuild/binaries:
pantsbuild/binaries#42

Original buildozer code:
https://github.com/bazelbuild/buildtools/tree/master/buildozer
Test the buildozer goal
This adds a tests around the buildozer goal by testing the add and remove
functionality.
Move `buildozer` to the contrib module
This moves the `buildozer` goal to the contrib module and adds it as a plugin to
pants. Note that `buildozer` should now show up as a goal in `./pants goals`.

This also removes the previous behavior of `location` and functions based of
`self.context.target_roots` instead.

Finally, select_binary is used over select_script since the `buildozer` goal
isn't platform depenendent.
Rename the `buildozer` plugin to `buildrefactor`
This moves the previous `buildozer` plugin to a more appropriately named
`buildrefactor` path. In the future, more goals besides `buildozer.py` will
exist within `buildrefactor` that also manipulate BUILD files  and targets.

This also adds additional testing for the buildozer goal to ensure that it can
add and remove multiple dependencies.

@15Dkatz 15Dkatz force-pushed the 15Dkatz:buildozer-goal branch from 0bad0e2 to 6620852 Oct 18, 2017

@wisechengyi

This comment has been minimized.

Copy link
Contributor

wisechengyi commented Oct 18, 2017

FYI the ruby shard failing is due to #4995.

@wisechengyi wisechengyi merged commit ef28af7 into pantsbuild:master Oct 18, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

15Dkatz added a commit to 15Dkatz/pants that referenced this pull request Oct 19, 2017

Add custom commands to the `buildozer` goal
This adds support for a `--command` option to the buildozer goal. This will be
particularly helpful for executing features beyond adding and removing
dependencies. For example, setting the name of an attribute, replacing an
attribute name, and printing attributes within a BUILD file are all custom
commands that can be executed now with --command.

Example to rename the name attribute to new_name in //tmp:tmp:

`./pants buildozer --command='set name new_name' //tmp:tmp`

This follows up on:
* pantsbuild#4921

This will also help out:
* pantsbuild#4882

15Dkatz added a commit to 15Dkatz/pants that referenced this pull request Oct 19, 2017

Add custom commands to the `buildozer` goal
This adds support for a `--command` option to the buildozer goal. This will be
particularly helpful for executing features beyond adding and removing
dependencies. For example, setting the name of an attribute, replacing an
attribute name, and printing attributes within a BUILD file are all custom
commands that can be executed now with --command.

Example to rename the name attribute to new_name in //tmp:tmp:

`./pants buildozer --command='set name new_name' //tmp:tmp`

This follows up on:
* pantsbuild#4921

This will also help out:
* pantsbuild#4882

15Dkatz added a commit to 15Dkatz/pants that referenced this pull request Oct 20, 2017

Add custom commands to the `buildozer` goal
This adds support for a `--command` option to the buildozer goal. This will be
particularly helpful for executing features beyond adding and removing
dependencies. For example, setting the name of an attribute, replacing an
attribute name, and printing attributes within a BUILD file are all custom
commands that can be executed now with --command.

Example to rename the name attribute to new_name in //tmp:tmp:

`./pants buildozer --command='set name new_name' //tmp:tmp`

This follows up on:
* pantsbuild#4921

This will also help out:
* pantsbuild#4882

@15Dkatz 15Dkatz deleted the 15Dkatz:buildozer-goal branch Oct 20, 2017

wisechengyi added a commit that referenced this pull request Oct 24, 2017

Add custom commands to the `buildozer` goal (#4998)
### Problem and Solution

We should take full advantage of the buildozer binary. This code change adds support for a `--command` option to the buildozer goal. This will be particularly helpful for executing features beyond adding and removing dependencies. For example, setting the name of an attribute, replacing an attribute name, and printing attributes within a BUILD file are all custom
commands that can be executed now with --command.

Example to rename the name attribute to new_name in //tmp:tmp:

`./pants buildozer --command='set name new_name' //tmp:tmp`

This follows up on:
* #4921

This will also help out:
* #4882

### To verify:
- Create tmp/BUILD and run the example: 
`./pants buildozer --command='set name new_name' //tmp:tmp`
- Run the test:
`/pants test contrib/buildrefactor/tests/python/pants_test/contrib/buildrefactor:buildozer`
- Execute any of the custom buildozer commands listed in: https://github.com/bazelbuild/buildtools/tree/master/buildozer

15Dkatz added a commit to 15Dkatz/pants that referenced this pull request Oct 26, 2017

Add custom commands to the `buildozer` goal
This adds support for a `--command` option to the buildozer goal. This will be
particularly helpful for executing features beyond adding and removing
dependencies. For example, setting the name of an attribute, replacing an
attribute name, and printing attributes within a BUILD file are all custom
commands that can be executed now with --command.

Example to rename the name attribute to new_name in //tmp:tmp:

`./pants buildozer --command='set name new_name' //tmp:tmp`

This follows up on:
* pantsbuild#4921

This will also help out:
* pantsbuild#4882

15Dkatz added a commit to 15Dkatz/pants that referenced this pull request Oct 26, 2017

Add custom commands to the `buildozer` goal
This adds support for a `--command` option to the buildozer goal. This will be
particularly helpful for executing features beyond adding and removing
dependencies. For example, setting the name of an attribute, replacing an
attribute name, and printing attributes within a BUILD file are all custom
commands that can be executed now with --command.

Example to rename the name attribute to new_name in //tmp:tmp:

`./pants buildozer --command='set name new_name' //tmp:tmp`

This follows up on:
* pantsbuild#4921

This will also help out:
* pantsbuild#4882
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment