Add SWT SVG fragment to E4 RCP feature #2862
Conversation
HeikoKlare
force-pushed
the
svg-to-feature
branch
from
7820769 to
55817ec
Compare
|
I lost touch with the long thread about this fragment... If we only need to ensure the bundle is in the update site, we could just add it here: |
|
From my understanding, the fragment is actually available in the update site but not included in the products we deploy (runtime binaries, SDK and SDK with test bundles). Basis for this PR are the comments starting from this (for those who understandably do not want to read the whole thread 🙂 ): eclipse-platform/eclipse.platform.swt#1944 (comment) |
HeikoKlare
force-pushed
the
svg-to-feature
branch
from
55817ec to
40c8387
Compare
Should it then not added to the test feature if it is only required by tests (at the moment). |
And then add it to the productive feature next week? Or leave it out there because it's implicitly taken through required capabilities there, so releng for productive and test deployment becomes different and people will need to know about that when eventually something does not work anymore? I have no strong opinion but I would really like to close the discussion and come to a conclusion here. For Eclipse products, the SVG fragment will be required once we use the first SVGs anyway, so I would not overcomplicate this. I would be in favor of just adding the SVG fragment to the RCP feature. |
|
Go for the RCP feature for now. |
There was a problem hiding this comment.
And then add it to the productive feature next week?
No, the whole problem currently is that platform do not require this at all... it is only required by a test-fragment at the moment. We do not need (or should) add it to any feature, like we do not add any 3rdparty dependencies to features for quite some time now.
Ad feature versions are hard requirements, they often cause numerous issues and require versionbumps if they are not part of the same build, and for me SWT is a 3rd party dependency ...
|
When blocking further progress, please propose exactly what solution you expect (in the best case via a PR) and please explain why it will be better to have test feature explicitly including a fragment and productive products implicitly including it via capabilities, makes everything more confusing. The "correct" solution would be to improve the overall build process (so that the test product includes the fragment also via capability), but that's obviously out of scope. |
If you are hunting for the "correct" way than we should not include a feature that is nowhere used. So basically we now have a feature in SWT that is nowhere used in platform, so the platform test fail .. that's really bad but somehow expected. So whatever collects all test-bundles into the product should include whats only needed for the tests, and we should not include it somewhere "just in case". e4.rcp feature is that feature every RCP application depends on (not only platform / IDE). A while back I tried to replace the "Eclipse Testing Framework" but it was said that is works great and we should not touch it, so it seems fair that from time to time we need to invest extra time in keeping it updated. Given that limiations, I would add the fragment to the |
My comment was supposed to express that we cannot hunt for the "correct" way anyway, so we need to find the best "pragmatic" one. I don't think it is expected that SWT tests with a defined required capability fail because that capability is not provided as no productive consumer is there. I would actually expect the mechanism creating the test product to also consider the dependencies of the test bundles added to the SDK. Everyone depending on Since I don't want to invest more into this rather pointless discussion, here we go: eclipse-platform/eclipse.platform.releng.aggregator#2948 |
The SWT test are running fine (apart from some known flapping one) as far as I can see.
Obviously it does, otherwise it would not fail :-) Because of this I always liked to see that SWT becomes a standalone project with its own build, its on deployment / tests and its own releases... this would make thing much more decoupled, but it seems I'm the only one seeing benefits in less coupling.
There is a very subtile difference between a (transitive) dependency and a bundle mentioned in a feature, a transitive dependency can be updated independently from a feature (if the version range matches), a feature does lock down an exact version what together with the excessive use of singleton bundles makes it impossible to upgrade the SVG bundle if we mention it in any features. |
No, it fails because when starting the product it cannot resolve the requirement. The mechanism creating the product does not consider it, which is why it is kind of "incomplete", e.g.: https://download.eclipse.org/eclipse/downloads/drops4/I20250326-1800/download.php?dropFile=eclipse-Automated-Tests-I20250326-1800.zip
I don't think it makes sense broaden this up further and have general discussions here. Feel free to proceed in that referenced discussion. Would just be great to not spread generic, misleading statements, like me not seeing benefits in less coupling. It's just that only seldom decisions are one-dimensional, which is why I weigh arguments against each other to find conclusions. |
Are we talking about the same issue or has the error changed? says:
That is an error from the P2 director that installing the test bundle is not possible, if it would be an error when starting the product the message would be different and the fragment would simply not be attached (or the bundle not resolve). |
|
If still relevant, please read again the thread in eclipse-platform/eclipse.platform.swt#1944. The SDK test product used for running the tests does not contain the mentioned fragment. So the mechanism generating that product does not consider the dependency of |
HannesWell
force-pushed
the
svg-to-feature
branch
from
40c8387 to
ff3cb48
Compare
There was a problem hiding this comment.
I can't see any "consistency" reasons here to be honest, as mentioned earlier, please do not add anything that is included automatically to a feature (if it is not part of the project).
This will just lock down the version of the dependency and make it harder to upgrade.
|
I think that not adding it will actually make it harder to upgrade it because p2 tends to be conservative and does not do "unnecessary" deep updates: So it might well be that there are updates to the fragment available that would not be upgraded unless something specifies a lower bound that would cause an update. Or? I think if we generalized "please don't add anything included automatically" to also apply for bundles and not just for fragments we'd be removing a lot of bundles that are transitively required wouldn't we? And would that be a good thing? |
"deep updates" is actually something completely different, and only mean that p2 will not offer an (UI) update for this, this does not mean it won't update anything. In any case if someone is concerned, it is not the responsibility of platform.ui feature to ensure this but swt shoudl offer a feature then.
From my experience P2 does update transitive things if I update the "parent" IU. And even if not, if nothing requires a higher version there is no immediate need to update. Still I can explicitly install an update on my own.
Yes this would actually be the case, e.g we include a lot of third party stuff here that should likely be removed instead of making the feature more eager to require more, if we absolutely want this it would be better to import things like we do for emf... so interestingly no one is concerned about emf not being updated even though not explicitly required here :-)
so yes I think it would be a good thing and we already did it in the past without any negative impact reported so far. |
|
You appear to argue now that the feature should not include org.eclipse.swt nor the fragments:
Let's do a little though experiment. Suppose the SVG fragment is updated and only that fragment's version is updated. So there is a new update site and the only difference is a new version of the SVG fragment. Running Check for Updates would not update it because that process looks for a feature with a higher version number so that it can make a request to replace the older feature with the newer feature. Ergo, not including the fragment in the feature makes it hard to update the fragment. This does not seem like a good thing. |
That's the point, currently we have no SWT features (except the
Taking your example the other way round, suppose only SVG fragment is updated, that means we need to bump |
The SVG fragment needs to be available in runtime and SDK products for the platform. This adds it to the feature that already includes the SWT bundle and native fragments.
merks
dismissed
laeubi’s stale review
As discussed, revisiting how the feature structure is something we can revisit in the future.
The SVG fragment needs to be available in runtime and SDK products for the platform. This adds it to the feature that already includes the SWT bundle and native fragments.
This is an extract of #2593 only containing the feature changes (potential other changes to be discussed in that other PR).
Fixes eclipse-platform/eclipse.platform.swt#1944