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

[SPARK-14511][Build] Upgrade genjavadoc to latest upstream #12707

Closed
wants to merge 1 commit into from
Closed

[SPARK-14511][Build] Upgrade genjavadoc to latest upstream #12707

wants to merge 1 commit into from

Conversation

jodersky
Copy link
Member

@jodersky jodersky commented Apr 26, 2016

What changes were proposed in this pull request?

In the past, genjavadoc had issues with package private members which led the spark project to use a forked version. This issue has been fixed upstream (lightbend/genjavadoc#70) and a release is available for scala versions 2.10, 2.11 and 2.12, hence a forked version for spark is no longer necessary.
This pull request updates the build configuration to use the newest upstream genjavadoc.

How was this patch tested?

The build was run sbt unidoc. During the process javadoc emits some errors on the generated java stubs, however these errors were also present before the upgrade. Furthermore, the produced html is fine.

@jodersky
Copy link
Member Author

cc @mengxr
@JoshRosen, is the maven build ok? I could only find references to genjavadoc in sbt.

@jodersky jodersky changed the title Upgrade genjavadoc and use upstream version [SPARK-14511][Build] Upgrade genjavadoc and use upstream version Apr 26, 2016
@jodersky jodersky changed the title [SPARK-14511][Build] Upgrade genjavadoc and use upstream version [SPARK-14511][Build] Upgrade genjavadoc to latest upstream Apr 26, 2016
@SparkQA
Copy link

SparkQA commented Apr 26, 2016

Test build #57016 has finished for PR 12707 at commit f7d649c.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@srowen
Copy link
Member

srowen commented Apr 27, 2016

Seems reasonable to me if the upstream version does all that's needed.
I wonder if this new version happens to fix the problem in ... https://issues.apache.org/jira/browse/SPARK-3359

@jodersky
Copy link
Member Author

The work I submitted upstream doesn't specifically target that issue,
however it is actually plausible that it fixes the private interface issue
as a side effect. I haven't checked it though.

On Wed, Apr 27, 2016 at 1:14 AM, Sean Owen [email protected] wrote:

Seems reasonable to me if the upstream version does all that's needed.
I wonder if this new version happens to fix the problem in ...
https://issues.apache.org/jira/browse/SPARK-3359


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#12707 (comment)

@JoshRosen
Copy link
Contributor

I saw some errors running this locally though jekyll:

[error] /Users/joshrosen/Documents/spark/mllib/target/java/org/apache/spark/ml/recommendation/ALSModel.java:79: error: unknown tag: group
[error]   /** @group setParam */
[error]

Any idea what's going on here?

@JoshRosen
Copy link
Contributor

Actually, it looks like this might be broken even prior to this patch :(

@jodersky
Copy link
Member Author

jodersky commented Apr 27, 2016

Hmm, the current genjavadoc also produces errors on Spark so I didn't really look into it (I tested the upstream fix on Akka). How does the final output compare? Could this be due to java 8's stricter behaviour as @srowen mentioned?

@mengxr
Copy link
Contributor

mengxr commented Apr 27, 2016

@JoshRosen Are you using Java 8? I tested this on Java 7 with the latest master. It worked fine.

@JoshRosen
Copy link
Contributor

Yeah, I think that was it. Forgot that I had pinned Java 7 for docs on Jenkins.

@mengxr
Copy link
Contributor

mengxr commented Apr 27, 2016

Though the doc compiles, it doesn't seem that the package private objects are hidden. Attached a snapshot. @jodersky Could you double check?

screen shot 2016-04-27 at 4 15 30 pm

@srowen
Copy link
Member

srowen commented Apr 28, 2016

Yeah, the problem is that Java 8 has a much stricter javadoc tool. @group isn't a valid javadoc tag, so when ALSModel.scala gets translated to Java to javadoc it, it fails. I am pretty sure this was one of several reasons I couldn't proceed along these lines -- that and the fact that the translation in genjavadoc would generate private top level classes, which isn't allowed in Java.

@jodersky you seem pretty plugged in to genjavadoc or am I imagining that? I think I had a PR open or at least a suggestion for fixing one of those.

@jodersky
Copy link
Member Author

@srowen, I did quite some digging through the genjavadoc codebase when re-implementing @mengxr's initial fix. I can try to fix the group warnings and object privacy issues, however I can't give you a fixed deadline since I won't be available until the 10th.
Since the Spark 2.0 freeze is coming up, do you still want to merge this (since it supports Scala 2.12)? I'll fix it regardless for a future version.

@mengxr
Copy link
Contributor

mengxr commented Apr 29, 2016

+1 on merging this first and fixing remaining issues later.

@srowen
Copy link
Member

srowen commented Apr 29, 2016

Merged to master

@asfgit asfgit closed this in 7226e19 Apr 29, 2016
@mengxr
Copy link
Contributor

mengxr commented Apr 29, 2016

I created https://issues.apache.org/jira/browse/SPARK-15006 and lightbend/genjavadoc#83 to track the issue with package private objects.

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.

5 participants