[
https://issues.apache.org/jira/browse/GROOVY-12690?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18137804#comment-18137804
]
ASF GitHub Bot commented on GROOVY-12690:
-----------------------------------------
codecov-commenter commented on PR #3209:
URL: https://github.com/apache/groovy/pull/3209#issuecomment-6624915462
##
[Codecov](https://app.codecov.io/gh/apache/groovy/pull/3209?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 74.1505%. Comparing base
([`9a1a0d0`](https://app.codecov.io/gh/apache/groovy/commit/9a1a0d0ccbc9a1eacd85cbbd408374f6c7f41fc8?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache))
to head
([`eec9172`](https://app.codecov.io/gh/apache/groovy/commit/eec91727482d7eaa410b56e6bbaed5b2e32998c1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 7 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/groovy/pull/3209?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #3209 +/- ##
==================================================
- Coverage 74.1518% 74.1505% -0.0013%
+ Complexity 43728 43725 -3
==================================================
Files 1692 1692
Lines 149527 149527
Branches 28360 28360
==================================================
- Hits 110877 110875 -2
- Misses 28438 28441 +3
+ Partials 10212 10211 -1
```
[see 12 files with indirect coverage
changes](https://app.codecov.io/gh/apache/groovy/pull/3209/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test
Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests,
report on failures, and find test suite problems.
- :package: [JS Bundle
Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save
yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
> Make the ServiceLoader-mediator requirements in module OSGi manifests
> optional, and extend groovy-osgi-test to cover Equinox and SPI Fly
> ----------------------------------------------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12690
> URL: https://issues.apache.org/jira/browse/GROOVY-12690
> Project: Groovy
> Issue Type: Bug
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> h2. Background
> Groovy's optional modules are published as OSGi fragments of the core
> {{groovy}} bundle ({{Fragment-Host: groovy}},
> GROOVY-8666/GROOVY-9402/GROOVY-11570), and the core bundle carries
> {{Eclipse-ExtensibleAPI: true}} for PDE (GROOVY-8713). Any module that ships
> a non-AST {{META-INF/services}} file also gets these ServiceLoader Mediator
> headers (GROOVY-8766, restored in GROOVY-10881):
> {code}
> Require-Capability:
> osgi.extender;filter:="(osgi.extender=osgi.serviceloader.processor)",
> osgi.extender;filter:="(osgi.extender=osgi.serviceloader.registrar)",
>
> osgi.serviceloader;filter:="(osgi.serviceloader=<service>)";cardinality:=multiple
> Provide-Capability: osgi.serviceloader;osgi.serviceloader="<service>"
> {code}
> The two {{osgi.extender}} requirements are *mandatory*. Only a ServiceLoader
> Mediator implementation (in practice Apache Aries SPI Fly) provides them. In
> a framework without SPI Fly these modules never resolve, so the fragment
> never attaches and its packages are not visible at all. This currently
> affects groovy-json, groovy-reactor, groovy-rxjava, groovy-test-junit5,
> groovy-test-junit6, groovy-testng, groovy-contracts and groovy-jsr223 (and
> also groovy-grape-ivy and groovy-grape-maven, although Grape's runtime
> dependency resolution is not meaningful inside an OSGi container).
> Historically, Groovy's OSGi users have fallen into two broad camps:
> * Felix-based containers (Karaf, Sling/AEM), which typically install SPI Fly
> (GROOVY-10881, GROOVY-11578).
> * Equinox / Eclipse RCP (GROOVY-8666, GROOVY-8713).
> Fixes for one camp have repeatedly broken the other. {{groovy-osgi-test}}
> (GROOVY-11898) was meant to break that cycle. However, it only covers the
> core bundle on Felix, and its two groovy-json tests are {{@Disabled}} (citing
> the unrelated GROOVY-5092). They fail because the test framework has no SPI
> Fly, so the fragment stays {{INSTALLED}}.
> h2. Investigation
> An embedded-framework probe (plain {{FrameworkFactory}} via
> {{ServiceLoader}}) installed the Groovy jars plus manifest-only consumer
> bundles. The consumers used either {{Import-Package: groovy.json}} or
> {{Require-Bundle: groovy}} and called {{JsonSlurper.parseText}}. The core
> bundle and consumers were started, as a real container would start them. The
> matrix covered:
> * *Frameworks:* Felix 7.0.5, Felix 8.0.0 and Equinox ({{org.eclipse.osgi}}
> 3.24.300).
> * *SPI Fly:* none, 1.2.4, or 1.3.8
> ({{org.apache.aries.spifly.dynamic.bundle}} + ASM).
> * *Manifests:* as built, or with the extender requirements marked
> {{resolution:=optional}}.
> * *Fallback:* {{FastStringUtils}} with its GROOVY-11578 default-service
> fallback, and a scratch build with the fallback removed.
> * *Thread context class loader (TCCL):* left as is, or set to the consumer
> bundle's class loader.
> h3. groovy-json, 7.0.0-SNAPSHOT, SPI Fly 1.3.8
> On Felix, the (/) results apply to {{Import-Package}} consumers only (see
> "Other findings").
> ||groovy-json variant||Felix, no SPI Fly||Felix + SPI Fly||Equinox, no SPI
> Fly||Equinox + SPI Fly||
> |mandatory requirements, fallback (current)|(x) fragment does not
> resolve|(/)|(x) fragment does not resolve|(/)|
> |mandatory requirements, no fallback|(x) fragment does not resolve|(/)|(x)
> fragment does not resolve|(/)|
> |optional requirements, fallback|(/)|(/)|(/)|(/)|
> |optional requirements, no fallback|(x) {{Unable to load
> FastStringService}}|(/)|(!) works only via Equinox's {{ContextFinder}} TCCL;
> fails if the caller sets its own TCCL|(/)|
> h3. groovy-reactor (core-side consumer: {{AwaitableAdapterRegistry}} uses its
> own class loader)
> ||groovy-reactor variant||No SPI Fly (Felix and Equinox)||With SPI Fly (Felix
> and Equinox)||
> |mandatory requirements (current)|(x) fragment does not resolve; only
> built-in adapters|(/) {{ReactorAwaitableAdapter}} discovered|
> |optional requirements|(/) {{ReactorAwaitableAdapter}} discovered|(/)|
> h3. Historical check: Groovy 4.0.26 (pre-fallback) from Maven Central
> ||SPI Fly||Felix 7.0.5||Felix 8.0.0||Equinox||
> |1.2.4|(x) weaving hook fails: {{Unsupported class file major version
> 61}}|(x) same|-|
> |1.3.8|(/)|(/)|(/)|
> h3. Conclusions
> # *SPI Fly mediation works for fragment-provided services.* With SPI Fly
> 1.3.x and the host bundle started, groovy-json works even without the
> fallback, on both Felix and Equinox. The Felix version (7.0.5 vs 8.0.0) makes
> no difference.
> # *Without SPI Fly, groovy-json only works because of the GROOVY-11578
> fallback.* {{FastStringUtils}} calls {{ServiceLoader.load(Class)}}, which
> uses the TCCL. Equinox's default {{ContextFinder}} TCCL masks this; an
> explicit TCCL does not.
> # *Core-side consumers do not need SPI Fly at all.*
> {{AwaitableAdapterRegistry}} passes its own class loader, and a fragment's
> {{META-INF/services}} is on the host class loader. For groovy-reactor, the
> mandatory requirement only stops the module from resolving.
> # *Marking the extender requirements optional loses nothing.* Every cell that
> works with mandatory requirements still works with optional ones, including
> SPI Fly mediation. The previously failing no-SPI-Fly cells start working.
> # *SPI Fly 1.2.4 is not viable on JDK 17+.* Its weaving hook fails even for
> Groovy 4.0.26, which is Java 8 bytecode. The likely cause is that its
> embedded ASM cannot read the JDK 17 runtime classes it inspects while
> weaving. So the GROOVY-11578 workaround of downgrading SPI Fly no longer
> applies; SPI Fly 1.3.x is required.
> # *The race described in GROOVY-11578 reproduces.* With the host resolved but
> not started, a consumer using JSON before SPI Fly has registered the provider
> fails without the fallback, and succeeds with it.
> h3. Other findings (not addressed here)
> * *Felix does not expose fragment-contributed exports to {{Require-Bundle}}
> consumers.* Equinox does. This reproduces with a minimal Groovy-free
> host/fragment/consumer, so it is framework behaviour rather than a Groovy
> manifest problem. On Felix, {{Import-Package}} works.
> * *{{Eclipse-ExtensibleAPI}} cannot be verified by a runtime test.* It is a
> PDE compile-time hint that Equinox ignores at runtime.
> h2. Proposed changes
> h3. 1. Make the ServiceLoader Mediator requirements optional
> In {{build-logic/src/main/groovy/org.apache.groovy-library.gradle}}:
> {code}
> instruction 'Require-Capability',
> 'osgi.extender;filter:="(osgi.extender=osgi.serviceloader.processor)";resolution:=optional'
> instruction 'Require-Capability',
> 'osgi.extender;filter:="(osgi.extender=osgi.serviceloader.registrar)";resolution:=optional'
> ...
> instruction 'Require-Capability',
> "osgi.serviceloader;filter:=\"(osgi.serviceloader=${it.name})\";cardinality:=multiple;resolution:=optional"
> {code}
> {{Provide-Capability}} is unchanged.
> * Where SPI Fly is installed, the optional requirements are still wired at
> resolve time and mediation still happens.
> * Where it is not, the modules now resolve. Services consumed via an explicit
> class loader work, and groovy-json works through its fallback.
> h3. 2. Extend groovy-osgi-test
> * *Equinox:* add a {{testEquinox}} task that runs the same tests with Felix
> swapped for Equinox on the classpath.
> * *SPI Fly as an optional configuration:* add an {{osgiSpifly}}
> configuration, not on the test classpath, containing SPI Fly 1.3.x and ASM
> (reusing Groovy's ASM version). Its jars are passed to the tests, which run
> each scenario both with and without SPI Fly installed.
> * *Replace the two {{@Disabled}} tests* with consumer-bundle tests
> ({{Import-Package}}, and {{Require-Bundle}} on Equinox) that exercise:
> ** *groovy-json resolution and use* ({{JsonSlurper.parseText}},
> {{JsonOutput.toJson}}), with and without SPI Fly.
> ** *groovy-reactor's {{AwaitableAdapter}} discovery*, a core-side
> ServiceLoader consumer, with and without SPI Fly.
> ** *SPI Fly mediation itself:* a small test-only fragment provides its own
> {{FastStringServiceFactory}}. {{FastStringUtils}} prefers a non-default
> service, so the test can assert that provider was used. That passes only if
> mediation actually happened, which the fallback would otherwise hide.
> ** *The GROOVY-11578 race:* JSON used while the host is resolved but not yet
> started.
> * *Record the Felix {{Require-Bundle}} limitation* as a documented expected
> behaviour rather than a failure.
> * *Pin versions* (SPI Fly 1.3.8, Equinox 3.24.300) and update
> {{gradle/verification-metadata.xml}}.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
