-
Notifications
You must be signed in to change notification settings - Fork 74
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
Update OWLAPI to version 4.5.29 #1200
Conversation
Not specific to this PR, but it made me notice that we have tons of references to a nonexistent FOAF property ("depicted_by"). It should be |
pom.xml
Outdated
@@ -204,12 +204,12 @@ | |||
<dependency> | |||
<groupId>ch.qos.logback</groupId> | |||
<artifactId>logback-classic</artifactId> | |||
<version>1.2.13</version> | |||
<version>1.3.0</version> |
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.
This is fine.
But just FYI, it would also be possible to make ROBOT compile against the new OWLAPI without having to drastically bump the versions of the logging libraries (Logback and Log4-over-SLF4J): simply add an explicit dependency to org.slf4j:slf4j-api
version 1.7.x.
The reason the build fails if we do not update the logging libraries is because the OWLAPI declares a dependency to org.slf4j:slf4j-api
2.x. ROBOT “inherits” that dependency (since it depends on the OWLAPI), so when we build the robot.jar
archive, that archive ends up containing a copy of the slf4j-api
2.x (inherited from the OWLAPI) and a copy of Logback-classic (explicitly declared here as a direct dependency) — and those two versions are not compatible.
But SLF4J-API is strictly backwards compatible. Just because the OWLAPI declares it depends on slf4j-api
>= 2.x does not mean it cannot work with slf4j-api
1.x.
So we could explicitly declare slf4j-api
1.7.x as a direct dependency of ROBOT here (alongside Logback-classic and Log4J-over-SLF4J). We would then end up with a robot.jar
archive containing only versions of logging libraries that are compatible with each other (SLF4J-API < 2 and Logback-classic < 1.3).
Not saying that we should do that, of course. It is perfectly fine to bump to Logback-classic 1.3 and Log4J-over-SLF4J 2.0. Just mentioning the other possibility.
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.
Oh thanks @gouttegd I didnt know that!
@jamesaoverton what is your preference? Bumping versions up, or adding this additional (transitive) dependency as a constraint?
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 @gouttegd for the explanation.
I despise Java logging, which may be clouding my judgement. But I think the simplest thing is just to bump these two dependencies to their latest versions:
- pkg:maven/ch.qos.logback/[email protected]
- pkg:maven/org.slf4j/[email protected]
This built and passed the test suite for me locally, so I don't see the benefit of adding yet another dependency (slf4j-api
) that I don't understand. If I misunderstood something, please let me know.
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 despise Java logging
I just spent the last 24 hours debugging issues caused by Java logging libraries, so… I share your feeling! :)
I don't see the benefit of adding yet another dependency (slf4j-api) that I don't understand. If I misunderstood something, please let me know.
Just that ROBOT is already dependent on slf4j-api
– it’s just that currently that dependency is an indirect one.
But I fully agree that adding the explicit dependency on slf4j-api
just to avoid bumping logback-classic
and log4-over-slf4j
would have no particular benefit in ROBOT’s case. It would only be useful if for some reason we wanted to keep using older versions of those libraries.
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.
The slf4j upgrade has been removed. Only the logback upgrade remains now.
@gouttegd explained the fact that, for some reason, the built passed without this change because of some issue with the shading plugin. We still need this upgrade
@matentzn Why not use the latest versions of these dependencies?
|
I have no opinion on these matters, other than not constraining the dependencies more than absolutely necessary- I leave it to you and am very happy to support using the most up to date versions! |
Ok. Since we're updating, I'd like to update to the latest stable releases of these dependencies. |
Update OWLAPI to version 4.5.28
docs/
have been added/updatedmvn verify
says all tests passmvn site
says all JavaDocs correctCHANGELOG.md
has been updated