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

Add support for scaladoc v.3 #1154

Closed

Conversation

romanowski
Copy link

@romanowski romanowski commented Feb 10, 2021

This PR adds support to scaladoc that will be released with Scala 3.0.0-RC1. The locally built version of this PR is was to test locally changes from scala/scala3#11349 - PR that removed scala3-doc (known as dottydoc as well).

Not heaving this PR is not a blocker for Scala 3.0.0-RC1 release however I would feel much better if I can merge scala/scala3#11349 without a pre-built version of mill.

@romanowski
Copy link
Author

cc @lefou

Copy link
Author

@romanowski romanowski left a comment

Choose a reason for hiding this comment

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

This PR makes all docJar tasks depending on compile. In the case of scaladoc v2.x and dottydoc, such dependency is not required however scaladoc v. 3.x is based on .tasty files so we need to trigger compilation to generate it.

I was looking for making this dependency applied only in case of scala 3.0.0-RC1 and newer however I was not able to find a way to achive it (basically I would need something like dynTask from SBT)

@@ -395,7 +395,7 @@ object HelloWorldTests extends TestSuite {
resourcePath = os.pwd / 'scalalib / 'test / 'resources / "hello-world"
){ eval =>
// scaladoc generation fails because of "-Xfatal-warnings" flag
val Left(Result.Failure("docJar generation failed", None)) = eval.apply(HelloWorldWithDocVersion.core.docJar)
val Left(Result.Failure("Compilation failed", None)) = eval.apply(HelloWorldWithDocVersion.core.docJar)
Copy link
Author

Choose a reason for hiding this comment

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

The failure message comes from compilation now.

@@ -169,7 +169,7 @@ object HelloWorldTests extends TestSuite {

object HelloWorldOnlyDocVersion extends HelloBase {
object core extends HelloWorldModule {
def scalacOptions = T(Seq("-Ywarn-unused", "-Xfatal-warnings"))
def scalacOptions = T(Seq("-Ywarn-unused"))
Copy link
Author

Choose a reason for hiding this comment

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

Now, docs trigger compilation and in case of HelloWorld it fails with a warning (turn to error) about unused import.

@lefou
Copy link
Member

lefou commented Feb 16, 2021

This PR makes all docJar tasks depending on compile. In the case of scaladoc v2.x and dottydoc, such dependency is not required however scaladoc v. 3.x is based on .tasty files so we need to trigger compilation to generate it.

I was looking for making this dependency applied only in case of scala 3.0.0-RC1 and newer however I was not able to find a way to achive it (basically I would need something like dynTask from SBT)

First, the way mills task tree works does not allow any dynamics in the dependencies which depend on the outcome of another task. We can partially achieve that with evaluator commands, but these would be no longer cachable. That said, having an unnecessary dependency in case of Scala 2.x isn't nice but probably something we could live with, as source documentation generation also needs valid source code and non-compiling source code probably isn't worth any documentation.
It would be fixable if the Scala version didn't come from a target, though.

But, it looks like you need to modify some compile setting to have a successful doc generation, at least in the test. So we either accept, that we can't generate docs if we can't compile, which might be ok. After all we can't create jars if compilation fails, too. Or we need to run a second compilation with modified settings, but this must be done in a new target. The latter would be the least invasive way, but probably with the slowest runtime performance.

@lefou
Copy link
Member

lefou commented Feb 16, 2021

Sorry, I have not very deep understanding how tasty works so here is another probably dumb though: If tasty is THE intermediate outcome, that is processed further to produce JVM binary, JS, natice or docs, we also could split the build into a new compileTasty target and let compile and fastOpt and also docJar depend on it.

Copy link
Member

@jodersky jodersky left a comment

Choose a reason for hiding this comment

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

Unfortunately I did not see this PR and created my own: #1190. Now, both PRs pretty much do the same thing (which is good I reckon :) ), however I also added some tests to mine which actually helped me weed out some subtle changes to the command line flags in scaladoc3.

I'm fine with going ahead with this one here, but in that case I would suggest to add some tests. You could apply the changes to the test/ directory from my PR, if you like.


val outputOptions =
if (isDottyOrScala3(scalaVersion()))
if (isDottyOrScala3(scalaVersion()) && !useScaladocInScala3(scalaVersion()))
Seq("-siteroot", javadocDir.toNIO.toString)
else
Seq("-d", javadocDir.toNIO.toString)
Copy link
Member

@jodersky jodersky Mar 3, 2021

Choose a reason for hiding this comment

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

I think we need a separate condition here for scaladoc3, which adds a separate -d and -siteroot parameter. (at least when I tested scaladoc 3 I needed to pass both those flags in)

@@ -213,7 +218,7 @@ trait ScalaModule extends JavaModule { outer =>
) match{
case true =>
val inputPath =
if (isDottyOrScala3(scalaVersion())) javadocDir / '_site
if (isDottyOrScala3(scalaVersion())&& !useScaladocInScala3(scalaVersion())) javadocDir / '_site
else javadocDir
Copy link
Member

Choose a reason for hiding this comment

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

would this still keep static files in scaladoc3? We already use javadocDir to consolidate all static files (so that we can support multiple effective site roots), so I think that because scaladoc3 no longer targets a _site subdirectory, it could lead into strange file-overwriting behavior.

When I worked on this, I needed to create a separate directory to use as the site root. See here.

@lefou
Copy link
Member

lefou commented Mar 4, 2021

@romanowski Do you have any intention to continue this PR? I think @jodersky is eager to get scaladoc 3 support in and I want to know if you're going to address his review comments or if I should review his PR #1190 instead.

@lefou
Copy link
Member

lefou commented Mar 6, 2021

@ramonmaruko (Edit: I meant to tag @romanowski) Thank you very much for your pull request! Unfortunately we had two submitted PRs for scaladoc v3 addition and I had to made a decission which one to push. Because of your silence to review comments and the fact that the other one already provided some additional tests, I decided to merge the other one (PR #1190). I highly appreciate your work.

@lefou lefou closed this Mar 6, 2021
@ramonmaruko
Copy link
Contributor

@lefou , I think you meant to tag @romanowski . :)

@lefou
Copy link
Member

lefou commented Apr 17, 2021

@lefou , I think you meant to tag @romanowski . :)

Oh yes, I'm sorry. Thanks for notifying me. Have a nice day!

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