Skip to content

Replayed relevant changes from scaladoc3. - #611

Open
chr78rm wants to merge 5 commits into
davidB:masterfrom
chr78rm:issue-604-1
Open

Replayed relevant changes from scaladoc3.#611
chr78rm wants to merge 5 commits into
davidB:masterfrom
chr78rm:issue-604-1

Conversation

@chr78rm

@chr78rm chr78rm commented Jun 8, 2022

Copy link
Copy Markdown

Fix for issue #604.

@chr78rm
chr78rm requested review from davidB and slandelle as code owners June 8, 2022 09:46

@slandelle slandelle left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for your contribution.

Please replace all sample code with your personal package and a GPL license with more simple and public domain license (see https://github.com/davidB/scala-maven-plugin/blob/master/UNLICENSE) code.

Comment thread pom.xml Outdated
<groupId>net.alchim31.maven</groupId>
<artifactId>scala-maven-plugin</artifactId>
<version>4.6.3-SNAPSHOT</version>
<version>4.6.3-EXPERIMENTAL</version>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please revert this change.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done.

Comment thread src/it/test_scaladoc3/validate.groovy Outdated

@davidB davidB left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for the PR, as also noticed by @slandelle , please provide a sample / test under the same license (and maybe an other sample, more basic)

Comment on lines +157 to +164
String apidocMainClassName = sc.apidocMainClassName(scaladocClassName);
JavaMainCaller jcmd;
if (sc.version().major < 3) {
jcmd = getEmptyScalaCommand(apidocMainClassName);
} else {
String targetClassesDir = project.getModel().getBuild().getOutputDirectory();
jcmd = new ScalaDoc3Caller(this, apidocMainClassName, targetClassesDir);
}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The original purpose of Context (and findScalaContext) is to avoid / to remove every if scalaVersion ... over the code. Can you try to keep this approach ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I tried to used getEmptyScalaCommand() but I couldn't get it working. In this case, the invocation of dotty.tools.scaladoc.Main raises a NullPointerException:

java.lang.NullPointerException: Cannot invoke "java.lang.ClassLoader.getResourceAsStream(String)" because the return value of "java.lang.Class.getClassLoader()" is null
        at dotty.tools.scaladoc.renderers.Resources.renderResource(Resources.scala:213)
        at dotty.tools.scaladoc.renderers.Resources.renderResource$(Resources.scala:26)
        at dotty.tools.scaladoc.renderers.Renderer.renderResource(Renderer.scala:27)
        at dotty.tools.scaladoc.renderers.HtmlRenderer.renderResources$$anonfun$1(HtmlRenderer.scala:60)
        at scala.collection.immutable.List.flatMap(List.scala:293)
        at scala.collection.immutable.List.flatMap(List.scala:79)
        at dotty.tools.scaladoc.renderers.HtmlRenderer.renderResources(HtmlRenderer.scala:60)
        at dotty.tools.scaladoc.renderers.HtmlRenderer.render(HtmlRenderer.scala:31)
        at dotty.tools.scaladoc.Scaladoc$.run(Scaladoc.scala:249)
        at dotty.tools.scaladoc.Scaladoc$.run$$anonfun$1(Scaladoc.scala:84)
        at scala.runtime.function.JProcedure1.apply(JProcedure1.java:15)
        at scala.runtime.function.JProcedure1.apply(JProcedure1.java:10)
        at scala.Option.map(Option.scala:242)
        at dotty.tools.scaladoc.Scaladoc$.run(Scaladoc.scala:88)
        at dotty.tools.scaladoc.Main.run(Main.scala:18)
        at dotty.tools.scaladoc.Main$.main(Main.scala:24)
        at dotty.tools.scaladoc.Main.main(Main.scala)
        at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
        at java.base/java.lang.reflect.Method.invoke(Method.java:577)
        at scala_maven_executions.MainHelper.runMain(MainHelper.java:157)
        at scala_maven_executions.MainWithArgsInFile.main(MainWithArgsInFile.java:30)

I tried to shift things from the bootclasspath to the normal classpath and some other things but without success. IMHO you should consider the removal of the Apache Commons Exec library in favor of ProcessBuilder and related classes from the JDK in the medium term. The Exec library hasn't seen any updates in years whereas ProcessBuilder and related classes have seen some improvements. I might be wrong about this but if someone upstream (Scala compiler etc.) decides that the usage of java.lang.Class.getClassLoader() might come in handy you might run into a similar issue as shown above.

The error above can be reproduced with the branch classloader-issue of the forked repo. This branch isn't merged but git diff master..classloader-issue gives you the relevant changes.

Since we are raising external processes the outcome depends very much on how well the environment has been prepared. But, I don't think the classloader issue has something to do with the environment since the ProcessBuilder works well enough.

I have used the interface JavaMainCaller to hide the internal details of the process invocation.

Comment thread src/main/java/scala_maven/ScalaMojoSupport.java Outdated
Comment thread src/main/java/scala_maven/ScalaDocMojo.java Outdated
Comment thread src/it/test_scaladoc3/src/main/scala/xyz/math/Polynomial.scala Outdated
Comment thread src/it/test_scaladoc3/validate.groovy
Comment thread src/it/test_scaladoc3/validate.groovy Outdated
@alexfoxgill

Copy link
Copy Markdown

@chr78rm & @slandelle, any chance this PR could be resurrected? it looks like it was almost over the line :)

@chr78rm

chr78rm commented Oct 20, 2023

Copy link
Copy Markdown
Author

Not this one, but I can provide an updated PR rooted at the latest commit from @slandelle in due course, if desired.

I have to note although, that I have observed some regressions regarding the generation of scaladocs beginning with Scala version 3.2.0. The same regressions can be reproduced by the direct usage of scaladoc via command line. The scaladocs will be produced but they have some defects, e.g. the links can only be followed by opening a new tab.

You might have noticed, that the documentation concerning Scala builds - inclusive the generation of scaladocs - at the EPFL in Lausanne is centered around the scala build tool (sbt), see e.g. https://dotty.epfl.ch/docs/contributing/scaladoc.html.

Perhaps the invocation via sbt adds some magic which is currently missing (at least starting with Scala version 3.2.0). That is, the invocation of scaladoc might have to be tweaked for the recent versions. Someone might have to reach out to the scaladoc team at the EPFL for clarification.

@alexfoxgill

Copy link
Copy Markdown

thanks @chr78rm, it seems like perhaps maven and scala are just not meant to be; we'll explore some other avenues

@slandelle

Copy link
Copy Markdown
Collaborator

@alexfoxgill Honestly, even as a maintainer of this project (will low bandwidth), this is also my feeling.

AFAIK, the main interest for Scala with maven come from:

  • Spark, but they never contribute and I doubt they'll consider Scala 3 any time soon
  • us from Gatling but we consider this combo as legacy

If there isn't more contributions from power users, this plugin is going to stall in terms of innovation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants