-
Notifications
You must be signed in to change notification settings - Fork 886
Update JSVG from 1.6.1 to 1.7.1 #8412
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
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.
Personally, I have doubts about the merits of increasing the release version to 2 on the basis of this relatively small API change. There are likely to be more inconsistencies considering #8382 (comment)
The changes should anyway go in the manifest, and I think for clarity match the use of implementation version for library version as in eg. libs.flatlaf manifest
If you are going to go ahead with the release version increment, then the module name needs to be suffixed with /2
and matched in modules depending on this one.
Manifest should end up including something like -
OpenIDE-Module: org.netbeans.libs.jsvg/2
OpenIDE-Module-Implementation-Version: 1.7.1
OpenIDE-Module-Specification-Version: 2.0.0
If you decide against the release version increment, please at least add the implementation version matching upstream.
Also, for additional info - https://bits.netbeans.org/25/javadoc/org-openide-modules/org/openide/modules/doc-files/api.html
|
||
is.autoload=true | ||
javac.source=1.8 | ||
|
||
nbm.homepage=https://github.com/weisJ/jsvg | ||
sigtest.gen.fail.on.error=false | ||
spec.version.base=1.23.0 | ||
spec.version.base=2.0.0 |
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.
Let's remove this, but add spec.version.base.fatal.warning=false
. Everything else could go in manifest, like FlatLaf.
I personally don't care about lib wrapper versions since libs are outside our control. I personally would match the version 1:1 without obfuscating anything, users have to look at the release notes anyway, no matter if it still compiles or not. (analog to osgi loaded libs, where we don't even set any version) @matthiasblaesing @neilcsmith-net I can change this PR to whatever you prefer. Going to update it to the suggestion neil made #8412 (comment). Just let me know what to do next time I update a lib. |
Ideally, yes, and we'd do that with spec versions, but they get auto-incremented each release. That's the reason for separating spec and implementation, and using the latter to match upstream. |
- bumped spec version due to API change
OpenIDE-Module: org.netbeans.libs.jsvg | ||
OpenIDE-Module-Implementation-Version: 1 | ||
OpenIDE-Module: org.netbeans.libs.jsvg/2 | ||
OpenIDE-Module-Implementation-Version: 1.7.1 | ||
OpenIDE-Module-Specification-Version: 2.0 |
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.
made it 2.0 instead of 2.0.0 since all other spec manifests seem to use only two components.
updated projext.xml of platform/openide.util.ui.svg
to make it compile
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! Looks OK at a glance. Will leave @matthiasblaesing to comment as he requested the release version bump. I remain happy without it too!
I might not be available later today, feel free to push into this PR in case of changes |
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.
Looks sane to me.
quick test run rendered all icons I checked correctly