Add pluginClasspaths to the configuration #23

rachelcarmena · 2019-10-03T15:55:42Z

Please, this pull request adds the ability to provide pluginClasspaths for the compilation.

Thanks in advance!

tschuchortdev

Thanks for this PR. I thought it was already possible to add compiler plugins via the pluginClasspath, but I guess I forgot about that.

This PR overlaps a lot with #15 which @Foso is working on. So it might make sense to start from what he already did, or rather combine it.

In particular we need to make sure that the compiler plugins are run in the correct phases.

tschuchortdev · 2019-10-03T16:35:10Z

src/main/kotlin/com/tschuchort/compiletesting/KotlinCompilation.kt

+		pluginClasspaths.forEach { filepath ->
+			if (!filepath.exists()) {
+				error("Plugin $filepath not found")
+				return ExitCode.INTERNAL_ERROR
+			}
+		}
+


I think this should probably be moved to the calling function, where writing of the source files happens as well.

Please, which function? stubsAndApt is called from compile:

// step 1 and 2: generate stubs and run annotation processors try { val exitCode = stubsAndApt(sourceFiles) if (exitCode != ExitCode.OK) { val messages = internalMessageBuffer.readUtf8() searchSystemOutForKnownErrors(messages) return Result(exitCode, classesDir, messages) } } finally { KaptComponentRegistrar.threadLocalParameters.remove() }

And previously, the writing of the source files appears:

// write given sources to working directory val sourceFiles = sources.map { it.writeIfNeeded(sourcesDir) }

Though I don't find an error checking section like the one I found in stubsAndApt.

I think I would just put it after val sourceFiles = sources.map { it.writeIfNeeded(sourcesDir) } in the compile function where all the initialization of directories is done. But you are right that it's kinda confusing where to put this stuff. There should be a clear separation between initialization and the different compile stages. Maybe I will rewrite it in the future.

tschuchortdev · 2019-10-03T16:38:15Z

src/main/kotlin/com/tschuchort/compiletesting/KotlinCompilation.kt

 		it.destination = classesDir.absolutePath
 		it.classpath = commonClasspaths().joinToString(separator = File.pathSeparator)

+		it.pluginClasspaths = pluginClasspaths.map(File::getAbsolutePath).toTypedArray()


The commonK2JVMArgs will be used in both steps: stubsAndApt and compileKotlin. So given compiler plugins will be run both times. Is this the desired behaviour? Unless the kapt plugin prevents the compiler from continuing with other plugins, like it short circuits the regular compilation. @Foso may know more.

Oh, I didn't have that problem because I didn't provide annotationProcessors, so stubsAndApt finished in the first conditional. Let's see the opinion by @Foso.

This issue still deserves some attention. Are we really sure that compiler plugins should be executed in both steps?

src/main/kotlin/com/tschuchort/compiletesting/KotlinCompilation.kt

src/test/kotlin/com/tschuchort/compiletesting/KotlinCompilationTests.kt

rachelcarmena · 2019-10-03T17:56:12Z

Thanks @tschuchortdev for the review!! I really appreciate it!
Do you know where I can find which @Foso is working on?

rachelcarmena · 2019-10-03T18:34:59Z

I pushed a new commit with 3 improvements from your comments.
Thanks again for your attention, the review and such an awesome plugin!! Congrats for it!!

rachelcarmena · 2019-10-03T19:43:23Z

Hi again @tschuchortdev, I found the development by @Foso and I think it's not necessary to combine both developments: see loadPluginsSafe (without componentRegistrars) and loadPlugins. Also I think his development is ad-hoc to his context. That being said, maybe I'm wrong!

tschuchortdev

I don't really see how PluginCLIParser is relevant here? It looks like loadPluginsSafe is just a wrapper around loadPlugins that catches exceptions, or am I missing something? Thanks for linking it though, might come in handy.

By "combining both developments" I don't mean necessarily on the same branch. But for example you both need to know during which stages the compiler plugins should be run, so it would be best to be on the same page here so you don't need to do the debugging twice. Similarly, you will both need to define some kind of test compiler plugin in order to test if it works; That could be shared as well.

Regarding the test: We can define a simple TestComponentRegistrar in the project that can be used in both cases. @Foso can just pass it by value and you will have to pass this over the classpath. One (complicated) way to do this is as follows: Add a inheritPluginClasspath option to inherit the host applications classpath as pluginClasspath similarly to the already present inheritClassPath option. Alternatively you can get the path of just the host project classes with the getResourcesPath() function. Then all classes of the host application will be on the pluginClasspath, but that is not enough: For ServiceLoaderLite to find them we also need to provide the service registrations in the corresponding META-INF file. This can be done by manually creating another jar containing just the META-INF file and adding it to the pluginClasspath (see example here). Perhaps there is an easier way to do this by just defining the TestComponentRegistrar in another Gradle module and then declaring it as some kind of jar dependency. But I don't know enough Gradle to do this.

tschuchortdev · 2019-10-03T20:11:44Z

src/main/kotlin/com/tschuchort/compiletesting/KotlinCompilation.kt

+		pluginClasspaths.forEach { filepath ->
+			if (!filepath.exists()) {
+				error("Plugin $filepath not found")
+				return ExitCode.INTERNAL_ERROR
+			}
+		}
+


I think I would just put it after val sourceFiles = sources.map { it.writeIfNeeded(sourcesDir) } in the compile function where all the initialization of directories is done. But you are right that it's kinda confusing where to put this stuff. There should be a clear separation between initialization and the different compile stages. Maybe I will rewrite it in the future.

src/main/kotlin/com/tschuchort/compiletesting/KotlinCompilation.kt

src/test/kotlin/com/tschuchort/compiletesting/KotlinCompilationTests.kt

rachelcarmena · 2019-10-04T09:23:04Z

I'll follow your ideas to create another test. Thanks!!

rachelcarmena · 2019-10-06T15:46:24Z

Hi @tschuchortdev,

Done. There is a new test to check that the new property pluginClasspaths works properly and the provided plugin is detected for the compilation. I'm not absolutely happy with the test because it's not shown how the plugin works. However, I think it's enough to check that the new property is working. Just adding a plugin in test runtime and getting its classpath for pluginClasspaths, shows a message about the provided plugin.

Please, let me know if I must do something more.

tschuchortdev

Aside from the missing documentation for classpathOf this looks fine to me. BUT we still don't know if given compiler plugins should be executed in both the stubsAndApt and compileKotlin steps! So I'm reluctant to merge this until we can be sure that it's correct... Or we could merge it now and fix it later. Naturally, I would prefer to get it right the first time.

tschuchortdev · 2019-10-06T16:34:46Z

src/test/kotlin/com/tschuchort/compiletesting/KotlinCompilationTests.kt

+
+	private fun classpathOf(dependency: String): File {
+		val regex = Regex(".*$dependency\\.jar")
+		val classGraph = ClassGraph().whitelistJars()


Why are you using whitelistJars() here? If I read the documentation correctly, this method should return an empty ClassGraph if no arguments are given.

Oh, sorry, fixed!

src/test/kotlin/com/tschuchort/compiletesting/KotlinCompilationTests.kt

tschuchortdev · 2019-10-06T16:37:55Z

src/main/kotlin/com/tschuchort/compiletesting/KotlinCompilation.kt

 		it.destination = classesDir.absolutePath
 		it.classpath = commonClasspaths().joinToString(separator = File.pathSeparator)

+		it.pluginClasspaths = pluginClasspaths.map(File::getAbsolutePath).toTypedArray()


This issue still deserves some attention. Are we really sure that compiler plugins should be executed in both steps?

Foso · 2019-10-06T17:35:15Z

Hi @tschuchortdev @rachelcarmena ,
i took a quick look at the PR and hope i can kind of help.

In my branch i used a list of ComponentRegistrars because i wanted to be able to directly interact with my plugin classes in the tests, without compiling the plugin and figuring out the classpath first.

I think it's independent from this PR. I can think of usecases where you want add a componentRegistrar and check it in combination with other plugins, that you add to the classpath.

Unfortunately i'm no expert when it comes to compiler phases, but the arrow-kt/arrow-meta-prototype@f7c4ea2 contains a few informations. I experienced that the attached ComponentRegistrar gets called when the compiler starts and then the compiler calls the attached Extension-classes, like AbstractKapt3Extension, when the compiler is in the right phase for it. So i'm not sure if we could add the plugins to wrong phase, because the compiler decides when they get called.

Btw:
Maybe the test checks configuration with pluginClasspaths should be renamed to something more specific (what exactly is the configuration/pluginClasspaths checked for?)

tschuchortdev · 2019-10-06T17:47:49Z

I think it's independent from this PR. I can think of usecases where you want add a componentRegistrar and check it in combination with other plugins, that you add to the classpath.

We should definitely have both, but I thought that maybe the test plugin could be shared.

So i'm not sure if we could add the plugins to wrong phase, because the compiler decides when they get called.

I think theses phases are actually "outside" the compiler and get orchestrated by the Kotlin Gradle and Kapt Gradle plugin. So Gradle decides what phases should receive which compiler plugins as arguments. But it's also possible that both phases receive all third-party compiler plugins and kapt actually prevents the registration of further compiler plugins by terminating the compiler before it gets there. Or the third option: Compiler plugins really have to be executed in both phases.

rachelcarmena · 2019-10-06T18:39:34Z

Thanks @tschuchortdev and @Foso ! I appreciate a lot your reviews.

Btw:
Maybe the test checks configuration with pluginClasspaths should be renamed to something more specific (what exactly is the configuration/pluginClasspaths checked for?)

Absolutely, it wasn't a good name. It exists in the first commit and it was replaced by returns an internal error when adding a non existing plugin for compilation in the second commit.

I added a warning message in the last commit in case the plugin is executed twice. However, as far as I know, annotation processors and compiler plugins are different alternatives to add code at compile-time and stubsAndApt has the following conditional at the beginning:

if(annotationProcessors.isEmpty()) {
	log("No services were given. Not running kapt steps.")
	return ExitCode.OK
}

so if a user provides pluginClasspaths instead of annotationProcessors, the plugin will be executed once for sure.

Could a user be interested in adding annotation processors and compiler plugins at the same time? In that case, will there be problems with two executions? I don't know. Maybe we could postpone that development. Now, the user receives a warning in case the plugin is executed twice (though the user will also see two traces).

Please, let me know if I must improve something else.

Thanks again!

tschuchortdev · 2019-10-06T19:05:20Z

Could a user be interested in adding annotation processors and compiler plugins at the same time?

Definitely. For example dagger + arrow. Jetpack Compose also uses compiler plugins iirc.

In that case, will there be problems with two executions?

It's possible. If the compiler plugin is used to generate code, it might try to generate the same class twice which will lead to errors. But similarly if we don't execute compiler plugins during the stubsAndApt phase, there may be the problem that Kotlin sources later generated by a compiler plugin are not subject to annotation processing.

Maybe we could postpone that development.

I have asked on the Kotlin slack about this (we could also summon someone from the compiler team here). If I don't receive an answer until tomorrow, I will probably merge this as it is and we can fix it later if someone complains. I assume you are also waiting for this to be available as a release so you can use it to test arrow-meta?

rachelcarmena · 2019-10-06T19:09:52Z

Thank you very much @tschuchortdev ! Yes, that's the reason why I'm interested in this change. Thanks again Thilo!

tschuchortdev · 2019-10-07T20:23:13Z

@rachelcarmena @raulraja Do you need a proper release or will a SNAPSHOT release suffice? I'd like to wait with the next real release until #15 is done because releasing on sonatype nexus is a real pain.

raulraja · 2019-10-07T22:54:14Z

@tschuchortdev we are fine depending on the snapshot until the release it's ready. Thanks!

Add pluginClasspaths to the configuration

7379cd5

tschuchortdev requested changes Oct 3, 2019

View reviewed changes

Requested changes after @tschuchortdev review

0def979

tschuchortdev requested changes Oct 3, 2019

View reviewed changes

Requested changes after @tschuchortdev review

c85aa65

rachelcarmena force-pushed the pluginClasspaths branch from 3af25f2 to 2b62824 Compare October 6, 2019 15:13

Requested changes after @tschuchortdev review

6707daa

rachelcarmena force-pushed the pluginClasspaths branch from 2b62824 to 6707daa Compare October 6, 2019 15:25

tschuchortdev reviewed Oct 6, 2019

View reviewed changes

Requested changes after @tschuchortdev review

6f37d94

tschuchortdev merged commit e2d6378 into tschuchortdev:master Oct 7, 2019

tschuchortdev mentioned this pull request Oct 21, 2019

#15 better support for compiler plugins #27

Closed

Add pluginClasspaths to the configuration #23

Add pluginClasspaths to the configuration #23

Uh oh!

Conversation

rachelcarmena commented Oct 3, 2019

Uh oh!

tschuchortdev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rachelcarmena commented Oct 3, 2019

Uh oh!

rachelcarmena commented Oct 3, 2019

Uh oh!

rachelcarmena commented Oct 3, 2019

Uh oh!

tschuchortdev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rachelcarmena commented Oct 4, 2019

Uh oh!

rachelcarmena commented Oct 6, 2019

Uh oh!

tschuchortdev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Foso commented Oct 6, 2019

Uh oh!

tschuchortdev commented Oct 6, 2019

Uh oh!

rachelcarmena commented Oct 6, 2019

Uh oh!

tschuchortdev commented Oct 6, 2019

Uh oh!

rachelcarmena commented Oct 6, 2019

Uh oh!

tschuchortdev commented Oct 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raulraja commented Oct 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tschuchortdev commented Oct 7, 2019 •

edited

Loading