-
-
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?
-
src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py (Diff revision 1) Does this fp get closed? maybe use
with
? -
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?
Cache files created by (specially designed) annotation processors.
Review Request #1250 — Created Oct. 29, 2014 and submitted
Information | |
---|---|
dturner-tw | |
pants | |
132 | |
41d268b... | |
Reviewers | |
pants-reviews | |
benjyw, davidt, ity, jinfeng, jsirois, peiyu, zundel |
These annotation processors (so far, only twitter.common.args) write a file into META-INF which lists all of the files they created. Make pants process that file to decide which files should go in the artifact cache.
Ran integration tests.
-
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
-
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) 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) add a check here for if line contains a space
-
src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py (Diff revision 1) lets log this error - its unused right now.
Change Summary:
(title, desc)
Summary: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
-
-
src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py (Diff revision 1) 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) 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) Ditto below--shouldn't this just hard fail?
-
src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py (Diff revision 1) Isn't this an error condition?
-
-
src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py (Diff revision 1) What is this for exactly? This duplicates information in jmake's pdb file, no?
-
src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py (Diff revision 1) Seems like a bad idea to continue? I prefer a hard crash on such errors, since we expect them to never happen.
-
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.
Change Summary:
Address comments and add tests.
-
-
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.
-
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.
-
-
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.
-
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?
-
-
src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py (Diff revision 2) 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.
Change Summary:
Address review comments
Commit: |
|
||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+353 -4)
|
-
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.
-
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.
-
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.
-
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.
-
-
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).
Change Summary:
One more try.
Commit: |
|
||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+368 -4)
|
Commit: |
|
||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+369 -5)
|