Do not compile on resource dependencies change

Review Request #3106 - Created Nov. 10, 2015 and submitted

Information
Timur Abishev
pants
do_not_compile_on_resource_dependencies_change
2535
Reviewers
pants-reviews
patricklaw, stuhood

Target's jvm compile should not be triggered by changes in dependencies with Resources type.

PR: https://github.com/pantsbuild/pants/pull/2535
Reviewed at https://rbcommons.com/s/twitter/r/3106/

https://travis-ci.org/ttim/pants/builds/90560450

Issues

  • 0
  • 2
  • 0
  • 2
Description From Last Updated
Timur Abishev
Stu Hood
Patrick Lawson
Timur Abishev
Stu Hood
Timur Abishev
Review request changed

Status: Closed (submitted)

Change Summary:

Merged as 9a0424863f12161825eb67cc26d5520b2d5c0379

Eric Ayers

   

As I pointed out in https://rbcommons.com/s/twitter/r/3185/, annotation processors can change the ouptut of the compiler on resource changes. We don't need to entirely kill this optimization, but maybe make it so that repos can opt-in if they want this behavior.

  1. I thought you have solution for this problem already because of https://pantsbuild.slack.com/archives/general/p1447039576000239 . Is it another face of the same problem? Should we just revert this rb or make something else?

  2. We have this solution in place, but this is for a file that lives outside of a resource path. And let me be clear, it is quite awful.

    It is incorrect in general to make this optimization. Althought it may work in most cases, if you make this optimization Pants will silently fail in this case leading to a very difficult to detect problem. That is why I think you should make it optional, and also make it so that you have to explicitly turn it on.

  3. From my point of view the question is in expected behaviour. Looks like both zinc and idea doesn't consider changes in resources as reason to recompile the code, am I right? Maybe we need a way to explicitly declare this kind of resources dependencies? Just for context - can you tell a little bit about how you are using annotation processors and what kind of info they are reading from files? Thanks.

  4. I think declaring a special type of resource might be a good idea, because it is not the normal usecase. Essentially, we have an annotation processor that reads from a resource file and uses it along with an annotation processor to create an output file. Imagine these possible use cases:

    • An annotation processor that creates output based on a mustache template
    • An annotation processor that creates source code based on options configured in a resource file

    I think it is fair to say that the way this annotation processor was written was a bad idea, but it was written before there was incremental compilation in the mix.

  5. Alternatively - could we just always do this for resource dependencies of annotation processors, and never for resource dependencies of libraries? If not then I agree that a separate "invalidating resource target" type makes sense.

    Also, we should make sure that the right behavior falls out naturally from the new engine. Basically, resources are a runtime dep, not a compile-time dep, but the annotation processor's runtime is the rest of the code's compile time.

  6. It seems to cause alot of confusion, but the annotation_processor target type only calls out annotation processors living in the current repo as loose source code; it's of no help identifying things like AutoValue - an annotation processor library brough in via a jar_library. So, today, we have no way to know when annotation processors are active in a compilation round - they may be activated via direct or indirect classpath dependencies.

  7. Yes, but those in-repo processors are the ones we're concerned about when it comes to in-repo resources affecting compiler outcomes.

    Also, something must be putting the annotation processors on the runtime classpath, no? Or are they consumed from the compile classpath?

  8. On the 1st point - likely true, although you can imagine a generic processor shipped by - say spring - that (optionally) can be configured via a well known META-INF/ located resource on the classpath.  Less exotically, you can imagine the internal processor is published internally for use in several repos.
    
    On the 2nd part - the processor will only generally be useful on the compile classpath.  Most - more or less by definition - don't play a role in the runtime.
  9. Ah right, I'm thinking about the new engine though. Would that be able to treat the runtime classpath of a tool as distinct from the compile classpath (i.e., the deps of the code the tool is running on)?

  10. A more useful response to part 2: The common pattern I've seen is for the annotation jar to contain or depend on the processor.  This puts the processor on the compile classpath transparently.
  11. "Would that be able to treat the runtime classpath of a tool as distinct from the compile classpath (i.e., the deps of the code the tool is running on)?" <- The problem is that the tool's runtime classpath - namely the element that contains the resource file that controls it - is, today, part of the compiler compile classpath.
    
    I think we'd need to use metadata we have today (annotation_processor target type for local) combined with metadata we do not grab today (scan of compile classpath for all META-INF/services/javax.annotation.processing.Processor) + javac's -processorpath to isolate this all correctly.  The most involved part would be using the data found in META-INF/services/javax.annotation.processing.Processor files to - post resolve - re-calculate the main compile classpath, factoring out the processors' classpaths for -processorpath.
  12. Even if what I said is true for other reasons - its still no good without explicit manual metadata: The runtime classpath of an annotation processor that uses user resources will be the processor's resolved classpath + a scan of the user's classpath (the compile classpath) for the resource files that are used to control it. So we'd need a way to mark certain resource targets as compile dependencies - which all resource targets were treated as prior to this change, and treat all other resource targets as (user) runtime classpath contributors only, eg maybe:

    resources(
      name='control_the_apt',
      sources=['control_the_apt.yml'],
      compile_classpath=True
    )
    

    In the current draft of the new engine, this would be spelled more like the following ("legacy" BUILD format):

    resources(
      name='control_the_apt',
      sources=sources(files=['control_the_apt.yml']),
      configurations=[
        jvm_compiler_config(
          compile_classpath=True   
        )
      ]
    )
    
Loading...