FasterXML / woodstox

The gold standard Stax XML API implementation. Now at Github.
Apache License 2.0
220 stars 81 forks source link

`woodstox-core` 6.3/6.4 javac warning due to unexpected new dependency on `aQute.bnd.annotation.Resolution` #163

Closed ben-spiller closed 1 year ago

ben-spiller commented 1 year ago

The GH-155 fix released in v6.3.1 introduces a major regression in woodstox-core since any build with javac warnings-as-errors enabled (as per best practice) will now fail (unless the aQute library is on classpath, which it usually isn't).

Example (using Java 11):

> javac -classpath 6.4.0\woodstox-core.jar -Werror WoodstoxBug.java
warning: unknown enum constant Resolution.OPTIONAL
  reason: class file for aQute.bnd.annotation.Resolution not found
error: warnings found and -Werror specified
1 error
1 warning

... where WoodstoxBug.java can be any code that refers to any of the main woodstox classes e.g.

public class WoodstoxBug {
    public static void main(String[] args){
        com.ctc.wstx.stax.WstxInputFactory f = new com.ctc.wstx.stax.WstxInputFactory();
    }
}

Requiring everyone to either disable warning checks or add an extra dependency on aQute would be a major compatibility break. The woodstox documentation states there is no mandatory dependency other than stax2, so could we get a fix that restores this?

cowtowncoder commented 1 year ago

That's odd, dependency is optional/provided:

        <dependency>
          <groupId>biz.aQute.bnd</groupId>
          <artifactId>biz.aQute.bnd.annotation</artifactId>
          <version>${version.bnd.annotation}</version>
          <optional>true</optional>
          <scope>provided</scope>
        </dependency>

which I thought would prevent this?

ben-spiller commented 1 year ago

That would only work for classes that are rarely used / not a core part of the API (and even then is a bit dodgy / needs very careful testing and doc of which packages/classes need aQute as a result).

But while configuration such as optional dependencies will allow you to get it building, the bottom line is that if you have a class that depends on aQute then it will have a reference to aQute in the compiled .class files so will get compiler warnings (or errors) when people try to use it. There's no way round that - it'd be a major breaking change for woodstox to start referencing aQute like this.

cowtowncoder commented 1 year ago

@ben-spiller Is this due to referencing value other than Annotation types? Handling of missing annotation types is special and should be safe. But I fear we have the same issue here as with jackson-core:

https://github.com/FasterXML/jackson-core/issues/824

and if so, yes, I suppose we better revert this addition here as well.

Thank you for bringing this to my attention. I wish I had realized unfortunate side effects: the intent is definitely to keep Woodstox as close to zero-dependency as possible (beyond Stax API, optional MSV for validation). My thinking was that OSGi annotations would be that way too, but alas not.

ben-spiller commented 1 year ago

Is this due to referencing value other than Annotation types? Handling of missing annotation types is special and should be safe.

Correct, see my example above - merely referring to WstxInputFactory itself introduces the warning.

and if so, yes, I suppose we better revert this addition here as well. Yes, like the jackson one, I think it needs reverting unfortunately

cowtowncoder commented 1 year ago

Fixed via #164, will be in 6.5.0 release.

cowtowncoder commented 1 year ago

6.5.0 released on January 14, 2023.