src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py (Diff revision 1)

Maybe I'm misreading it, but the code and regex appear to indicate the left-hand-side is classnames?

1. 

   David Turner Oct. 30, 2014, 1:09 p.m.

   Oops, yeah, copied that from the ticket rather than what's actually done.

src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py (Diff revision 1)

Show all issues

Does this fp get closed? maybe use with?

1. 

   David Turner Oct. 30, 2014, 1:09 p.m.

   fps are automatically closed when they are garbage collected, which this one will be immediately -- but with is still a better practice.

2. 

   Patrick Lawson Oct. 30, 2014, 1:56 p.m.

   This should be using a context manager, i.e. with open(filename, 'rb') as f: ....  There's no excuse not to--do not rely on the GC behavior.

src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py (Diff revision 1)

Is it safe to assume that all resource paths returned here will always be contained within the artifact_root?

1. 

   David Turner Oct. 30, 2014, 1:09 p.m.

   Yes.  That's because anything that writes files can write them to the artifact root, and if it can, then it should.

Tests?

src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py (Diff revision 1)

Just a little terminology clarification.  I think you mean "annotation processors" here instead of "APTs"
APT stands for Annotation Processing Tool, which is a standalone task to run Annotation Processors.  'apt' as a standalone tool is obsolete as of Java 7 and javac can just run annotation processors by itself.
http://docs.oracle.com/javase/7/docs/technotes/guides/apt/
This protocol is only implemented to my knowledge by com.twitter.tools.args.apt.CmdLineProcessor and it isn't enabled by default in OSS pants. You might want to mention that here.
https://github.com/twitter/commons/blob/71a06f9c15f0bd2db67a63b32440278543864f18/src/java/com/twitter/common/args/apt/CmdLineProcessor.java

1. 

   David Turner Oct. 30, 2014, 1:09 p.m.

   Thanks.

the approach seems plausible to me, will wait for tests to do a 2nd pass.

src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py (Diff revision 1)

Show all issues

early exit here would clean this up instead of the whole logic being in an if condition

src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py (Diff revision 1)

Show all issues

add a check here for if line contains a space

1. 

   David Turner Oct. 30, 2014, 1:09 p.m.

   We'll get a ValueError if it doesn't, which we catch.

src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py (Diff revision 1)

Show all issues

lets log this error - its unused right now.

1. 

   David Turner Oct. 30, 2014, 1:09 p.m.

   Will do.

src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py (Diff revision 1)

Show all issues

This is a pretty monsterous loop.  It should be broken up a bit if at all possible.

src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py (Diff revision 1)

Show all issues

This whole parsing loop seems like it should be broken out into a helper class that just takes the full string contents of the file and does the parsing in the constructor or a factory classmethod.

src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py (Diff revision 1)

Show all issues

Ditto below--shouldn't this just hard fail?

src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py (Diff revision 1)

Show all issues

Isn't this an error condition?

src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py (Diff revision 1)

Show all issues

What is this for exactly? This duplicates information in jmake's pdb file, no?

1. 

   David Turner Oct. 30, 2014, 3:44 p.m.

   https://github.com/pantsbuild/pants/issues/132 seems to indicate that it does not do so.  That is, that jmake does not collect information about which additional output files are created by annotation processors; even if it did so, it could not know which class files are responsible for which such output files.  But we need to know about these additional files (and where they come from) so that we can create correct artifacts.

2. 

   Benjy Weinberger Oct. 30, 2014, 3:51 p.m.

   Ah great, thanks for the extra context. I remember this now.  It wasn't clear from the comments in the code that this is exclusively for any extra files created by the annotation processor. There's some confusion between those files and the META-INF/compiler/resource-mappings files describing them.

3. 

   David Turner Oct. 30, 2014, 4:05 p.m.

   I'll clarify that.

src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py (Diff revision 1)

Show all issues

Seems like a bad idea to continue? I prefer a hard crash on such errors, since we expect them to never happen.

1. 

   David Turner Oct. 30, 2014, 4:04 p.m.

   Will fix this.

General comment on the design: I don't think write_to_artifact_cache is the right place for this. For example, it doesn't address bundling these files when creaing jars.
We already have a general mechanism for this sort of thing, i.e., the 'products' mechanism. In fact we already propagate target->resources mappings under the 'resources_by_target' product (e.g., so they can get bundled into relevant jars).
It would uniform to simply add these extra mappings into that product, and ensure that the artifact cache picks them up from there when writing.
This way you get the right bundling behavior for free.

src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py (Diff revision 2)

nit: jvm_compile.py is getting pretty big.    It seems like ResourceMapping could be a standalone module.

1. 

   David Turner Nov. 17, 2014, 11:39 a.m.

   Will do.

src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py (Diff revision 2)

it would be nice to add a unit test that could exercise these conditions.

1. 

   David Turner Nov. 17, 2014, 11:39 a.m.

   I'm going to skip unit testing these errors.  I think we are unlikely to see them in practice, as these are machine-generated files.  I think that most bad output will be egregiously bad and will definitely trigger some sort of error (rather than silently doing the wrong thing).  And if we do see an error here, we'll either get the expected exception, or some other exception pointing to this spot in the code.  So I don't think it will hurt debugging.  That said, there is a bug in this code (var name e vs error), so I'll fix that.

testprojects/src/java/com/pants/testproject/annotation/example/Example.java (Diff revision 2)

I think this is irrelevant for testing resource mapping.

1. 

   David Turner Nov. 17, 2014, 11:39 a.m.

   Yeah, we can get away with some built-in annotation.

testprojects/src/java/com/pants/testproject/annotation/main/BUILD (Diff revision 2)

I think this is really a test project for the resource mapping feature.

1. 

   David Turner Nov. 17, 2014, 11:39 a.m.

   will fix.

testprojects/src/java/com/pants/testproject/annotation/processor/ExampleProcessor.java (Diff revision 2)

This looks like you copied examples/src/java/pants/examples/annotation/processor/ExampleProcessor.java and added a few lines of code to it.

1) Could you rename it? ResourceMappingProcessor would be a better name.
2) Could you rip out the stuff from ExampleProcessor that is irrelevant?
3) Could you update the javadoc to explain what this processor is used for?

1. 

   David Turner Nov. 17, 2014, 11:39 a.m.

   Will rename, remove irrelevant stuff, and update javadoc.

src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py (Diff revision 2)

Show all issues

If this Task requires resources_by_target then you should add this as a requirement in prepare(..) as round_manager.require_date('resources_by_target')
and then you wont need/shoudnt create it here. In fact, a creation here actually indicates that this Task returns a resources_by_target (which should in turn be reflected in product_types(..)) which is not your intention afaict.

1. 

   David Turner Nov. 17, 2014, 9:53 a.m.

   Actually, I think my intention is to create resources_by_target, so I'll fix that.

I may be missing the point of this change, so I'm not sure if my comments make sense.  Is the idea to create dependencies on resources without actually declaring them as resources in BUILD files?  How are these resources created?

testprojects/src/java/com/pants/testproject/annotation/main/Main.java (Diff revision 3)

This project doesn't really test annotations in general, it is testing your resource mapping logic, which could be created in other ways than using an annotation processor.  How about putting it in
testprojects/src/java/com/pants/testproject/resourcemapping/...

testprojects/src/java/com/pants/testproject/annotation/main/Main.java (Diff revision 3)

I don't think you want to use this, you want to make up a new annotation, like @Resource("com/pants/testproject/resourcemapping/foo.txt")

testprojects/src/java/com/pants/testproject/annotation/processor/BUILD (Diff revision 3)

I'd suggest creating a static resource and adding a static dependency here.

1. 

   Eric Ayers Nov. 18, 2014, 11:10 a.m.

   (never mind, that is not the problem we're trying to solve.)

testprojects/src/java/com/pants/testproject/annotation/processor/ResourceMappingProcessor.java (Diff revision 3)

I think you could simplify this code some more.
I don't know what the internal Twitter code looks like, but I doubt it is the annotation processor that is creating the file.  I'll bet that Twitter has an internal annotation that takes a resource path as an argument.  I'm thinking that is the file you should be referencing in your mapping, not a file created by the annotation processor.

1. 

   Eric Ayers Nov. 18, 2014, 11:10 a.m.

   (never mind, this should be OK)

testprojects/src/java/com/pants/testproject/annotation/processor/ResourceMappingProcessor.java (Diff revision 3)

Don't use 'Deprecated' here, create a new annotation that takes a filename argument.

1. 

   Eric Ayers Nov. 18, 2014, 11:10 a.m.

   I still think you should use a new annotation and not co-opt Deprecated, which doesn't make a lot of sense.

testprojects/src/java/com/pants/testproject/annotation/processor/ResourceMappingProcessor.java (Diff revision 3)

You should be able to get the filename from the annotations passed.  If you don't want to do this, then justify it and add a big comment explaining what's going on.

1. 

   Eric Ayers Nov. 18, 2014, 11:10 a.m.

   Maybe the annotation doesn't need to include the filename.  You could make the annotation named "GenerateExampleResource" and then use the package name of the class for the path of the resource to generate.  I think then this test processor would make a little more sense.

testprojects/src/java/com/pants/testproject/annotation/processor/ResourceMappingProcessor.java (Diff revision 3)

The last version of this had a bunch of code that looked over the annotated classes and did stuff with them, but you thought that was too complicated.  So I ripped it all out and replaced it with something trivial.  The point of this Java code isn't to be useful in any way; it's to demonstrate that if an annotation processor creates a resource resource and a resource mapping, then pants can process it.  So it doesn't really matter what this code does or how it does it; the same bits of pants are going to be tested.

From this perspective, re-using Deprecated is not crazy; I could imagine having an annotation processor that creates a "deprecation report" (e.g. so that you could interrogate your JAR files to see which of them are using deprecated APIs).

1. 

   Eric Ayers Nov. 18, 2014, 12:35 p.m.

   Sorry, I'm not trying to send you running around in circles. Its just that it is very hard to link the original report to what's being done in this annotation processor. 
   I think we could fix this just by adding comments and updating the names to make the use case more clear.  Your example makes sense to me, so maybe  change 'example.txt' (strangely, the same resource created by another annotation processor in the codebase) to 'deprecation_report.txt' and put the names of the classes with the annotation found in it.  Giving the generated resources a static name and path doesn't make clear what the original problem was, which I think is that you can't predict the name and path of resources generated by the processor.

Ship It!

Ship It!