You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "oehme (via GitHub)" <gi...@apache.org> on 2023/01/30 19:45:39 UTC
[GitHub] [maven-mvnd] oehme opened a new pull request, #784: Make Classworld setup more alike to vanilla Maven
oehme opened a new pull request, #784:
URL: https://github.com/apache/maven-mvnd/pull/784
Instead of providing a custom URLClassLoader, create the ClassWorld using the plexus configurator and then pass it down the call chain.
This improves compatbility with any extensions or plugins that assume that their ClassLoader is a ClassRealm.
--
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
[GitHub] [maven-mvnd] gnodet commented on a diff in pull request #784: Make Classworld setup more alike to vanilla Maven
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #784:
URL: https://github.com/apache/maven-mvnd/pull/784#discussion_r1091091086
##########
common/src/main/java/org/mvndaemon/mvnd/common/MavenDaemon.java:
##########
@@ -18,67 +18,39 @@
*/
package org.mvndaemon.mvnd.common;
-import java.net.MalformedURLException;
-import java.net.URL;
-import java.net.URLClassLoader;
+import java.io.InputStream;
+import java.lang.reflect.Constructor;
import java.nio.file.Files;
import java.nio.file.Path;
-import java.util.stream.Stream;
+
+import org.codehaus.plexus.classworlds.ClassWorld;
+import org.codehaus.plexus.classworlds.launcher.Launcher;
+import org.codehaus.plexus.classworlds.realm.ClassRealm;
public class MavenDaemon {
public static void main(String[] args) throws Exception {
- final Path mvndHome = Environment.MVND_HOME.asPath();
- URL[] classpath = Stream.concat(
- /* jars */
- Stream.of("lib/ext", "lib", "boot")
- .map(mvndHome::resolve)
- .flatMap((Path p) -> {
- try {
- return Files.list(p);
- } catch (java.io.IOException e) {
- throw new RuntimeException("Could not list " + p, e);
- }
- })
- .filter(p -> {
- final String fileName = p.getFileName().toString();
- return fileName.endsWith(".jar") && !fileName.startsWith("mvnd-client-");
- })
- .filter(Files::isRegularFile),
- /* resources */
- Stream.of(mvndHome.resolve("conf"), mvndHome.resolve("conf/logging")))
- .map(Path::normalize)
- .map(Path::toUri)
- .map(uri -> {
- try {
- return uri.toURL();
- } catch (MalformedURLException e) {
- throw new RuntimeException(e);
- }
- })
- .toArray(URL[]::new);
- ClassLoader loader = new URLClassLoader(classpath, null) {
+ Path mvndHome = Environment.MVND_HOME.asPath();
+ Launcher launcher = new Launcher();
+ launcher.setSystemClassLoader(new ClassLoader(ClassLoader.getPlatformClassLoader()) {
+
@Override
protected Class<?> findClass(String name) throws ClassNotFoundException {
- try {
- return super.findClass(name);
- } catch (ClassNotFoundException e) {
+ if (name.startsWith("org.codehaus.plexus.classworlds.")
+ || name.startsWith("org.codehaus.classworlds.")) {
return MavenDaemon.class.getClassLoader().loadClass(name);
}
+ throw new ClassNotFoundException(name);
}
-
- @Override
- public URL getResource(String name) {
- URL url = super.getResource(name);
- if (url == null) {
- url = MavenDaemon.class.getClassLoader().getResource(name);
- }
- return url;
- }
- };
- Thread.currentThread().setContextClassLoader(loader);
- Class<?> clazz = loader.loadClass("org.mvndaemon.mvnd.daemon.Server");
- try (AutoCloseable server = (AutoCloseable) clazz.getConstructor().newInstance()) {
+ });
+ try (InputStream in = Files.newInputStream(mvndHome.resolve("bin/m2.conf"))) {
Review Comment:
That looks wrong. We should have a custom config that properly boots the `org.mvndaemon.mvnd.daemon.Server`.
##########
client/src/main/java/org/mvndaemon/mvnd/client/DaemonConnector.java:
##########
@@ -339,22 +339,34 @@ private Process startDaemonProcess(String daemonId, ClientOutput output) {
final Path mvndHome = parameters.mvndHome();
final Path workingDir = parameters.userDir();
String command = "";
- try (DirectoryStream<Path> jarPaths =
- Files.newDirectoryStream(mvndHome.resolve("lib").resolve("ext"))) {
+ try {
List<String> args = new ArrayList<>();
// executable
final String java = Os.current().isUnixLike() ? "bin/java" : "bin\\java.exe";
args.add(parameters.javaHome().resolve(java).toString());
// classpath
String mvndCommonPath = null;
String mvndAgentPath = null;
- for (Path jar : jarPaths) {
- String s = jar.getFileName().toString();
- if (s.endsWith(".jar")) {
- if (s.startsWith("mvnd-common-")) {
- mvndCommonPath = jar.toString();
- } else if (s.startsWith("mvnd-agent-")) {
- mvndAgentPath = jar.toString();
+ String plexusClassworldsPath = null;
+ try (DirectoryStream<Path> jarPaths = Files.newDirectoryStream(mvndHome.resolve("lib/ext"))) {
Review Comment:
The implementation of `resolve` implies the usage of the file separator, which is OS specific.
--
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
[GitHub] [maven-mvnd] oehme commented on a diff in pull request #784: Make Classworld setup more alike to vanilla Maven
Posted by "oehme (via GitHub)" <gi...@apache.org>.
oehme commented on code in PR #784:
URL: https://github.com/apache/maven-mvnd/pull/784#discussion_r1097293553
##########
common/src/main/java/org/mvndaemon/mvnd/common/MavenDaemon.java:
##########
@@ -18,67 +18,39 @@
*/
package org.mvndaemon.mvnd.common;
-import java.net.MalformedURLException;
-import java.net.URL;
-import java.net.URLClassLoader;
+import java.io.InputStream;
+import java.lang.reflect.Constructor;
import java.nio.file.Files;
import java.nio.file.Path;
-import java.util.stream.Stream;
+
+import org.codehaus.plexus.classworlds.ClassWorld;
+import org.codehaus.plexus.classworlds.launcher.Launcher;
+import org.codehaus.plexus.classworlds.realm.ClassRealm;
public class MavenDaemon {
public static void main(String[] args) throws Exception {
- final Path mvndHome = Environment.MVND_HOME.asPath();
- URL[] classpath = Stream.concat(
- /* jars */
- Stream.of("lib/ext", "lib", "boot")
- .map(mvndHome::resolve)
- .flatMap((Path p) -> {
- try {
- return Files.list(p);
- } catch (java.io.IOException e) {
- throw new RuntimeException("Could not list " + p, e);
- }
- })
- .filter(p -> {
- final String fileName = p.getFileName().toString();
- return fileName.endsWith(".jar") && !fileName.startsWith("mvnd-client-");
- })
- .filter(Files::isRegularFile),
- /* resources */
- Stream.of(mvndHome.resolve("conf"), mvndHome.resolve("conf/logging")))
- .map(Path::normalize)
- .map(Path::toUri)
- .map(uri -> {
- try {
- return uri.toURL();
- } catch (MalformedURLException e) {
- throw new RuntimeException(e);
- }
- })
- .toArray(URL[]::new);
- ClassLoader loader = new URLClassLoader(classpath, null) {
+ Path mvndHome = Environment.MVND_HOME.asPath();
+ Launcher launcher = new Launcher();
+ launcher.setSystemClassLoader(new ClassLoader(ClassLoader.getPlatformClassLoader()) {
+
@Override
protected Class<?> findClass(String name) throws ClassNotFoundException {
- try {
- return super.findClass(name);
- } catch (ClassNotFoundException e) {
+ if (name.startsWith("org.codehaus.plexus.classworlds.")
+ || name.startsWith("org.codehaus.classworlds.")) {
return MavenDaemon.class.getClassLoader().loadClass(name);
}
+ throw new ClassNotFoundException(name);
}
-
- @Override
- public URL getResource(String name) {
- URL url = super.getResource(name);
- if (url == null) {
- url = MavenDaemon.class.getClassLoader().getResource(name);
- }
- return url;
- }
- };
- Thread.currentThread().setContextClassLoader(loader);
- Class<?> clazz = loader.loadClass("org.mvndaemon.mvnd.daemon.Server");
- try (AutoCloseable server = (AutoCloseable) clazz.getConstructor().newInstance()) {
+ });
+ try (InputStream in = Files.newInputStream(mvndHome.resolve("bin/m2.conf"))) {
Review Comment:
Fixed and as a result I could also remove the custom ClassRealmManager, making the code even simpler.
--
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
[GitHub] [maven-mvnd] oehme commented on pull request #784: Make Classworld setup more alike to vanilla Maven
Posted by "oehme (via GitHub)" <gi...@apache.org>.
oehme commented on PR #784:
URL: https://github.com/apache/maven-mvnd/pull/784#issuecomment-1445304171
Yes, the Gradle Enterprise Maven extension does some "importRealm" magic, so that other extensions (written by customers) can access its API no matter how they were registered (ext/lib, maven.ext.classpath, .mvn/extensions.xml)
--
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
[GitHub] [maven-mvnd] lppedd commented on pull request #784: Make Classworld setup more alike to vanilla Maven
Posted by "lppedd (via GitHub)" <gi...@apache.org>.
lppedd commented on PR #784:
URL: https://github.com/apache/maven-mvnd/pull/784#issuecomment-1445194111
@oehme hi! Just passing by and wondering, in which case a plugin/extension would want to explicitly check for a class loader type? Did you think about this because of some real world use case?
--
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
[GitHub] [maven-mvnd] oehme commented on pull request #784: Make Classworld setup more alike to vanilla Maven
Posted by "oehme (via GitHub)" <gi...@apache.org>.
oehme commented on PR #784:
URL: https://github.com/apache/maven-mvnd/pull/784#issuecomment-1419032775
That windows failure looks unrelated, it couldn't delete the daemon registry.
--
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
[GitHub] [maven-mvnd] oehme commented on a diff in pull request #784: Make Classworld setup more alike to vanilla Maven
Posted by "oehme (via GitHub)" <gi...@apache.org>.
oehme commented on code in PR #784:
URL: https://github.com/apache/maven-mvnd/pull/784#discussion_r1097292232
##########
daemon/src/main/java/org/apache/maven/cli/DaemonMavenCli.java:
##########
@@ -473,6 +472,7 @@ DefaultPlexusContainer container() throws Exception {
List<File> extClassPath = Stream.of(
Environment.MVND_EXT_CLASSPATH.asString().split(","))
+ .filter(s -> s != null && !s.isEmpty())
Review Comment:
The missing empty filter here lead to the empty String being added to the extClassPath list, which made the classloader structure slightly different from vanilla Maven in some scenarios. Not a big deal in practice, but it broke some of our tests, so I decided to fix it for consistency :)
--
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
[GitHub] [maven-mvnd] oehme commented on pull request #784: Make Classworld setup more alike to vanilla Maven
Posted by "oehme (via GitHub)" <gi...@apache.org>.
oehme commented on PR #784:
URL: https://github.com/apache/maven-mvnd/pull/784#issuecomment-1409272433
FYI the builds are failing since https://github.com/apache/maven-mvnd/commit/bf892cd1bf10083448a9981b61615774869bf16b - Seems those artifacts aren't there yet. I'll rebase this branch on an earlier commit to get some CI feedback in the meantime.
--
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
[GitHub] [maven-mvnd] oehme commented on pull request #784: Make Classworld setup more alike to vanilla Maven
Posted by "oehme (via GitHub)" <gi...@apache.org>.
oehme commented on PR #784:
URL: https://github.com/apache/maven-mvnd/pull/784#issuecomment-1422818492
@gnodet this is ready for another look
--
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
[GitHub] [maven-mvnd] gnodet merged pull request #784: Make Classworld setup more alike to vanilla Maven
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet merged PR #784:
URL: https://github.com/apache/maven-mvnd/pull/784
--
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