Skip to content

Consume SVG rasterizer via capability in SWT tests bundle #1946

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

Merged

Conversation

HeikoKlare
Copy link
Contributor

Currently, requiring an SVG rasterizer needs manual placement of an according fragment on the classpath. With this change, requiring SVG support can be defined explicitly via a required capability that is provided by the according SWT fragment.

Contributes to #1944

@HeikoKlare
Copy link
Contributor Author

@laeubi I am not sure whether this is the correct way of doing it, but at least it seems to work when locally executing the build.

I also tried with a "custom" capability (not using osgi.extender) but that did not work.

Copy link
Contributor

@laeubi laeubi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im not sure if this is really an extender

An Extender is a bundle that uses the life cycle events from another bundle, the extendee, to extend that bundle's functionality when that bundle is active. It can use metadata (headers, or files inside the extendee) to control its functionality.

I have added a suggestion how I would assume it should work to use a custom namespace, or maybe osgi.implementation would be more suitable

The Implementation Namespace is intended to be used for:
Preventing a bundle from resolving if there is not at least one bundle that provides an implementation of the specified specification or contract.

then it might look similar to

Provide-Capability: osgi.implementation;
    osgi.implementation="swt.svg";
    version:Version="1.0";
    uses:="<packages that are used e.g. jsvg>"

@@ -11,3 +11,4 @@ Import-Package: com.github.weisj.jsvg;version="[1.7.0,2.0.0)",
com.github.weisj.jsvg.geometry.size;version="[1.7.0,2.0.0)",
com.github.weisj.jsvg.parser;version="[1.7.0,2.0.0)"
Export-Package: org.eclipse.swt.svg
Provide-Capability: osgi.extender;osgi.extender="org.eclipse.swt.svgrasterizer"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Provide-Capability: osgi.extender;osgi.extender="org.eclipse.swt.svgrasterizer"
Provide-Capability: swt.renderer;swt.renderer="svg"

@@ -20,3 +20,4 @@ Require-Bundle: org.junit;bundle-version="4.13.2",
Eclipse-BundleShape: dir
Bundle-RequiredExecutionEnvironment: JavaSE-17
Automatic-Module-Name: org.eclipse.swt.tests
Require-Capability: osgi.extender;filter:="(osgi.extender=org.eclipse.swt.svgrasterizer)"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Require-Capability: osgi.extender;filter:="(osgi.extender=org.eclipse.swt.svgrasterizer)"
Require-Capability: swt.renderer;filter:="(swt.renderer=svg)"

Copy link
Contributor

github-actions bot commented Mar 26, 2025

Test Results

   545 files  ±0     545 suites  ±0   31m 40s ⏱️ + 5m 13s
 4 367 tests ±0   4 351 ✅ ±0   16 💤 ±0  0 ❌ ±0 
16 616 runs  ±0  16 502 ✅ ±0  114 💤 ±0  0 ❌ ±0 

Results for commit 69fe298. ± Comparison against base commit d8b1f98.

♻️ This comment has been updated with latest results.

@HeikoKlare HeikoKlare force-pushed the svgrasterizer-capability-tests branch from 63b4021 to 8e69c6a Compare March 26, 2025 10:16
@HeikoKlare
Copy link
Contributor Author

Thank you for the pointer! The osgi.implementation namespace sounds like the exact right thing for this. I have adapted the PR accordingly.

@HeikoKlare HeikoKlare marked this pull request as ready for review March 26, 2025 10:18
@HeikoKlare HeikoKlare force-pushed the svgrasterizer-capability-tests branch from 8e69c6a to 179ee13 Compare March 26, 2025 12:09
@HeikoKlare
Copy link
Contributor Author

@laeubi are you fine with these changes now and would you expect them to fix #1944?

In that case, I would propose to merge and maybe trigger a new I-Build to see if tests are running fine again.

@HeikoKlare HeikoKlare force-pushed the svgrasterizer-capability-tests branch from 179ee13 to fc6825b Compare March 26, 2025 12:46
Copy link
Contributor

@laeubi laeubi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

Comment on lines 14 to 16
Provide-Capability: osgi.implementation;
osgi.implementation="swt.svgrasterizer";
version:Version="1.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the given reference I have the impression that the osgi.implementation namespace is inteded for implementations of OSGi specifications not arbitrary specs.
While it would probably not be harmful to use the osgi-namespace, I think a custom one would be more suitable, for example:

Suggested change
Provide-Capability: osgi.implementation;
osgi.implementation="swt.svgrasterizer";
version:Version="1.0"
Provide-Capability: eclipse.swt.implementation;image.format="svg";version:Version="1.0"

Later we could then also use this namespace to introduce a requirement of o.e.swt to it's name fragments that could then provide Provide-Capability: eclipse.swt.implementation;implementation="native" or similar.

@HeikoKlare HeikoKlare force-pushed the svgrasterizer-capability-tests branch from fc6825b to 52daa9a Compare March 26, 2025 14:57
Currently, requiring an SVG rasterizer needs manual placement of an
according fragment providing a rasterizer on the classpath. With this
change, requiring SVG support can be defined explicitly via a required
capability that is provided by the according SWT fragment.

Contributes to
eclipse-platform#1944
@HeikoKlare HeikoKlare force-pushed the svgrasterizer-capability-tests branch from 52daa9a to 69fe298 Compare March 26, 2025 15:28
Copy link
Member

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update, this is more compact and a bit more generic.

@HeikoKlare
Copy link
Contributor Author

Thank you all for your input! Hopefully this even fixes #1944.

@HeikoKlare HeikoKlare merged commit 536fea8 into eclipse-platform:master Mar 26, 2025
15 checks passed
@HeikoKlare HeikoKlare deleted the svgrasterizer-capability-tests branch March 26, 2025 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants