You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "charangowdamd-cmd (via GitHub)" <gi...@apache.org> on 2026/10/16 19:08:42 UTC
[PR] [3.10.x] Fix #13583: Allow core extensions to see container components with Sisu 1.1.0 [maven]
charangowdamd-cmd opened a new pull request, #13584:
URL: https://github.com/apache/maven/pull/13584
Fixes #13583
### Description
In Eclipse Sisu 1.1.0, JSR-330 bean visibility filtering was introduced. When core extensions are present:
- Maven creates a container realm maven.ext with plexus.core as parent.
- maven.ext imports each core extension realm's exported items.
- However, the core extension realms never imported maven.ext.
- When an extension's lifecycle participant executes, DefaultMaven sets the TCCL to the extension's ClassRealm.
- Any subsequent programmatic or lazy component lookup/injection failed because Sisu filtered out all components belonging to maven.ext (all core components).
### Proposed Fix
In MavenCli.setupContainerRealm, have each core extension realm import the container realm ( ealm.importFrom(extRealm, extRealm.getId())), establishing visibility from the extension realm to the container realm within Sisu without requiring changes to Sisu itself.
Also added estCoreExtensionComponentVisibility in MavenCliTest to verify that components registered in the container realm are visible and resolvable when TCCL is set to the core extension realm.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [3.10.x] Fix #13583: Allow core extensions to see container components with Sisu 1.1.0 [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet closed pull request #13584: [3.10.x] Fix #13583: Allow core extensions to see container components with Sisu 1.1.0
URL: https://github.com/apache/maven/pull/13584
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [3.10.x] Fix #13583: Allow core extensions to see container components with Sisu 1.1.0 [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #13584:
URL: https://github.com/apache/maven/pull/13584#issuecomment-6343166030
Thank you for the backport PR and the detailed analysis!\n\nAfter discussion, we're closing this in favour of #13582, which was already merged into and fixes the same issue (#13583) using a different approach: rather than altering the classloader realm hierarchy with a reverse , it uses Sisu's API to bypass JSR330 filtering when the TCCL is a core extension realm.\n\nTamas's concern about altering classloading in Maven 3.x is valid, and #13582 addresses the bug without touching the realm import graph. The approach from this PR is already the fix on / Maven 4 (via #12554), where classloading is handled differently.\n\nThanks again for the contribution!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [3.10.x] Fix #13583: Allow core extensions to see container components with Sisu 1.1.0 [maven]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13584:
URL: https://github.com/apache/maven/pull/13584#issuecomment-6222798150
<!-- license-declaration-check -->
### ⚠️ License declaration missing
The pull request description must contain at least one of the following lines with a **checked** box (`[x]`):
```markdown
- [ ] I hereby declare this contribution to be licensed under the [Apache License Version 2.0, January 2004](https://www.apache.org/licenses/LICENSE-2.0)
- [ ] I hereby declare that I have filed an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
```
This comment will be hidden automatically once the description is fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [3.10.x] Fix #13583: Allow core extensions to see container components with Sisu 1.1.0 [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #13584:
URL: https://github.com/apache/maven/pull/13584#discussion_r4298444852
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -978,6 +982,10 @@ private ClassRealm setupContainerRealm(
// sisu uses realm imports to establish component visibility
extRealm.importFrom(realm, realm.getId());
}
+ if (realm != null) {
Review Comment:
⚠️ **Dead code.** `realm` is unconditionally dereferenced at line 983 via `extRealm.importFrom(realm, realm.getId())` — if `realm` were `null`, a `NullPointerException` would have already been thrown there. This guard can never be `false` in practice. Remove it.
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -952,18 +952,22 @@ private List<CoreExtension> selectCoreExtensions(Map<Path, List<CoreExtension>>
return new ArrayList<>(selectedExtensions.values());
}
- private ClassRealm setupContainerRealm(
+ ClassRealm setupContainerRealm(
Review Comment:
💡 The visibility was widened from `private` to package-private solely for test access. Please annotate with `@VisibleForTesting` (from Guava or an equivalent) to make the intent explicit:
```suggestion
@VisibleForTesting
ClassRealm setupContainerRealm(
```
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -952,18 +952,22 @@ private List<CoreExtension> selectCoreExtensions(Map<Path, List<CoreExtension>>
return new ArrayList<>(selectedExtensions.values());
}
- private ClassRealm setupContainerRealm(
+ ClassRealm setupContainerRealm(
ClassWorld classWorld, ClassRealm coreRealm, List<File> extClassPath, List<CoreExtensionEntry> extensions)
throws Exception {
if (!extClassPath.isEmpty() || !extensions.isEmpty()) {
ClassRealm extRealm = classWorld.newRealm("maven.ext", null);
extRealm.setParentRealm(coreRealm);
- slf4jLogger.debug("Populating class realm {}", extRealm.getId());
+ if (slf4jLogger != null) {
+ slf4jLogger.debug("Populating class realm {}", extRealm.getId());
+ }
for (File file : extClassPath) {
- slf4jLogger.debug(" Included {}", file);
+ if (slf4jLogger != null) {
+ slf4jLogger.debug(" Included {}", file);
+ }
Review Comment:
⚠️ **Test-only null guards polluting production code.** The original code called `slf4jLogger.debug(...)` unconditionally, proving `slf4jLogger` is always non-null in production. These guards exist purely to let the new test call `setupContainerRealm` on a partially-initialized `MavenCli` instance. The right fix is to initialize `slf4jLogger` in the test via reflection rather than weakening the production invariant:
```java
Field f = MavenCli.class.getDeclaredField("slf4jLogger");
f.setAccessible(true);
f.set(cli, LoggerFactory.getLogger(MavenCli.class));
```
Then these guards can be removed.
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -978,6 +982,10 @@ private ClassRealm setupContainerRealm(
// sisu uses realm imports to establish component visibility
extRealm.importFrom(realm, realm.getId());
}
+ if (realm != null) {
+ // sisu uses realm imports to establish component visibility
Review Comment:
💡 **Duplicated comment.** This is a verbatim copy of the comment on line 982, but the two `importFrom` calls have opposite directions and different purposes. The forward import (`extRealm.importFrom(realm, ...)`) establishes package visibility. The new reverse import makes Sisu discover components registered in the container realm from within the extension realm — worth a distinct comment, e.g.:
```suggestion
// reverse import: allows Sisu to look up container components from within the extension realm
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [3.10.x] Fix #13583: Allow core extensions to see container components with Sisu 1.1.0 [maven]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #13584:
URL: https://github.com/apache/maven/pull/13584#issuecomment-6290938412
I am -1 on this change, because it _alters classloading_, something IMHO we should not do in Maven 3.x. What happens in 4+ is different story. Also, there is an alternate PR for this fix: #13582
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [3.10.x] Fix #13583: Allow core extensions to see container components with Sisu 1.1.0 [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #13584:
URL: https://github.com/apache/maven/pull/13584#discussion_r4301495654
##########
maven-embedder/src/test/java/org/apache/maven/cli/MavenCliTest.java:
##########
@@ -607,4 +617,63 @@ void skipsCheckWhenAlternatePomFileSpecified() throws Exception {
assertNull(ex, "Expected no ExitException when -f/--file is specified");
}
}
+
+ @Test
+ public void testCoreExtensionComponentVisibility() throws Exception {
+ Field f = MavenCli.class.getDeclaredField("slf4jLogger");
+ f.setAccessible(true);
+ f.set(cli, LoggerFactory.getLogger(MavenCli.class));
+
+ ClassWorld classWorld = new ClassWorld();
+ ClassRealm coreRealm =
+ classWorld.newRealm("plexus.core", Thread.currentThread().getContextClassLoader());
+ ClassRealm extRealm1 =
+ classWorld.newRealm("coreExtension>ext1", Thread.currentThread().getContextClassLoader());
+
+ CoreExtensionEntry entry = new CoreExtensionEntry(extRealm1, Collections.emptyList(), Collections.emptyList());
+ ClassRealm containerRealm = cli.setupContainerRealm(
+ classWorld, coreRealm, Collections.emptyList(), Collections.singletonList(entry));
+
+ assertTrue(
+ extRealm1.getImportRealms().contains(containerRealm),
+ "Extension realm must import container realm for Sisu component visibility");
+
+ ContainerConfiguration cc = new DefaultContainerConfiguration()
+ .setClassWorld(classWorld)
+ .setRealm(containerRealm)
+ .setClassPathScanning(PlexusConstants.SCANNING_INDEX)
+ .setAutoWiring(true)
+ .setJSR250Lifecycle(true)
+ .setName("maven");
+
+ DefaultPlexusContainer container = new DefaultPlexusContainer(cc);
+ try {
+ container.setLookupRealm(null);
+
+ ComponentDescriptor<Runnable> descriptor = new ComponentDescriptor<>();
+ descriptor.setRole(Runnable.class.getName());
+ descriptor.setRoleHint("test");
+ descriptor.setImplementationClass(MyLazyComponent.class);
+ container.addComponentDescriptor(descriptor);
Review Comment:
💡 **Test design note:** `addComponentDescriptor` registers the component via the Plexus classic path, which bypasses Sisu's JSR330 index-based discovery. This means the `container.lookup()` call below tests Plexus resolution rather than Sisu visibility filtering.
The real validation of the fix is the structural assertion above — `extRealm1.getImportRealms().contains(containerRealm)` directly verifies the new `realm.importFrom(extRealm, extRealm.getId())` line. You might consider noting that distinction, or simplifying the lookup portion since it doesn't exercise the Sisu path.
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -48,6 +48,7 @@
import java.util.regex.Pattern;
import java.util.stream.Stream;
+import com.google.common.annotations.VisibleForTesting;
Review Comment:
💡 **Style:** `@VisibleForTesting` from Guava is used here solely to document that `setupContainerRealm` is package-private for test access. Consider replacing the annotation with a simple comment — it conveys the same intent without introducing a Guava annotation into production code:
```suggestion
```
(and inline on the method: `/* package-private for testing */`)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org