SWR API Incompatible with JDK7 Compiler

[alrubinger]

Michal Matloka today noticed this issue with building SWR with JDK7:

  https://issues.jboss.org/browse/SHRINKRES-78

I believe we have 3 options.

1) Change the API so this isn't a problem

2) Put some post-compilation step in place to fix up the bytecode (it'll work fine in the runtime, but the compiler is being overly aggressive IMO)

3) Enforce that we build with JDK6

Drawbacks to these:

1) But the API we have is so nice!

2) Complexity / feasibility.  If this can be worked through, it's my favorite option

3) SWR in its current state will never be able to move to JDK7+ libraries or features.

Again, we're in Alpha, so if we need to make API changes we *can*.  Opinions sought.  Go.

[alrubinger]

I *also* wonder if it's just the Oracle JDK7 compiler implementation which is introducing this restriction.  I'm not opposed to mandating we build the project with another vendor's compiler.

[alrubinger]

Fails with:

IBM JDK: Java version: 1.7.0, vendor: IBM Corporation

Oracle JDK: Java version: 1.7.0_03, vendor: Oracle Corporation

OpenJDK: Java version: 1.7.0_147-icedtea, vendor: Oracle Corporation

[alrubinger]

This is legal compilation under JDK6:

  // access flags 0x401
  // signature (Ljava/lang/Class<Ljava/io/File;>;)[Ljava/io/File;
  // declaration: java.io.File[] as(java.lang.Class<java.io.File>)
  public abstract as(Ljava/lang/Class;)[Ljava/io/File; throws java/lang/IllegalArgumentException

  // access flags 0x401
  // signature (Ljava/lang/Class<Ljava/io/InputStream;>;)[Ljava/io/InputStream;
  // declaration: java.io.InputStream[] as(java.lang.Class<java.io.InputStream>)
  public abstract as(Ljava/lang/Class;)[Ljava/io/InputStream; throws java/lang/IllegalArgumentException

[alrubinger]

API Changes for debate:

  https://github.com/ALRubinger/resolver/commit/1da03cb29b39278e2404a76c625f0f0310d7507a

This renames:

as(File.class) > asFile();

asSingle(File.class) > asSingleFile();

...etc

[kpiwko]

I was thinking of making as() an SPI point. So methods would be T[] as(Class<T> clazz) and T asSingle(Class<T> clazz). T would be any type,

there would be a FormatterFactory, which will delegate to an impl which is able to handle T or fail with an exception.

That way we:

+ 1/ Keep the API simpler and nicer

+ 2/ Allow users to wrap results as whatever type they want

+ 3/ It would force us to wrap ArtifactResult object into proper SPI

- 1/ We loose type safe API in sense that user would be able to retype to JavaArchive.class even if that formatter is not on classpath, failing in runtime instead of compile time.

I have to do a prototype impl to see if that works first though.

[alrubinger]

I believe 2) and 3) are still covered by my patch, which continues to provide:

<RETURNTYPE> RETURNTYPE[] as(Class<RETURNTYPE> type, FormatProcessor<RETURNTYPE> processor)
        throws IllegalArgumentException;
<RETURNTYPE> RETURNTYPE asSingle(Class<RETURNTYPE> type, FormatProcessor<RETURNTYPE> processor)
        throws IllegalArgumentException, NonUniqueResultException, NoResolvedResultException;

In this way we still accept an extensible FormatProcessor without losing type safety (and IDE autocomplete makes it clear to the user what operations are permitted:

asFile()

asSingleFile()

asInputStream()

asSingleInputStream()

asResolvedArtifactMetadata()

asSingleResolvedArtifactMetadata()

as(MyClass.class,new MyClassFormatter())

asSingle(MyClass.class,new MyClassFormatter())

...or am I missing some other benefit to your FormatterFactory approach?  If you put up a commit/patch for review we can debate the path to move us forward.

[kpiwko]

Seen that, but currently occupied with other stuff. I'll definitely provide an implementation to discuss "soonish". The only benefit of my approach is that it avoids method explosion.

[alrubinger]

Cool.  My preference will probably go for whatever's most intuitive, looking forward to seeing it.

[kpiwko]

I finally managed to do the prototype impl. It is stored here:

https://github.com/kpiwko/resolver/tree/SHRINKRES-78

Changes done:

1/ An SPI service to replace existing one allowing to register multiple services per interface

2/ Changed FormatProcessor API

3/ Changed MavenFormatStageImpl to dynamically load FormatProcessors

4/ Changed FormatStage to allow any RETURTYPE type, this means we do not longer fail compile-time

5/ Aligned ResolvedArtifactInfo

Side effect fixes:

1/ [not yet reported] Former SPI does not close input stream

2/ [SHRINKRES-79] MavenDependencyImpl exclusion ordering

Changes to be done if this implementation is preferred:

1/ Create a proper SPI module

2/ Propagate registered services, e.g. search classpath only once

3/ Remove all other FormatStages and reduce number of entry points, simply generics in the project

Karel

[alrubinger]

Karel and I reviewed this in IRC, and iniitally I really liked this proposal.  On further inspection, though, I've found a problem:

By registering the supported FormatProcessors at runtime, we lose the ability to give the users a hint as to what's supported at compile-time.  They know they're supposed to put in Class<SOMETHING>, but how are they to know that File.class, InputStream.class, etc are supported?

For this reason and this reason alone, I'd prefer to go with the method-overload approach in my patch.  It's not as pretty, but IMO it does make for a more intuitive experience.

Thoughts?

[alrubinger]

Another hitch: it's impossible to resolve a ResolvedArtifactInfo from the FormatProcessor SPI - the contract doesn't provide enough info.

[kpiwko]

As for FormatProcessor, its API is poorly designed...we need a specific object to be passed between Strategy and Formatter. Then we can have a contract which will make ResolvedArtifactInfo processor possible.

I'm thinking of a compromise. Having asFile() and as(Class<?> returnType). People need mostly files, right? More advanced types we can document.

[kpiwko]

I've updated SHRINKRES-78 branch at https://github.com/kpiwko/resolver/tree/SHRINKRES-78 with following changes:

1/ Refactored ResolvedArtifactInfo into ResolvedArtifact, which is now primary object passed from StrategyStage to FormatStage

2/ Modified FormatProcessors to handle ResolvedArtifact (or its subtype) instead of File

3/ Modified FormatStage to have following methods (e.g. used almost your approach while removing direct injection of FormatProcessors and adding asResolvedArtifact()):

• as(Class<RETURNTYPE), asFile(), asInputStream(), asResolvedArtifact()
• asSingle variants of above

4/ ServiceRegistry which acts as an app-scoped container for registered services, solving ClassLoader propagation

5/ Updated MavenResolvedArtifact to have the same API as FormatStage

The only call where as(X) is needed is with as(Class<Archive<?>>). With SPI support, we still have possibility to trim number of interfaces and remove ShrinkWrapMaven altogether, as discussed on irc.

[alrubinger]

Great hybrid approach, I think.  Well done.

Currently in upstream/master:

• I pulled from Karel's branch and fixed a failing test
• I separated out the FormatProcessor stuff from the API into a proper SPI - users shouldn't be seeing it.  Some reflection was required for this bit.  Documented as https://issues.jboss.org/browse/SHRINKRES-82.
• Removed ShrinkWrapMaven and the rest of the api-maven-archive module. Documented as https://issues.jboss.org/browse/SHRINKRES-83.

So there's no longer IDE autocomplete for as(JavaArchive.class), but I think that's livable, especially since we got to remove that pesky second ShrinkWrapMaven entry point (which served only to close the generic context in a way that extended the API).

All of this in 2.0.0-alpha-5.  Not a bad release, I must say.  Thanks to all.

S,

ALR