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.

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

Replayed relevant changes from scaladoc3. #611

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

chr78rm
Copy link

@chr78rm chr78rm commented Jun 8, 2022

Fix for issue #604.

Copy link
Collaborator

@slandelle slandelle left a comment

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.

pom.xml Outdated
@@ -2,7 +2,7 @@
<modelVersion>4.0.0</modelVersion>
<groupId>net.alchim31.maven</groupId>
<artifactId>scala-maven-plugin</artifactId>
<version>4.6.3-SNAPSHOT</version>
<version>4.6.3-EXPERIMENTAL</version>
Copy link
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
Author

Choose a reason for hiding this comment

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

Done.

src/it/test_scaladoc3/validate.groovy Outdated Show resolved Hide resolved
Copy link
Owner

@davidB davidB left a comment

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
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
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.

src/main/java/scala_maven/ScalaMojoSupport.java Outdated Show resolved Hide resolved
src/main/java/scala_maven/ScalaDocMojo.java Outdated Show resolved Hide resolved
src/it/test_scaladoc3/validate.groovy Show resolved Hide resolved
src/it/test_scaladoc3/validate.groovy Outdated Show resolved Hide resolved
@alexfoxgill
Copy link

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

@chr78rm
Copy link
Author

chr78rm commented Oct 20, 2023

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

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

@slandelle
Copy link
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.

None yet

4 participants