Update commons libs, Freemarker and JNA#8382
Conversation
75cc5c6 to
a42d896
Compare
|
resolved a conflict and rebased |
a42d896 to
f2af285
Compare
matthiasblaesing
left a comment
There was a problem hiding this comment.
Only eyeballed, but change makes mostly sense to me and seems to be done very thoroughly.
This does not hold true for the JSVG update. The commit message is wrong (update goes to 1.7.1, not 1.6.2). While it did not bump major version, it clearly broke API. Compare:
1.6.1:
1.7.1:
Existing callers are broken by this.
|
thanks for spotting the copy/paste issue in the commit message. Will check the other messages too. I noticed the API change and updated the (single) usage in the same commit as mentioned:
|
We wrap the module, so this then should be come major version 2 with spec 2.0.0. At least that is my interpretation. |
|
I don't think we took responsibility for fixing the versioning scheme of third party libs before. But i could be wrong and I also don't really care that much since it is just about a version String. But this would be extra work to add those checks for each wrapper update from now on. |
|
@mbien the problem is, that we decided to expose the module as API. I know, that we had such situations. The problem is, that I don't remember which library did this. Solution could be to remove the exported api and make this a module, that can be used as an implementation dependency. Then noone can rely on it. @weisJ would it be possible, that JSVG in the future bumps major version when incompatible changes are introduced into the codebase? |
I'm already relying on it! https://www.praxislive.org/blog/on-watch/ So, -1 to hiding it now. In fact, in general -1 to hiding third-party libraries - had enough problems with that with JNA years ago. Sometimes third-party libraries have different API rules than we do ourselves. I'm not sure if this is also related to weisJ/jsvg#119 somehow? Anyway, I say we live with it. |
|
i will bump that spec version as suggested, want to fix something else first till i get to this pr |
|
I think it would make sense to split the update of JSVG off from this and do that in isolation. |
Sorry, that was me! For my future reference, is there a way to do a "private" library wrapper module, used by only specific other modules? |
f2af285 to
2383115
Compare
|
removed the jsvg commit and rebased, will link the jsvg PR later from PR text |
Yes this is very much related to the mentioned issue. In the current way the packages are structured it is almost impossible for me to keep full A(P/B)I compatibility with new releases. The next release will be focused on fixing this situation and provide a set of stable public facing APIs. |
@eirikbakke good, don't apologize, that was the right approach! You can include wrapper libraries inside the utilising module, or expose the library packages via friend API to another module as you might with module code. Have a really good reason if you take either of those approaches. Hiding third-party APIs is generally not great. We should be OK with the fact that they do not necessarily follow the same versioning policies that we do in exposing them, and in documenting how people might use them. |
Update JSVG from 1.6.1 to 1.7.1moved to Update JSVG from 1.6.1 to 1.7.1 #8412JSVG had a small code incompatibility which was addressed in the same commit
To prevent #7816 happening again, I checked the dependency trees for changes:
Details
rest didn't have any dependencies