Compiler warnings related to com.infradna.tool.bridge_method_injector #738

Closed
cowwoc opened this Issue May 5, 2014 · 10 comments

Comments

Projects
None yet
3 participants
@cowwoc
Contributor

cowwoc commented May 5, 2014

When upgrading from querydsl-sql 3.3.2 to 3.3.3 I suddenly get the following compiler warnings:

Cannot find annotation method 'value()' in type 'com.infradna.tool.bridge_method_injector.WithBridgeMethods': class file for com.infradna.tool.bridge_method_injector.WithBridgeMethods not found
Cannot find annotation method 'castRequired()' in type 'com.infradna.tool.bridge_method_injector.WithBridgeMethods'

and so on. My project builds under 1.8.0-b132 but I don't think this is JDK8-related because the previous version didn't generate these warnings.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest May 5, 2014

Member

I am not sure if the WithBridgeMethods annotation can be removed in the related plugin http://bridge-method-injector.infradna.com/apidocs/com/infradna/tool/bridge_method_injector/WithBridgeMethods.html

@TuomasKiviaho Do you have any idea/opinion about this?

Member

timowest commented May 5, 2014

I am not sure if the WithBridgeMethods annotation can be removed in the related plugin http://bridge-method-injector.infradna.com/apidocs/com/infradna/tool/bridge_method_injector/WithBridgeMethods.html

@TuomasKiviaho Do you have any idea/opinion about this?

@TuomasKiviaho

This comment has been minimized.

Show comment
Hide comment
@TuomasKiviaho

TuomasKiviaho May 6, 2014

Contributor

@timowest I think that the culprit is true tag of bridge-method-annotation dependency as seen below. It worked in prior version when querydsl-sql/jpa/jdo were not yet pointing to it indirectly. annotation-indexer is only needed when querydsl itself is compiled and in my opinion can still remain as optional.

The only problem is OSGi Import-Package clauses where com.infradna.tool can't be added because bridge-method-annotation library is not OSGi compliant. I'll have to ponder over this in the next comment.

 <!-- backwards compatibility -->
    <dependency>
      <groupId>com.infradna.tool</groupId>
      <artifactId>bridge-method-annotation</artifactId>
      <version>${bridge-method.version}</version>
      <optional>true</optional>
      <exclusions>
        <exclusion>
          <groupId>org.jenkins-ci</groupId>
          <artifactId>annotation-indexer</artifactId>
        </exclusion>
      </exclusions>
    </dependency>
    <dependency>
      <groupId>org.jvnet.hudson</groupId>
      <artifactId>annotation-indexer</artifactId>
      <version>1.2</version>
      <optional>true</optional>
    </dependency>
Contributor

TuomasKiviaho commented May 6, 2014

@timowest I think that the culprit is true tag of bridge-method-annotation dependency as seen below. It worked in prior version when querydsl-sql/jpa/jdo were not yet pointing to it indirectly. annotation-indexer is only needed when querydsl itself is compiled and in my opinion can still remain as optional.

The only problem is OSGi Import-Package clauses where com.infradna.tool can't be added because bridge-method-annotation library is not OSGi compliant. I'll have to ponder over this in the next comment.

 <!-- backwards compatibility -->
    <dependency>
      <groupId>com.infradna.tool</groupId>
      <artifactId>bridge-method-annotation</artifactId>
      <version>${bridge-method.version}</version>
      <optional>true</optional>
      <exclusions>
        <exclusion>
          <groupId>org.jenkins-ci</groupId>
          <artifactId>annotation-indexer</artifactId>
        </exclusion>
      </exclusions>
    </dependency>
    <dependency>
      <groupId>org.jvnet.hudson</groupId>
      <artifactId>annotation-indexer</artifactId>
      <version>1.2</version>
      <optional>true</optional>
    </dependency>
@TuomasKiviaho

This comment has been minimized.

Show comment
Hide comment
@TuomasKiviaho

TuomasKiviaho May 6, 2014

Contributor

@timowest bridge-method-annotation has CLASS as RetentionPolicy for the annotation so this should not be happening in the first place. Same form of design is used on other projects such as org.apache.felix.scr.annotations where helper annotations are not needed on classpath after compilation process is done.

It might be also that I've misunderstood the difference of SOURCE and CLASS policies, but in any case it would be easiest to...

  1. Mutilate the annotations away by introducing a maven plugin switch and accompanying ASM code configuration to bridge-method-injector project.
  2. Ask whether or annotation not policy could be changed to SOURCE (if indeed that would make a difference).
  3. Check whether there is something wrong with the way policies are handled by jdk 1.8.0-b132.
Contributor

TuomasKiviaho commented May 6, 2014

@timowest bridge-method-annotation has CLASS as RetentionPolicy for the annotation so this should not be happening in the first place. Same form of design is used on other projects such as org.apache.felix.scr.annotations where helper annotations are not needed on classpath after compilation process is done.

It might be also that I've misunderstood the difference of SOURCE and CLASS policies, but in any case it would be easiest to...

  1. Mutilate the annotations away by introducing a maven plugin switch and accompanying ASM code configuration to bridge-method-injector project.
  2. Ask whether or annotation not policy could be changed to SOURCE (if indeed that would make a difference).
  3. Check whether there is something wrong with the way policies are handled by jdk 1.8.0-b132.

@TuomasKiviaho TuomasKiviaho referenced this issue in infradna/bridge-method-injector May 6, 2014

Closed

Integration as optional dependency causes compiler warnings #5

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest May 6, 2014

Member

@TuomasKiviaho Source retention is not an option, since the annotation needs to make it into the binaries, since there the transformation happens. There is the choice between CLASS and RUNTIME however http://docs.oracle.com/javase/7/docs/api/java/lang/annotation/RetentionPolicy.html

Member

timowest commented May 6, 2014

@TuomasKiviaho Source retention is not an option, since the annotation needs to make it into the binaries, since there the transformation happens. There is the choice between CLASS and RUNTIME however http://docs.oracle.com/javase/7/docs/api/java/lang/annotation/RetentionPolicy.html

@cowwoc

This comment has been minimized.

Show comment
Hide comment
@cowwoc

cowwoc May 9, 2014

Contributor

Looks like we have our answer. <optional>true</optional> is not supported. What now? :)

Contributor

cowwoc commented May 9, 2014

Looks like we have our answer. <optional>true</optional> is not supported. What now? :)

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest May 9, 2014

Member

Is there is no easy way to remove the related annotations from the bytecode we can remove the optional flag, if there is consensus on that.

Member

timowest commented May 9, 2014

Is there is no easy way to remove the related annotations from the bytecode we can remove the optional flag, if there is consensus on that.

@cowwoc

This comment has been minimized.

Show comment
Hide comment
@cowwoc

cowwoc May 9, 2014

Contributor

Can you give me more background on why we need this annotation on the first place? Which part of the library uses it, and why?

Contributor

cowwoc commented May 9, 2014

Can you give me more background on why we need this annotation on the first place? Which part of the library uses it, and why?

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest May 9, 2014

Member

It's used in querydsl-core and querydsl-sql to ensure binary level backwards compatibility for certain changes. But the purpose is only to add as compilation post processing markers and via the optional=true we tried to avoid transitive dependency to the annotation, which didn't really work out.

Member

timowest commented May 9, 2014

It's used in querydsl-core and querydsl-sql to ensure binary level backwards compatibility for certain changes. But the purpose is only to add as compilation post processing markers and via the optional=true we tried to avoid transitive dependency to the annotation, which didn't really work out.

@cowwoc

This comment has been minimized.

Show comment
Hide comment
@cowwoc

cowwoc May 9, 2014

Contributor

Yes, I see. http://bridge-method-injector.infradna.com/ explains the problem quite nicely.

It sounds to me like the bridge-method-injector library could strip its own annotations at the same time as injecting the bridge methods, no?

I'd file a RFE for that, and disable <optional>true</optional> until it's implemented.

Contributor

cowwoc commented May 9, 2014

Yes, I see. http://bridge-method-injector.infradna.com/ explains the problem quite nicely.

It sounds to me like the bridge-method-injector library could strip its own annotations at the same time as injecting the bridge methods, no?

I'd file a RFE for that, and disable <optional>true</optional> until it's implemented.

@timowest timowest added the fixed label May 10, 2014

@timowest timowest added this to the 3.3.4 milestone May 10, 2014

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest May 20, 2014

Member

Released in 3.3.4

Member

timowest commented May 20, 2014

Released in 3.3.4

@timowest timowest closed this May 20, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment