Handle scaladoc for Scala3 #18248

BarkingBad · 2021-09-09T11:53:38Z

Fixes #16527

Context

The scaladoc was not working for scala 3. I still need help with creating proper integration tests.

Contributor Checklist

Review Contribution Guidelines
Make sure that all commits are signed off to indicate that you agree to the terms of Developer Certificate of Origin.
Make sure all contributed code can be distributed under the terms of the Apache License 2.0, e.g. the code was written by yourself or the original code is licensed under a license compatible to Apache License 2.0.
Check "Allow edit from maintainers" option in pull request so that additional changes can be pushed by Gradle team
Provide integration tests (under <subproject>/src/integTest) to verify changes from a user perspective
Provide unit tests (under <subproject>/src/test) to verify logic
Update User Guide, DSL Reference, and Javadoc for public-facing changes
Ensure that tests pass sanity check: ./gradlew sanityCheck // warnings for other subprojects I haven't touched
Ensure that tests pass locally: ./gradlew <changed-subproject>:quickTest

Gradle Core Team Checklist

Verify design and implementation
Verify test coverage and CI build status
Verify documentation
Recognize contributor in release notes

BarkingBad · 2021-09-15T15:16:49Z

@ljacomet could you take a look? :D

tgodzik

Thanks for the fix! I left 2 comments, but otherwise looks good.

subprojects/scala/src/main/java/org/gradle/api/plugins/scala/ScalaPlugin.java

subprojects/scala/src/main/java/org/gradle/api/tasks/ScalaRuntime.java

ljacomet

Thanks a lot for working on this!

A couple questions / comments, see below:

subprojects/scala/src/main/java/org/gradle/api/tasks/scala/internal/GenerateScaladoc.java

ljacomet · 2021-09-17T09:14:28Z

subprojects/scala/src/main/java/org/gradle/api/tasks/scala/internal/GenerateScaladoc.java

        Object scaladoc = scaladocClass.getDeclaredConstructor().newInstance();
        process.invoke(scaladoc, new Object[]{args.toArray(new String[0])});
    }
+
+    private static String getScalaVersion(ClassLoader scalaClassLoader) {


I wonder if we could not inject that information in the worker instead of having to load something here. After all, we had to ask the same question earlier.

I think this is impossible since the plugin is universal (it's just scala) and we determine the version at runtime by the scala library dependency, but maybe you have some idea how to workaround it.

The path would be something like: ScalaBasePlugin -> ScalaDoc -> ScaladocParameters -> GenerateScaladoc

makes sense?

ljacomet · 2021-09-17T09:18:29Z

subprojects/scala/src/integTest/groovy/org/gradle/scala/scaladoc/ScalaDocIntegrationTest.groovy

@@ -83,4 +83,28 @@ class ScalaDocIntegrationTest extends AbstractIntegrationSpec implements Directo
        // Started Gradle worker daemon (0.399 secs) with fork options DaemonForkOptions{executable=/Library/Java/JavaVirtualMachines/adoptopenjdk-11.jdk/Contents/Home/bin/java, minHeapSize=null, maxHeapSize=234M, jvmArgs=[], keepAliveMode=DAEMON}.
        outputContains("maxHeapSize=234M")
    }
+
+    def "scaladoc uses scala3"() {


Is there anything specific to Scala 3 scaladoc that we could test for here?

I'll think about that, but I think the output is very alike to other doc tools. We can assert the existence of certain files I think.

BarkingBad · 2021-09-17T15:02:53Z

~~Do you know why all the checks have failed? Locally everything was ok... I don't have access to check teamcity logs though, so I don't know what is wrong~~

My bad, I pushed one of the fixes too fast

subprojects/scala/src/main/java/org/gradle/api/plugins/scala/ScalaPlugin.java

BarkingBad · 2021-09-20T09:39:13Z

Hi, I implemented the path you proposed for injecting whether it is scala 3 version.

BarkingBad · 2021-09-20T13:32:20Z

I forgot to run a sanity check previously so unused imports broke the CI, sorry. Now I checked ./gradlew sanityCheck and ./gradlew scala:quickTest, both work fine. Previously Java 16 check passed, but failed for: Quick Feedback, Quick Feedback Linux, and Java 8, do you know why?

BarkingBad · 2021-09-21T09:31:40Z

~~Could you bring some insight into why it fails for Java8 and Quick Feedback?~~

Oh, I didn't know that Login as Guest shows the logs and was trying to log with my tc account which was rejected as "Not authorized too see this project"

tgodzik · 2021-09-21T09:53:13Z

It looks like ScalaPluginTest.addsScalaDoc3TasksToTheProject, IdeaIntegrationTest.addsScalaFacetAndCompilerLibraries, IdeaIntegrationTest.addsScalaSdkAndCompilerLibraries and IdeaIntegrationTest.addsScalaSdkAndCompilerLibraries are failing.

BarkingBad · 2021-09-21T12:27:28Z

I fixed ScalaPluginTest (I think that there was a problem with creating two test directories, which in fact were the very same directory (they were based on getClass()), and on windows there were problems with cleaning caches (Linux was working ok).
I also fixed IdeaIntegrationTest, which made me think whether we should rethink how we add scaladoc to gradle, because in scala3 there are a lot of extra transitive dependencies from scaladoc added also to compiler classpath, which can eventually lead to some errors, but changing the whole architecture to keep two separate classpaths can be a tedious job.
Lastly, I don't know why CancellationIntegrationTest failed, I have no idea how my changes affected this test.

ljacomet · 2021-09-28T08:49:58Z

subprojects/scala/src/main/java/org/gradle/api/tasks/scala/ScalaDoc.java

+    @Incubating
+    @Optional
+    @Input
+    public Boolean getIsScala3() {


Thanks for looking into my suggestion to inject the Scala version down here.
However adding a public field for this feels wrong.

I had a look at the other changes and what could be done instead is reach out to ScalaBasePlugin.isScala3 method, which would need to be made public.

This would also remove the need for adding a convention mapping - a concept we are trying to move away from.

Actually, reviewing #18347, let's just add the ScalaRuntime as an @Internal field instead.
See https://github.com/gradle/gradle/pull/18347/files#diff-bd022006e2ee4c0e9471eff5abec3f44a6e40a84e760796df120df9818c9ca3cR51

Thanks for the tip, I just pushed the changes

ljacomet · 2021-09-28T08:50:18Z

subprojects/scala/src/main/java/org/gradle/api/tasks/scala/ScalaDoc.java

@@ -198,6 +210,7 @@ protected void generate() {
            parameters.getClasspath().from(getClasspath());
            parameters.getOutputDirectory().set(getDestinationDir());
            parameters.getSources().from(getSource());
+            parameters.getIsScala3().set(getIsScala3());


As indicated, reach out to the ScalaBasePlugin instead

ljacomet · 2021-09-28T08:50:38Z

subprojects/scala/src/main/java/org/gradle/api/plugins/scala/ScalaBasePlugin.java

@@ -279,6 +288,7 @@ private void configureScaladoc(final Project project, final ScalaRuntime scalaRu
            scalaDoc.getConventionMapping().map("destinationDir", (Callable<File>) () -> project.getExtensions().getByType(JavaPluginExtension.class).getDocsDir().dir("scaladoc").get().getAsFile());
            scalaDoc.getConventionMapping().map("title", (Callable<String>) () -> project.getExtensions().getByType(ReportingExtension.class).getApiDocTitle());
            scalaDoc.getConventionMapping().map("scalaClasspath", (Callable<FileCollection>) () -> scalaRuntime.inferScalaClasspath(scalaDoc.getClasspath()));
+            scalaDoc.getConventionMapping().map("isScala3", (Callable<Boolean>) () -> isScala3(scalaDoc.getScalaClasspath()));


As indicated, remove this as the information is obtained in another way

ljacomet · 2021-09-28T08:52:20Z

@BarkingBad Note that we are preparing 7.3 RC1 so if you rebase, make sure to rebase on the release branch.

BarkingBad · 2021-09-28T09:35:31Z

Ok, thanks, I'll try to do it today. One more question, do you know why this one test fails with leaking files (or how to use this tip org.gradle.integtests.fixtures.executer.GradleExecuter.withFileLeakDetection for Integration tests). I feel a little bit lost with that. As a workaround, I think moving the scala3 test to another file so a new separate directory will be created can fix the problem.

ljacomet · 2021-09-28T13:01:38Z

Let me relaunch a test suite and see

Issue gradle#16527

ljacomet · 2021-10-01T14:35:58Z

Well looks like I missed one way of fixing this which seems elegant enough and gets rid of the problem, thanks @big-guy

@BarkingBad Any chance you can confirm this works on a real project?

ljacomet · 2021-10-01T14:38:57Z

subprojects/scala/src/main/java/org/gradle/api/tasks/scala/ScalaDoc.java

-            parameters.getSources().from(getSource());
+            boolean isScala3 = ScalaRuntimeHelper.findScalaJar(getScalaClasspath(), "library_3") != null;
+            parameters.getIsScala3().set(isScala3);
+            if (isScala3) {


This effectively assumes that a custom ScalaDoc task on a custom source set would properly wire both the classpath and the sources and that we can simply here change what is to be considered "sources"

* Use test fixture for Maven Central * Fake scala jar to not have real dependency resolution Issue gradle#16527

bot-gradle · 2021-10-01T16:46:52Z

OK, I've already triggered a build for you.

bot-gradle · 2021-10-01T17:39:32Z

Pre-tested commit build failed.

ljacomet · 2021-10-01T20:13:48Z

@bot-gradle test and merge

bot-gradle · 2021-10-01T20:14:02Z

OK, I've already triggered a build for you.

BarkingBad · 2021-10-04T11:07:57Z

Sorry, I was absent this weekend, but I just have tested it on a small project and it is working fine :D

dubinsky · 2021-10-19T01:07:35Z

@ljacomet @BarkingBad is this supposed to be working in 7.3-rc-1?
I am getting scala.MatchError: val <none> (of class dotty.tools.dotc.core.Symbols$NoSymbol$) while compiling ...

BarkingBad · 2021-10-19T07:32:10Z

Hi, it should be working right now. This error indicates some problems with the compiler itself and can be the result of wrongly gathered classpath for scaladoc generation. Could you share the repo so I could take a look, or try to generate scaladoc from the executable (installed manually or via coursier). Also, what scala version are you using, and is it completing the generation? There used to be problems in older scaladoc versions because it was not stable yet, but all these MatchErrors should not break the generation.

dubinsky · 2021-10-19T19:59:56Z

@BarkingBad

Hi, it should be working right now.
This error indicates some problems with the compiler itself and can be the result
of wrongly gathered classpath for scaladoc generation.
what scala version are you using, and is it completing the generation?
There used to be problems in older scaladoc versions because it was not stable yet,
but all these MatchErrors should not break the generation.
Could you share the repo so I could take a look ... .

Thanks!!!

I am using Scala 3.1.1-RC1 (with Gradle 7.3-rc-1).
On a fresh clone of the repository and with no Gradle daemons from the previous build running, I no longer see scala.MatchError, but the build fails:

$ git clone https://github.com/opentorah/opentorah.git
...
$ cd opentorah
$ ./gradlew scaladoc
...
> Task :opentorah-calendar:scaladoc
-- Error: ... undefined ... at readTasty ... cannot take signature of MethodType(...) ... 
the classfile defining the type might be missing from the classpath while compiling ...
> Task :opentorah-calendar:scaladoc FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':opentorah-calendar:scaladoc'.
> A failure occurred while executing org.gradle.api.tasks.scala.internal.GenerateScaladoc
   > Could not generate scaladoc

BarkingBad · 2021-10-19T21:01:13Z

Hmm, I think I know what can cause the problem. I will experiment tomorrow. Thanks for the repo to test on :)

dubinsky · 2021-10-19T21:41:02Z

Hmm, I think I know what can cause the problem. I will experiment tomorrow. Thanks for the repo to test on :)

Thank you!

BarkingBad · 2021-10-20T19:26:18Z

I've identified 3 parts that don't work as they should. For one I have an easy fix, but the other two I have to investigate. I hope you are not in a great hurry, because I don't know if I can handle them asap. I'll get back to you when I will have some more info about the case.

dubinsky · 2021-10-20T19:39:19Z

I've identified 3 parts that don't work as they should. For one I have an easy fix, but the other two I have to investigate. I hope you are not in a great hurry, because I don't know if I can handle them asap. I'll get back to you when I will have some more info about the case.

Take your time and thank you for working on this!

ljacomet · 2021-10-21T07:08:57Z

@dubinsky Could you open an issue for this please?

BarkingBad · 2021-10-21T09:44:16Z

Actually one of the problems is related to a bug in my Gradle implementation and the second one is probably on the compiler side (it is reproducible without Gradle). I will elaborate on them once the issue will be created.

dubinsky · 2021-10-21T14:45:16Z

@ljacomet @BarkingBad issue: #18732

BarkingBad force-pushed the scaladoc-fixes branch from bc2448f to 0e570c6 Compare September 9, 2021 13:46

BarkingBad marked this pull request as ready for review September 14, 2021 08:10

BarkingBad force-pushed the scaladoc-fixes branch 3 times, most recently from 24bca5b to 1d9340c Compare September 14, 2021 11:24

tgodzik suggested changes Sep 16, 2021

View reviewed changes

subprojects/scala/src/main/java/org/gradle/api/plugins/scala/ScalaPlugin.java Outdated Show resolved Hide resolved

subprojects/scala/src/main/java/org/gradle/api/tasks/ScalaRuntime.java Outdated Show resolved Hide resolved

BarkingBad force-pushed the scaladoc-fixes branch from 022c987 to 41c8b61 Compare September 16, 2021 09:47

ljacomet requested changes Sep 17, 2021

View reviewed changes

ljacomet self-assigned this Sep 17, 2021

ljacomet added this to the 7.3 RC1 milestone Sep 17, 2021

ljacomet reviewed Sep 17, 2021

View reviewed changes

subprojects/scala/src/main/java/org/gradle/api/plugins/scala/ScalaPlugin.java Outdated Show resolved Hide resolved

BarkingBad force-pushed the scaladoc-fixes branch from 2fe7840 to 25f0eb3 Compare September 20, 2021 09:34

gradle deleted a comment from ljacomet Sep 20, 2021

big-guy added the @core Issue owned by GBT Core label Sep 20, 2021

BarkingBad force-pushed the scaladoc-fixes branch from cbccdba to 572f3e0 Compare September 21, 2021 12:19

big-guy changed the base branch from master to release September 27, 2021 17:12

gradle deleted a comment from ljacomet Sep 28, 2021

ljacomet requested changes Sep 28, 2021

View reviewed changes

BarkingBad force-pushed the scaladoc-fixes branch from 4138d72 to 2f4ee4d Compare September 28, 2021 12:37

gradle deleted a comment from ljacomet Sep 28, 2021

Scaladoc is supported for Scala 3

48266a4

Issue gradle#16527

ljacomet force-pushed the scaladoc-fixes branch from 655cb6c to 48266a4 Compare October 1, 2021 14:34

BarkingBad requested a review from pioterj as a code owner October 1, 2021 14:34

gradle deleted a comment from ljacomet Oct 1, 2021

ljacomet reviewed Oct 1, 2021

View reviewed changes

Fix tests

7901f01

* Use test fixture for Maven Central * Fake scala jar to not have real dependency resolution Issue gradle#16527

gradle deleted a comment from ljacomet Oct 1, 2021

bot-gradle merged commit 14f0a5a into gradle:release Oct 1, 2021

ljacomet modified the milestones: 7.4 RC1, 7.3 RC1 Oct 11, 2021

BarkingBad mentioned this pull request Oct 21, 2021

Fix scaladoc sources gathering for Scala3 #18730

Closed

13 tasks

Handle scaladoc for Scala3 #18248

Handle scaladoc for Scala3 #18248

Uh oh!

Conversation

BarkingBad commented Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Contributor Checklist

Gradle Core Team Checklist

Uh oh!

BarkingBad commented Sep 15, 2021

Uh oh!

tgodzik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ljacomet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

BarkingBad commented Sep 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

BarkingBad commented Sep 20, 2021

Uh oh!

BarkingBad commented Sep 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BarkingBad commented Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgodzik commented Sep 21, 2021

Uh oh!

BarkingBad commented Sep 21, 2021

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!

Choose a reason for hiding this comment

Uh oh!

ljacomet commented Sep 28, 2021

Uh oh!

BarkingBad commented Sep 28, 2021

Uh oh!

ljacomet commented Sep 28, 2021

Uh oh!

ljacomet commented Oct 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bot-gradle commented Oct 1, 2021

Uh oh!

bot-gradle commented Oct 1, 2021

Uh oh!

ljacomet commented Oct 1, 2021

Uh oh!

bot-gradle commented Oct 1, 2021

BarkingBad commented Sep 9, 2021 •

edited

Loading

BarkingBad commented Sep 17, 2021 •

edited

Loading

BarkingBad commented Sep 20, 2021 •

edited

Loading

BarkingBad commented Sep 21, 2021 •

edited

Loading

ljacomet commented Oct 1, 2021 •

edited

Loading