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.

Sign up for GitHub

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

Jump to bottom

reproduces redirecting java_protobuf_library through target() #121

Closed
wants to merge 1 commit into from
Closed

reproduces redirecting java_protobuf_library through target() #121

wants to merge 1 commit into from

Conversation

ericzundel
Copy link
Member

I had a case where I wanted to redirect to a java_protobuf_library() through a dependencies() target.
Should this work? This is a repro case I made in the protobuf code sample in pants.

./pants goal binary src/java/com/pants/examples/protobuf

14:27:07 00:00 [main]
               See a report at: http://localhost:57641/run/pants_run_2014_05_13_14_27_07_254
14:27:07 00:00   [setup]
14:27:07 00:00     [bootstrap]
14:27:07 00:00     [parse]
14:27:07 00:00   [jvm-locate]
14:27:07 00:00   [jvm-locate]
14:27:07 00:00   [jvm-locate]
14:27:07 00:00   [jvm-locate]
14:27:07 00:00   [jvm-locate]
14:27:07 00:00   [jvm-locate]
14:27:07 00:00   [bootstrap]
14:27:07 00:00     [bootstrap-jvm-tools]
14:27:07 00:00   [gen]
14:27:07 00:00     [thrift]
14:27:07 00:00     [scrooge]
14:27:07 00:00     [protoc]
                   Left with unexpected unconsumed gen targets:
                    JarLibrary(src/protobuf/com/pants/examples/distance/BUILD:distance) -> set([Pants(src/protobuf/com/pants/examples/distance/BUILD:redirect)])
                   Invalidated 1 target containing 1 source file.
               FAILURE
Traceback (most recent call last):
  File "/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/runpy.py", line 122, in _run_module_as_main
    "__main__", fname, loader, pkg_name)
  File "/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/runpy.py", line 34, in _run_code
    exec code in run_globals
  File "/Users/zundel/Src/Pants/pants.pex/__main__.py", line 24, in <module>
  File "/Users/zundel/Src/Pants/pants.pex/.bootstrap/_twitter_common_python/pex_bootstrapper.py", line 65, in bootstrap_pex
  File "/Users/zundel/Src/Pants/pants.pex/.bootstrap/_twitter_common_python/pex.py", line 215, in execute
  File "/Users/zundel/Src/Pants/pants.pex/.bootstrap/_twitter_common_python/pex.py", line 258, in execute_entry
  File "/Users/zundel/Src/Pants/pants.pex/.bootstrap/_twitter_common_python/pex.py", line 281, in execute_pkg_resources
  File "/Users/zundel/Src/Pants/pants.pex/pants/bin/pants_exe.py", line 169, in main
  File "/Users/zundel/Src/Pants/pants.pex/pants/bin/pants_exe.py", line 150, in _run
  File "/Users/zundel/Src/Pants/pants.pex/pants/commands/goal.py", line 301, in run
  File "/Users/zundel/Src/Pants/pants.pex/pants/engine/engine.py", line 49, in execute
  File "/Users/zundel/Src/Pants/pants.pex/pants/engine/group_engine.py", line 283, in attempt
  File "/Users/zundel/Src/Pants/pants.pex/pants/engine/group_engine.py", line 172, in attempt
  File "/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/contextlib.py", line 34, in __exit__
    self.gen.throw(type, value, traceback)
  File "/Users/zundel/Src/Pants/pants.pex/pants/goal/context.py", line 181, in new_workunit
  File "/Users/zundel/Src/Pants/pants.pex/pants/engine/group_engine.py", line 172, in attempt
  File "/Users/zundel/Src/Pants/pants.pex/pants/engine/group_engine.py", line 143, in execute_task
  File "/Users/zundel/Src/Pants/pants.pex/pants/tasks/code_gen.py", line 122, in execute
  File "/Users/zundel/Src/Pants/pants.pex/pants/tasks/protobuf_gen.py", line 125, in createtarget
  File "/Users/zundel/Src/Pants/pants.pex/pants/tasks/protobuf_gen.py", line 145, in _create_java_target
AttributeError: 'JarLibrary' object has no attribute 'update_dependencies'

@ericzundel
Copy link
Member Author

I fixed this by using a delegating java_protobuf_library() rule with an empty list of sources and specifying the build target I wanted to delegate in the dependencies [] argument.

@benjyw
Copy link
Sponsor Contributor

benjyw commented May 13, 2014

Glad you found a workaround, but that's a bug we should fix.

However I'll wait until Patrick's change lands, because that shakes up so
many things about targets anyway that it may fix this (or manifest in a
different way).

On Tue, May 13, 2014 at 11:35 AM, Eric Ayers notifications@github.comwrote:

I fixed this by using a delegating java_protobuf_library() rule with an
empty list of sources and specifying the build target I wanted to delegate
in the dependencies [] argument.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/121#issuecomment-42994461
.

@ericzundel
Copy link
Member Author

Rebased this change after the big refactor and checked to see that it was still failing.

@ericzundel
Copy link
Member Author

There is a similar problem when you use 'dependencies()' to point to multiple jvm_bundle() targets. Running pants goal bundle on a target that is defined with dependencies() does not actually bundle its jvm_binary() dependencies. For an example, see the protobuf example in pants:

./pants goal bundle src/java/com/pants/examples/protobuf

@ericzundel
Copy link
Member Author

This issue seems to be fixed for dependencies of java_protobuf_library in commit 293b31e

pants goal binary examples/src/java/com/pants/examples/protobuf/distance

@ericzundel ericzundel closed this Nov 13, 2014
@ericzundel ericzundel reopened this Nov 13, 2014
@ericzundel ericzundel changed the title reproduces redirecting java_protobuf_library through dependencies() reproduces redirecting java_protobuf_library through target() Nov 13, 2014
@ericzundel
Copy link
Member Author

It still doesn't work. It doesn't throw a stacktrace, but it doesn't generate code either (I was fooled because the generated code was already there). I updated the example again.

./pants goal clean-all
./pants goal run testprojects/src/java/com/pants/testproject/indirectprotodep

@ericzundel ericzundel closed this Nov 13, 2014
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 2ff3c23 on ericzundel:zundel/proto-dep-bug into b86be0e on pantsbuild:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 2ff3c23 on ericzundel:zundel/proto-dep-bug into b86be0e on pantsbuild:master.

@ericzundel
Copy link
Member Author

The bug here is in the calculation of dependees_by_gentarget(). It only looks at direct dependees of a gentarget, not considering there might be indirect references through target() (Dependencies target type).

A workaround for this issue is to add --lang=java to the command line or in pants.ini

[gen.protoc]
lang: [ "java" ]

@ericzundel ericzundel reopened this Mar 26, 2015
@coveralls
Copy link

Coverage Status

Coverage increased (+4.02%) to 68.72% when pulling 2ff3c23 on ericzundel:zundel/proto-dep-bug into b86be0e on pantsbuild:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+4.02%) to 68.72% when pulling 2ff3c23 on ericzundel:zundel/proto-dep-bug into b86be0e on pantsbuild:master.

ericzundel added a commit that referenced this pull request Mar 29, 2015
@ericzundel
Force java generation on for protobuf_gen, get rid of spurious warning
0b29805 
Testing Done:
Manually tested using the repro case in #121

I found that setting pants.ini to

```
[gen.protoc]
lang: ["java"]
```

Enables us to compile in our repo in the latest version of pants.  This allows us to get past problems that started
cropping up with the UnionDependencies change to the classpath product introduced to help with classpath hygene.

Bugs closed: 121, 1334

Reviewed at https://rbcommons.com/s/twitter/r/1994/
@ericzundel
Copy link
Member Author

Fixed in commit 44a8f0e (https://rbcommons.com/s/twitter/r/2302/) by moving off of the CodeGen superclass.

@ericzundel ericzundel closed this Jun 2, 2015
@ericzundel ericzundel deleted the zundel/proto-dep-bug branch June 23, 2015 12:46
@jasonqsong jasonqsong mentioned this pull request Apr 6, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ericzundel @benjyw @coveralls