Update JSVG dependency to 2.0.0 #2597
Conversation
|
FYI, this build needed to finish to make the TP changes available: https://ci.eclipse.org/platform/job/eclipse.platform.releng.aggregator/job/master/3818/changes |
Yeah, thanks! I saw that I was just too fast with the follow-up 🙂 |
|
Do we need a minor version bump of a bundle/fragment when we update the major version of one of it's dependencies? I.e., do we need to bump minor version of the SVG fragment because we make a major bump of the JSVG dependency? I do not find specific information about the case in our guidelines: https://github.com/eclipse-platform/eclipse.platform/blob/master/docs/VersionNumbering.md |
|
Most recently major updates of 3rd party deps have been Lucene and Jetty - in both cases major version of the bundle requiring them wasn't bumped. So only a minor bump is what I would recommend. |
|
A minor version bump seems appropriate. |
Thank you! That aligns with what I would do if in doubt. Just because I wasn't completely precise in my question: I did actually not think about major vs. minor version bump of the fragment but rather about minor vs. service version bump. I have adapted the PR to bump the minor version of the fragment. |
|
Last time fragments bundle versions were changed without SWT change and that caused eclipse-platform/eclipse.platform.releng.aggregator#3347 Could it be, all SWT fragments & bundle versions need to be touched now? |
In my understanding, that should only be the case for the (actually non-optional) native fragments. This change one affects the SVG fragment which is fully optional, which is also why it's not bumped together with all the other fragments and the host. So I expect no issues with only bumping the SVG fragment version. |
I have no objection to applying a minor version bump, but I also think it's not strictly necessary as JSVG is not part of that bundles API so that change should not be a noticeable change for consumers. And as this is a fragment there are hardly any direct consumers. Btw. can anyone recall why the |
Yes I think not strictly necessary but no harm done either. |
I do not remember and I do not see any actual reason for it. Maybe it was an oversight or it was necessary for the slightly more complex setup of the fragment we had in the beginning (with additional p2.inf file etc.). Anyway, it has been introduced at the very beginning: I scanned through the PR comments but I did not find anything on that topic, except that you had a comment next to the export within the Manifest 😛 #1638 (comment) If no one has complains, I plan to merge this before the next I-Build to see if everything works as expected (e.g., regarding the mentioned version bump issue #2597 (comment)). |
Nobody is perfect 😅 But you are probably right and it's a left-over. Or maybe it was done to give down-stream consumers the ability to reference the package (what they shouldn't do)?
Yes, this is totally fine for me. Thank you for migrating this. |
I agree. We could do that separately from this change. What makes it a bit complicated is that such a change is of course considered API-breaking (which can be "resolved" with an API filter for the fragment) but unfortunately also requires downstream adaptations (those error remain when adding an API filter for the fragment): |
This is independent of How this is handled by API tools is another story, but I would assume that |
Yeah, when I thought about potential reasons for the API tools complaining I also found that this is something you can probably not analyze properly. And in my opinion it does not really matter either as you should probably avoid doing something like this anyway and the export we have here is simply wrong/unintended anyway. So we can simply fix it by removing the export and adding according API filters to the downstream consumers that reexport SWT. We had to do the same in other situations as well: when we had to add an API filter to SWT, we always had to do the same for the downstream consumers that reexport SWT. |
Just another reason why reexporting is really a bad thing, I'm currently try to came up with some things we can get rid of reexports on the long run but in the meanwhile we need to scope with that sadly :-\ |
No description provided.