-
Notifications
You must be signed in to change notification settings - Fork 397
Change JAXB implementation for better java version compatibility #996
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Downgrade jgrapht to maintain java 8 support - Switch JAXB to glassfish implementation for better java 15+ support
josemduarte
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! good catch about jgrapht dependency. I think we'll need a new bugfix release soon to keep the java-8 compatibility.
See comments below
| <dependency> | ||
| <groupId>org.glassfish.jaxb</groupId> | ||
| <artifactId>jaxb-runtime</artifactId> | ||
| </dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like biojava-aa-prop did not have an xml dependency before. I think this is not needed here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got test failures without it ('No JAXP implementation found'). I think it was serendipitously included before since we were using the com.sun namespace, but that the glassfish implementation needs to be specifically included. I have included it on all modules that import javax.xml.bind.
| <dependency> | ||
| <groupId>org.glassfish.jaxb</groupId> | ||
| <artifactId>jaxb-runtime</artifactId> | ||
| </dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like biojava-core did not have an xml dependency before. I think this is not needed here
| <dependency> | ||
| <groupId>org.glassfish.jaxb</groupId> | ||
| <artifactId>jaxb-runtime</artifactId> | ||
| </dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like biojava-modfinder did not have an xml dependency before. I think this is not needed here
This also works with Java 8
josemduarte
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks!
There was an issue with running CeSymm on java 15+ (rcsb/symmetry#104). I suspect that this issue effected all projects that include biojava with the maven-shade-plugin. I was able to trace the problem back to the old com.sun.xml.bind implementations. Switching these to the glassfish JAXB implementation fixed the problem.