Cache files created by (specially designed) annotation processors.

Review Request #1250 — Created Oct. 29, 2014 and submitted

dturner-tw
pants
132
41d268b...
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.

  • 0
  • 0
  • 9
  • 2
  • 11
Description From Last Updated
DA
  1. 
      
  2. Maybe I'm misreading it, but the code and regex appear to indicate the left-hand-side is classnames?

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

  3. Does this fp get closed? maybe use with?

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

    2. 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.

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

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

  5. 
      
ZU
  1. Tests?

    1. Yep, as I said above, this is a wip to validate the concept; if the concept is sound, I'll write some tests.

  2. 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

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

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

  3. add a check here for if line contains a space

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

  4. lets log this error - its unused right now.

  5. 
      
DT
PA
  1. 
      
  2. This is a pretty monsterous loop. It should be broken up a bit if at all possible.

  3. 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.

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

  5. Isn't this an error condition?

  6. 
      
BE
  1. 
      
  2. What is this for exactly? This duplicates information in jmake's pdb file, no?

    1. 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. 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. I'll clarify that.

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

  4. 
      
BE
  1. 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.

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

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

    1. 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.

  4. I think this is irrelevant for testing resource mapping.

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

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

  6. 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. Will rename, remove irrelevant stuff, and update javadoc.

  7. 
      
IT
  1. 
      
  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.

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

  3. 
      
DT
ZU
  1. 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?

    1. My apologies, I should have re-read the original issue: https://github.com/pantsbuild/pants/issues/132 Files emitted from annotation processors is exactly te use case, so some of these comments are irrelevent.

  2. 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/...

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

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

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

  5. 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. (never mind, this should be OK)

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

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

  7. 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. 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.

  8. 
      
DT
  1. 
      
  2. 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. 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.

  3. 
      
DT
ZU
  1. Ship It!

  2. 
      
IT
  1. Ship It!

  2. 
      
DT
DT
DT
DT
Review request changed

Status: Closed (submitted)

Loading...