-
Notifications
You must be signed in to change notification settings - Fork 225
Native Image Friendly #231
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
Changes from 18 commits
1e2efe0
0ffe8ef
6f6260e
decfd9c
ede8325
8aa10d1
9380215
62fba5c
41a10a2
14167cf
4103622
795c656
2697810
7075ddf
ef3dd1a
d969187
bf2cb03
7eaad18
e02804f
be8468d
b207ba9
49272c7
3121fce
bb89c00
0f110e1
9482857
e07d463
5446214
84a2b9c
58334a6
93f150a
99a541d
e5909fa
5a0af91
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,20 +16,57 @@ | |
<packaging>jar</packaging> | ||
|
||
<properties> | ||
<java.version>8</java.version> | ||
<maven.compiler.source>1.8</maven.compiler.source> | ||
<maven.compiler.target>1.8</maven.compiler.target> | ||
</properties> | ||
|
||
<build> | ||
<plugins> | ||
<plugin> | ||
<groupId>org.apache.maven.plugins</groupId> | ||
<artifactId>maven-surefire-plugin</artifactId> | ||
<version>${surefire.version}</version> | ||
</plugin> | ||
</plugins> | ||
</build> | ||
<profiles> | ||
<profile> | ||
<id>java8</id> | ||
<activation> | ||
<jdk>1.8</jdk> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this still valid? I guess this needs to be java 11 |
||
</activation> | ||
<build> | ||
<plugins> | ||
<plugin> | ||
<groupId>org.apache.maven.plugins</groupId> | ||
<artifactId>maven-surefire-plugin</artifactId> | ||
<version>${surefire.version}</version> | ||
</plugin> | ||
<plugin> | ||
<groupId>org.apache.maven.plugins</groupId> | ||
<artifactId>maven-compiler-plugin</artifactId> | ||
<configuration> | ||
<compilerArguments> | ||
<bootclasspath> | ||
${java.home}/lib/rt.jar${path.separator}${java.home}/lib/jce.jar${path.separator}${java.home}/../lib/tools.jar | ||
</bootclasspath> | ||
</compilerArguments> | ||
</configuration> | ||
</plugin> | ||
</plugins> | ||
</build> | ||
</profile> | ||
<profile> | ||
<id>default</id> | ||
<activation> | ||
<activeByDefault>true</activeByDefault> | ||
</activation> | ||
<build> | ||
<plugins> | ||
<plugin> | ||
<groupId>org.apache.maven.plugins</groupId> | ||
<artifactId>maven-surefire-plugin</artifactId> | ||
<version>${surefire.version}</version> | ||
</plugin> | ||
</plugins> | ||
</build> | ||
</profile> | ||
</profiles> | ||
|
||
|
||
|
||
|
||
|
||
<dependencies> | ||
<dependency> | ||
|
@@ -82,5 +119,34 @@ | |
<artifactId>javassist</artifactId> | ||
<version>3.27.0-GA</version> | ||
</dependency> | ||
|
||
<dependency> | ||
<groupId>com.google.testing.compile</groupId> | ||
<artifactId>compile-testing</artifactId> | ||
<version>0.19</version> | ||
<scope>test</scope> | ||
</dependency> | ||
|
||
<dependency> | ||
<groupId>com.google.auto.service</groupId> | ||
<artifactId>auto-service</artifactId> | ||
<version>1.0-rc2</version> | ||
<scope>compile</scope> | ||
</dependency> | ||
|
||
<dependency> | ||
<groupId>io.quarkus</groupId> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove quarkus dependency, we'll add it in dedicated module |
||
<artifactId>quarkus-core</artifactId> | ||
<version>1.9.2.Final</version> | ||
</dependency> | ||
|
||
<dependency> | ||
<groupId>com.squareup</groupId> | ||
<artifactId>javapoet</artifactId> | ||
<version>1.13.0</version> | ||
<scope>compile</scope> | ||
</dependency> | ||
|
||
|
||
</dependencies> | ||
</project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,21 +1,18 @@ | ||
package io.javaoperatorsdk.operator; | ||
|
||
import io.javaoperatorsdk.operator.api.Controller; | ||
import io.javaoperatorsdk.operator.api.ResourceController; | ||
import io.fabric8.kubernetes.api.builder.Function; | ||
import io.fabric8.kubernetes.client.CustomResource; | ||
import io.fabric8.kubernetes.client.CustomResourceDoneable; | ||
import javassist.*; | ||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
import io.javaoperatorsdk.operator.api.Controller; | ||
import io.javaoperatorsdk.operator.api.ResourceController; | ||
|
||
import java.lang.reflect.ParameterizedType; | ||
import java.util.Arrays; | ||
import java.util.HashMap; | ||
import java.util.Map; | ||
|
||
|
||
public class ControllerUtils { | ||
|
||
private final static double JAVA_VERSION = Double.parseDouble(System.getProperty("java.specification.version")); | ||
private static final String FINALIZER_NAME_SUFFIX = "/finalizer"; | ||
|
||
// this is just to support testing, this way we don't try to create class multiple times in memory with same name. | ||
|
@@ -37,7 +34,14 @@ static boolean getGenerationEventProcessing(ResourceController controller) { | |
} | ||
|
||
static <R extends CustomResource> Class<R> getCustomResourceClass(ResourceController<R> controller) { | ||
return (Class<R>) getAnnotation(controller).customResourceClass(); | ||
final Class<R> type = Arrays | ||
.stream(controller.getClass().getGenericInterfaces()) | ||
.filter(i -> i instanceof ParameterizedType) | ||
.map(i -> (ParameterizedType) i) | ||
.findFirst() | ||
.map(i -> (Class<R>) i.getActualTypeArguments()[0]) | ||
.get(); | ||
return type; | ||
} | ||
|
||
static String getCrdName(ResourceController controller) { | ||
|
@@ -48,34 +52,11 @@ static String getCrdName(ResourceController controller) { | |
public static <T extends CustomResource> Class<? extends CustomResourceDoneable<T>> | ||
getCustomResourceDoneableClass(ResourceController<T> controller) { | ||
try { | ||
Class<? extends CustomResource> customResourceClass = getAnnotation(controller).customResourceClass(); | ||
String className = customResourceClass.getPackage().getName() + "." + customResourceClass.getSimpleName() + "CustomResourceDoneable"; | ||
|
||
if (doneableClassCache.containsKey(customResourceClass)) { | ||
csviri marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return (Class<? extends CustomResourceDoneable<T>>) doneableClassCache.get(customResourceClass); | ||
} | ||
|
||
ClassPool pool = ClassPool.getDefault(); | ||
pool.appendClassPath(new LoaderClassPath(Thread.currentThread().getContextClassLoader())); | ||
|
||
CtClass superClass = pool.get(CustomResourceDoneable.class.getName()); | ||
CtClass function = pool.get(Function.class.getName()); | ||
CtClass customResource = pool.get(customResourceClass.getName()); | ||
CtClass[] argTypes = {customResource, function}; | ||
CtClass customDoneable = pool.makeClass(className, superClass); | ||
CtConstructor ctConstructor = CtNewConstructor.make(argTypes, null, "super($1, $2);", customDoneable); | ||
customDoneable.addConstructor(ctConstructor); | ||
|
||
Class<? extends CustomResourceDoneable<T>> doneableClass; | ||
if (JAVA_VERSION >= 9) { | ||
doneableClass = (Class<? extends CustomResourceDoneable<T>>) customDoneable.toClass(customResourceClass); | ||
} else { | ||
doneableClass = (Class<? extends CustomResourceDoneable<T>>) customDoneable.toClass(); | ||
} | ||
doneableClassCache.put(customResourceClass, doneableClass); | ||
return doneableClass; | ||
} catch (CannotCompileException | NotFoundException e) { | ||
throw new IllegalStateException(e); | ||
final Class<T> customResourceClass = getCustomResourceClass(controller); | ||
return (Class<? extends CustomResourceDoneable<T>>) Class.forName(customResourceClass.getCanonicalName() + "Doneable"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will this work if the CR class is an inner class? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes it does work, haven't tested that with native-images yet though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, I'll add this test There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After while, I believe that we should test it here, not in example. We would like to introduce native build as a main feature of the SDK. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we could add native build as a test step in next PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
True, would be great to have it as one of the examples in this repository, will be easier to find for the users and also can be used as regression tests for future changes in the framework. |
||
} catch (ClassNotFoundException e) { | ||
e.printStackTrace(); | ||
return null; | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,21 +1,20 @@ | ||
package io.javaoperatorsdk.operator.api; | ||
|
||
import io.fabric8.kubernetes.client.CustomResource; | ||
import io.quarkus.runtime.annotations.RegisterForReflection; | ||
|
||
import java.lang.annotation.ElementType; | ||
import java.lang.annotation.Retention; | ||
import java.lang.annotation.RetentionPolicy; | ||
import java.lang.annotation.Target; | ||
|
||
@Retention(RetentionPolicy.RUNTIME) | ||
@RegisterForReflection | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this the only reason we have quarkus on classpath? Would be cleaner if we could do it without it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So if somebody wan't to use quarkus, it can be added to the project. But if not its an unnecessary dependency on classpath. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, good point. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, I'm planning on doing that with #228. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So my suggestion is to:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That said, I'm not sure that native compilation will work properly without using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kirek007 another aspect is also that #228 tries to extract the configuration parts from the Spring Boot starter so that it could be reused in a Quarkus extension instead of duplicating the code so I really think that it should get done sooner rather than later. That said, I'm not quite satisfied with how it currently is, in terms of configuration I mean. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can remove the Quarkus dependency from here, the other way to achieve the same is generating a ReflectionConfigurationFile in compile time (just like what Quarkus is doing with the annotations). Can be done in the new module that will be added. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would be great @psycho-ir There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is related to quarkus dependency and should be removed |
||
@Target({ElementType.TYPE}) | ||
public @interface Controller { | ||
String NULL = ""; | ||
|
||
String crdName(); | ||
|
||
Class<? extends CustomResource> customResourceClass(); | ||
|
||
/** | ||
* Optional finalizer name, if it is not, | ||
* the crdName will be used as the name of the finalizer too. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,102 @@ | ||
package io.javaoperatorsdk.operator.processing; | ||
|
||
import com.google.auto.service.AutoService; | ||
import com.squareup.javapoet.*; | ||
import com.sun.tools.javac.code.Symbol; | ||
import com.sun.tools.javac.code.Type; | ||
import io.fabric8.kubernetes.api.builder.Function; | ||
import io.fabric8.kubernetes.client.CustomResourceDoneable; | ||
import io.javaoperatorsdk.operator.api.ResourceController; | ||
import io.quarkus.runtime.annotations.RegisterForReflection; | ||
|
||
import javax.annotation.processing.*; | ||
import javax.lang.model.SourceVersion; | ||
import javax.lang.model.element.Element; | ||
import javax.lang.model.element.Modifier; | ||
import javax.lang.model.element.TypeElement; | ||
import javax.lang.model.type.TypeMirror; | ||
import javax.tools.JavaFileObject; | ||
import java.io.IOException; | ||
import java.io.PrintWriter; | ||
import java.util.ArrayList; | ||
import java.util.List; | ||
import java.util.Set; | ||
|
||
@SupportedAnnotationTypes( | ||
"io.javaoperatorsdk.operator.api.Controller") | ||
@SupportedSourceVersion(SourceVersion.RELEASE_8) | ||
@AutoService(Processor.class) | ||
public class ControllerAnnotationProcessor extends AbstractProcessor { | ||
@Override | ||
public boolean process(Set<? extends TypeElement> annotations, RoundEnvironment roundEnv) { | ||
for (TypeElement annotation : annotations) { | ||
Set<? extends Element> annotatedElements | ||
= roundEnv.getElementsAnnotatedWith(annotation); | ||
annotatedElements.stream().filter(element -> element instanceof Symbol.ClassSymbol) | ||
.map(e -> (Symbol.ClassSymbol) e) | ||
.forEach(this::generateDoneableClass); | ||
} | ||
return false; | ||
} | ||
|
||
private void generateDoneableClass(Symbol.ClassSymbol controllerClassSymbol) { | ||
try { | ||
final TypeMirror resourceType = findResourceType(controllerClassSymbol); | ||
Symbol.ClassSymbol customerResourceSymbol = (Symbol.ClassSymbol) processingEnv | ||
.getElementUtils() | ||
.getTypeElement(resourceType.toString()); | ||
|
||
JavaFileObject builderFile = processingEnv.getFiler() | ||
.createSourceFile(customerResourceSymbol.className() + "Doneable"); | ||
|
||
try (PrintWriter out = new PrintWriter(builderFile.openWriter())) { | ||
final MethodSpec constructor = MethodSpec.constructorBuilder() | ||
.addModifiers(Modifier.PUBLIC) | ||
.addParameter(TypeName.get(resourceType), "resource") | ||
.addParameter(Function.class, "function") | ||
.addStatement("super(resource,function)") | ||
.build(); | ||
|
||
final TypeSpec typeSpec = TypeSpec.classBuilder(customerResourceSymbol.name + "Doneable") | ||
.addAnnotation(RegisterForReflection.class) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This annotation won't compile after removing quarkus-core |
||
.superclass(ParameterizedTypeName.get(ClassName.get(CustomResourceDoneable.class), TypeName.get(resourceType))) | ||
.addModifiers(Modifier.PUBLIC) | ||
.addMethod(constructor) | ||
.build(); | ||
|
||
JavaFile file = JavaFile.builder(customerResourceSymbol.packge().fullname.toString(), typeSpec) | ||
.build(); | ||
file.writeTo(out); | ||
} | ||
} catch (IOException ioException) { | ||
ioException.printStackTrace(); | ||
} catch (Exception ex) { | ||
ex.printStackTrace(); | ||
} | ||
} | ||
|
||
private TypeMirror findResourceType(Symbol.ClassSymbol controllerClassSymbol) throws Exception { | ||
final Type controllerType = collectAllInterfaces(controllerClassSymbol) | ||
.stream() | ||
.filter(i -> i.toString() | ||
.startsWith(ResourceController.class.getCanonicalName()) | ||
) | ||
.findFirst() | ||
.orElseThrow(() -> new Exception("ResourceController is not implemented by " + controllerClassSymbol.toString())); | ||
|
||
final TypeMirror resourceType = controllerType.getTypeArguments().get(0); | ||
return resourceType; | ||
} | ||
|
||
private List<Type> collectAllInterfaces(Symbol.ClassSymbol classSymbol) { | ||
List<Type> interfaces = new ArrayList<>(classSymbol.getInterfaces()); | ||
Symbol.ClassSymbol superclass = (Symbol.ClassSymbol) processingEnv.getTypeUtils().asElement(classSymbol.getSuperclass()); | ||
|
||
while (superclass != null) { | ||
interfaces.addAll(superclass.getInterfaces()); | ||
superclass = (Symbol.ClassSymbol) processingEnv.getTypeUtils().asElement(superclass.getSuperclass()); | ||
} | ||
|
||
return interfaces; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
package io.javaoperatorsdk.operator.processing; | ||
|
||
import com.google.testing.compile.*; | ||
import com.google.testing.compile.Compiler; | ||
import org.junit.jupiter.api.Test; | ||
|
||
import javax.tools.JavaFileObject; | ||
|
||
class ControllerAnnotationProcessorTest { | ||
@Test | ||
public void generateCorrectDoneableClassIfInterfaceIsSecond() { | ||
Compilation compilation = Compiler.javac() | ||
.withProcessors(new ControllerAnnotationProcessor()) | ||
.compile(JavaFileObjects.forResource("ControllerImplemented2Interfaces.java")); | ||
CompilationSubject.assertThat(compilation).succeeded(); | ||
|
||
final JavaFileObject expectedResource = JavaFileObjects.forResource("ControllerImplemented2InterfacesExpected.java"); | ||
JavaFileObjectSubject.assertThat(compilation.generatedSourceFiles().get(0)).hasSourceEquivalentTo(expectedResource); | ||
} | ||
|
||
@Test | ||
public void generateCorrectDoneableClassIfThereIsAbstractBaseController() { | ||
|
||
Compilation compilation = Compiler.javac() | ||
.withProcessors(new ControllerAnnotationProcessor()) | ||
.compile( | ||
JavaFileObjects.forResource("AbstractController.java"), | ||
JavaFileObjects.forResource("ControllerImplementedIntermediateAbstractClass.java") | ||
); | ||
CompilationSubject.assertThat(compilation).succeeded(); | ||
|
||
final JavaFileObject expectedResource = JavaFileObjects.forResource("ControllerImplementedIntermediateAbstractClassExpected.java"); | ||
JavaFileObjectSubject.assertThat(compilation.generatedSourceFiles().get(0)).hasSourceEquivalentTo(expectedResource); | ||
|
||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't that be removed now that we only target Java 11+?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@psycho-ir, this should be removed, because we support only 11+ (it's already in master)