Skip to content
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.

Sign up for GitHub

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

Create a new base classloader including parent-first test scoped dependencies when bootstrapping for CT #30383

Merged
merged 1 commit into from
Mar 29, 2023
Merged

Create a new base classloader including parent-first test scoped dependencies when bootstrapping for CT #30383

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 0 additions & 7 deletions core/deployment/src/main/java/io/quarkus/deployment/dev/testing/JunitTestRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -751,7 +751,6 @@ static class Builder {
private TestType testType = TestType.ALL;
private TestState testState;
private long runId = -1;
private DevModeContext devModeContext;
private CuratedApplication testApplication;
private ClassScanResult classScanResult;
private TestClassUsages testClassUsages;
Expand Down Expand Up @@ -780,11 +779,6 @@ public Builder setTestType(TestType testType) {
return this;
}

public Builder setDevModeContext(DevModeContext devModeContext) {
this.devModeContext = devModeContext;
return this;
}

public Builder setTestApplication(CuratedApplication testApplication) {
this.testApplication = testApplication;
return this;
Expand Down Expand Up @@ -846,7 +840,6 @@ public Builder setExcludeEngines(List<String> excludeEngines) {
}

public JunitTestRunner build() {
Objects.requireNonNull(devModeContext, "devModeContext");
Objects.requireNonNull(testClassUsages, "testClassUsages");
Objects.requireNonNull(testApplication, "testApplication");
Objects.requireNonNull(testState, "testState");
Expand Down
5 changes: 1 addition & 4 deletions core/deployment/src/main/java/io/quarkus/deployment/dev/testing/ModuleTestRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,15 @@ public class ModuleTestRunner {

final TestState testState = new TestState();
private final TestSupport testSupport;
private final DevModeContext devModeContext;
private final CuratedApplication testApplication;
private final DevModeContext.ModuleInfo moduleInfo;

private final TestClassUsages testClassUsages = new TestClassUsages();
private JunitTestRunner runner;

public ModuleTestRunner(TestSupport testSupport, DevModeContext devModeContext, CuratedApplication testApplication,
public ModuleTestRunner(TestSupport testSupport, CuratedApplication testApplication,
DevModeContext.ModuleInfo moduleInfo) {
this.testSupport = testSupport;
this.devModeContext = devModeContext;
this.testApplication = testApplication;
this.moduleInfo = moduleInfo;
}
Expand All @@ -50,7 +48,6 @@ Runnable prepare(ClassScanResult classScanResult, boolean reRunFailures, long ru
}
JunitTestRunner.Builder builder = new JunitTestRunner.Builder()
.setClassScanResult(classScanResult)
.setDevModeContext(devModeContext)
.setRunId(runId)
.setTestState(testState)
.setTestClassUsages(testClassUsages)
Expand Down
143 changes: 105 additions & 38 deletions core/deployment/src/main/java/io/quarkus/deployment/dev/testing/TestSupport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,13 @@

import java.io.IOException;
import java.io.InputStream;
import java.io.UncheckedIOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
Expand All @@ -18,6 +17,7 @@
import java.util.Set;
import java.util.concurrent.CopyOnWriteArrayList;
import java.util.concurrent.atomic.AtomicLong;
import java.util.function.Consumer;
import java.util.regex.Pattern;
import java.util.stream.Collectors;

Expand All @@ -26,14 +26,19 @@

import io.quarkus.bootstrap.app.CuratedApplication;
import io.quarkus.bootstrap.app.QuarkusBootstrap;
import io.quarkus.bootstrap.app.QuarkusBootstrap.Mode;
import io.quarkus.bootstrap.classloading.ClassPathElement;
import io.quarkus.bootstrap.classloading.QuarkusClassLoader;
import io.quarkus.bootstrap.model.ApplicationModel;
import io.quarkus.deployment.dev.ClassScanResult;
import io.quarkus.deployment.dev.CompilationProvider;
import io.quarkus.deployment.dev.DevModeContext;
import io.quarkus.deployment.dev.DevModeContext.ModuleInfo;
import io.quarkus.deployment.dev.QuarkusCompiler;
import io.quarkus.deployment.dev.RuntimeUpdatesProcessor;
import io.quarkus.dev.spi.DevModeType;
import io.quarkus.dev.testing.TestWatchedFiles;
import io.quarkus.maven.dependency.ResolvedDependency;
import io.quarkus.paths.PathList;
import io.quarkus.runtime.configuration.HyphenateEnumConverter;

Expand Down Expand Up @@ -143,46 +148,36 @@ public void start() {
}
}

private static Pattern getCompiledPatternOrNull(Optional<String> patternStr) {
return patternStr.isPresent() ? Pattern.compile(patternStr.get()) : null;
Copy link
Member

Choose a reason for hiding this comment

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

Same same but different:

Suggested change
return patternStr.isPresent() ? Pattern.compile(patternStr.get()) : null;
return patternStr.map(Pattern::compile).orElse(null);

}

public void init() {
if (moduleRunners.isEmpty()) {
TestWatchedFiles.setWatchedFilesListener((s) -> RuntimeUpdatesProcessor.INSTANCE.setWatchedFilePaths(s, true));
final Pattern includeModulePattern = getCompiledPatternOrNull(config.includeModulePattern);
final Pattern excludeModulePattern = getCompiledPatternOrNull(config.excludeModulePattern);
for (var module : context.getAllModules()) {
boolean mainModule = module == context.getApplicationRoot();
final boolean mainModule = module == context.getApplicationRoot();
if (config.onlyTestApplicationModule && !mainModule) {
continue;
} else if (config.includeModulePattern.isPresent()) {
Pattern p = Pattern.compile(config.includeModulePattern.get());
if (!p.matcher(module.getArtifactKey().getGroupId() + ":" + module.getArtifactKey().getArtifactId())
} else if (includeModulePattern != null) {
if (!includeModulePattern
.matcher(module.getArtifactKey().getGroupId() + ":" + module.getArtifactKey().getArtifactId())
.matches()) {
continue;
}
} else if (config.excludeModulePattern.isPresent()) {
Pattern p = Pattern.compile(config.excludeModulePattern.get());
if (p.matcher(module.getArtifactKey().getGroupId() + ":" + module.getArtifactKey().getArtifactId())
} else if (excludeModulePattern != null) {
if (excludeModulePattern
.matcher(module.getArtifactKey().getGroupId() + ":" + module.getArtifactKey().getArtifactId())
.matches()) {
continue;
}
}

try {
Set<Path> paths = new LinkedHashSet<>();
module.getTest().ifPresent(test -> {
paths.add(Paths.get(test.getClassesPath()));
if (test.getResourcesOutputPath() != null) {
paths.add(Paths.get(test.getResourcesOutputPath()));
}
});
if (mainModule) {
curatedApplication.getQuarkusBootstrap().getApplicationRoot().forEach(paths::add);
} else {
paths.add(Paths.get(module.getMain().getClassesPath()));
}
for (var i : paths) {
if (!Files.exists(i)) {
Files.createDirectories(i);
}
}
QuarkusBootstrap.Builder builder = curatedApplication.getQuarkusBootstrap().clonedBuilder()
final Path projectDir = Path.of(module.getProjectDirectory());
final QuarkusBootstrap.Builder bootstrapConfig = curatedApplication.getQuarkusBootstrap().clonedBuilder()
.setMode(QuarkusBootstrap.Mode.TEST)
.setAssertionsEnabled(true)
.setDisableClasspathCache(false)
Expand All @@ -192,20 +187,62 @@ public void init() {
.setTest(true)
.setAuxiliaryApplication(true)
.setHostApplicationIsTestOnly(devModeType == DevModeType.TEST_ONLY)
.setProjectRoot(Paths.get(module.getProjectDirectory()))
.setApplicationRoot(PathList.from(paths))
.setProjectRoot(projectDir)
.setApplicationRoot(getRootPaths(module, mainModule))
.clearLocalArtifacts();

final QuarkusClassLoader ctParentFirstCl;
final Mode currentMode = curatedApplication.getQuarkusBootstrap().getMode();
// in case of quarkus:test the application model will already include test dependencies
if (Mode.CONTINUOUS_TEST != currentMode && Mode.TEST != currentMode) {
// In this case the current application model does not include test dependencies.
// 1) we resolve an application model for test mode;
// 2) we create a new CT base classloader that includes parent-first test scoped dependencies
// so that they are not loaded by augment and base runtime classloaders.
var appModelFactory = curatedApplication.getQuarkusBootstrap().newAppModelFactory();
appModelFactory.setBootstrapAppModelResolver(null);
appModelFactory.setTest(true);
appModelFactory.setLocalArtifacts(Set.of());
if (!mainModule) {
appModelFactory.setAppArtifact(null);
appModelFactory.setProjectRoot(projectDir);
}
final ApplicationModel testModel = appModelFactory.resolveAppModel().getApplicationModel();
bootstrapConfig.setExistingModel(testModel);

QuarkusClassLoader.Builder clBuilder = null;
Copy link
Member

Choose a reason for hiding this comment

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

This is not really proper 'parent first', they are still sitting in a ClassLoader that is not the JDK class loader.

Generally parent first has been used to hack around bugs in code that expects it to be in the same CL as the JDK itself, so I am not sure how much this actually helps?

It seems like a more correct solution would be to always put parent first test scoped deps onto the class path, but then 'hide' them from the base runtime and augment CLs. Technically other parent first classes could still see them, but I think that is unlikely to cause problems.

Copy link
Member Author

Choose a reason for hiding this comment

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

It does help the pact extension case. As to the always adding test scoped parent-first dependencies, that means always resolving test scope dependencies, which we don't want to do, since it will influence version convergence with the prod dependency tree.
To do it properly, we need to completely isolate the classpath of test apps when launching in dev mode.

Copy link
Member

Choose a reason for hiding this comment

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

Do you have an example of test scoped deps affecting non-test convergence? This seems like a problem as it would mean that tests may not be running against the same version as the actual application?

Either way I think this is fine for now, in general we should be trying to have as few parent first things as possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are instructions to reproduce the issue in Holly's issue report, in case you are curious. But, I agree, this is not a complete fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have a specific example but chances are this is happening more often than people realize. The same can be said about build time dependencies, such as provided and optional.

var currentParentFirst = curatedApplication.getApplicationModel().getParentFirst();
for (ResolvedDependency d : testModel.getDependencies()) {
if (d.isClassLoaderParentFirst() && !currentParentFirst.contains(d.getKey())) {
if (clBuilder == null) {
clBuilder = QuarkusClassLoader.builder("Continuous Testing Parent-First",
getClass().getClassLoader().getParent(), false);
}
clBuilder.addElement(ClassPathElement.fromDependency(d));
}
}

ctParentFirstCl = clBuilder == null ? null : clBuilder.build();
if (ctParentFirstCl != null) {
bootstrapConfig.setBaseClassLoader(ctParentFirstCl);
}
} else {
ctParentFirstCl = null;
if (mainModule) {
// the model and the app classloader already include test scoped dependencies
bootstrapConfig.setExistingModel(curatedApplication.getApplicationModel());
}
}

//we always want to propagate parent first
//so it is consistent. Some modules may not have quarkus dependencies
//so they won't load junit parent first without this
for (var i : curatedApplication.getApplicationModel().getDependencies()) {
if (i.isClassLoaderParentFirst()) {
builder.addParentFirstArtifact(i.getKey());
bootstrapConfig.addParentFirstArtifact(i.getKey());
}
}
var testCuratedApplication = builder // we want to re-discover the local dependencies with test scope
.build()
.bootstrap();
var testCuratedApplication = bootstrapConfig.build().bootstrap();
if (mainModule) {
//horrible hack
//we really need a compiler per module but we are not setup for this yet
Expand All @@ -215,7 +252,7 @@ public void init() {
//has complained much
compiler = new QuarkusCompiler(testCuratedApplication, compilationProviders, context);
}
var testRunner = new ModuleTestRunner(this, context, testCuratedApplication, module);
var testRunner = new ModuleTestRunner(this, testCuratedApplication, module);
QuarkusClassLoader cl = (QuarkusClassLoader) getClass().getClassLoader();
cl.addCloseTask(new Runnable() {
@Override
Expand All @@ -224,6 +261,9 @@ public void run() {
close();
} finally {
testCuratedApplication.close();
if (ctParentFirstCl != null) {
ctParentFirstCl.close();
}
}
}
});
Expand All @@ -235,6 +275,37 @@ public void run() {
}
}

private PathList getRootPaths(ModuleInfo module, final boolean mainModule) {
final PathList.Builder pathBuilder = PathList.builder();
final Consumer<Path> paths = new Consumer<>() {
@Override
public void accept(Path t) {
if (!pathBuilder.contains(t)) {
if (!Files.exists(t)) {
try {
Files.createDirectories(t);
} catch (IOException e) {
throw new UncheckedIOException(e);
}
}
pathBuilder.add(t);
}
}
};
module.getTest().ifPresent(test -> {
paths.accept(Path.of(test.getClassesPath()));
if (test.getResourcesOutputPath() != null) {
paths.accept(Path.of(test.getResourcesOutputPath()));
}
});
if (mainModule) {
curatedApplication.getQuarkusBootstrap().getApplicationRoot().forEach(paths::accept);
} else {
paths.accept(Path.of(module.getMain().getClassesPath()));
}
return pathBuilder.build();
}

public synchronized void close() {
closed = true;
stop();
Expand Down Expand Up @@ -522,10 +593,6 @@ public boolean isStarted() {
return started;
}

public CuratedApplication getCuratedApplication() {
return curatedApplication;
}

public QuarkusCompiler getCompiler() {
return compiler;
}
Expand Down
28 changes: 20 additions & 8 deletions ...-projects/bootstrap/core/src/main/java/io/quarkus/bootstrap/BootstrapAppModelFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,10 @@
import java.nio.file.Paths;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Set;

import org.apache.maven.model.Dependency;
import org.jboss.logging.Logger;

import io.quarkus.bootstrap.app.CurationResult;
Expand Down Expand Up @@ -74,9 +72,9 @@ public static BootstrapAppModelFactory newInstance() {
private MavenArtifactResolver mavenArtifactResolver;

private BootstrapMavenContext mvnContext;
Set<ArtifactKey> reloadableModules = Collections.emptySet();
Set<ArtifactKey> reloadableModules = Set.of();

private Collection<io.quarkus.maven.dependency.Dependency> forcedDependencies = Collections.emptyList();
private Collection<io.quarkus.maven.dependency.Dependency> forcedDependencies = List.of();

private BootstrapAppModelFactory() {
}
Expand Down Expand Up @@ -306,9 +304,23 @@ private boolean isWorkspaceDiscoveryEnabled() {
}

private LocalProject loadWorkspace() throws AppModelResolverException {
return projectRoot == null || !Files.isDirectory(projectRoot)
? null
: createBootstrapMavenContext().getCurrentProject();
if (projectRoot == null || !Files.isDirectory(projectRoot)) {
return null;
}
LocalProject project = createBootstrapMavenContext().getCurrentProject();
if (project == null) {
return null;
}
if (project.getDir().equals(projectRoot)) {
return project;
}
for (LocalProject p : project.getWorkspace().getProjects().values()) {
if (p.getDir().equals(projectRoot)) {
return p;
}
}
log.warnf("Expected project directory %s does not match current project directory %s", projectRoot, project.getDir());
return project;
}

private CurationResult createAppModelForJar(Path appArtifactPath) {
Expand All @@ -321,7 +333,7 @@ private CurationResult createAppModelForJar(Path appArtifactPath) {
}
modelResolver.relink(appArtifact, appArtifactPath);
//we need some way to figure out dependencies here
appModel = modelResolver.resolveManagedModel(appArtifact, Collections.emptyList(), managingProject,
appModel = modelResolver.resolveManagedModel(appArtifact, List.of(), managingProject,
reloadableModules);
} catch (AppModelResolverException | IOException e) {
throw new RuntimeException("Failed to resolve initial application dependencies", e);
Expand Down
Loading