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.

Sign up for GitHub

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

Jump to bottom

Fix broken Scaladoc external references with JDK11 #8663

Merged
merged 1 commit into from Apr 16, 2020
Merged

Fix broken Scaladoc external references with JDK11 #8663

merged 1 commit into from Apr 16, 2020

Conversation

steven-barnes
Copy link
Contributor

@steven-barnes steven-barnes commented Jan 27, 2020 •
edited by SethTisue

Constructs URL for Java API references based on java.version; can override using -jdk-api-doc-base argument. partially addresses scala/bug#11839

only supports mappings to the java.base module — supporting other modules remains as future work

@scala-jenkins scala-jenkins added this to the 2.13.2 milestone Jan 27, 2020
@lrytz lrytz requested a review from retronym January 27, 2020 09:43
@SethTisue
Copy link
Member

SethTisue commented Jan 29, 2020 •
edited

tests passing — can you rebase/squash? once it's a single commit, the "combined" error will go away (the "combined" check means that every commit must be green, not just the last one)

@steven-barnes
Copy link
Contributor Author

My branch has merges from 2.13.x. Is it safe to rebase/squash them?

@SethTisue
Copy link
Member

yes. you should find that when you rebase, the merge commits will go away.

@steven-barnes
Copy link
Contributor Author

Can we get moving with the code review? I will be happy to rewrite the code as needed.

SethTisue
SethTisue reviewed Feb 4, 2020
View reviewed changes
Copy link
Member

@SethTisue SethTisue left a comment

Choose a reason for hiding this comment

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

I expect @retronym will attempt to review this in time for the 2.13.2 release, but he might be backed up at the moment.

project/ScalaOptionParser.scala Outdated
@@ -124,7 +124,7 @@ object ScalaOptionParser {
private def scalaDocBooleanSettingNames = List("-implicits", "-implicits-debug", "-implicits-show-all", "-implicits-sound-shadowing", "-implicits-hide", "-author", "-diagrams", "-diagrams-debug", "-raw-output", "-no-prefixes", "-no-link-warnings", "-expand-all-types", "-groups")
private def scalaDocIntSettingNames = List("-diagrams-max-classes", "-diagrams-max-implicits", "-diagrams-dot-timeout", "-diagrams-dot-restart")
private def scalaDocChoiceSettingNames = Map("-doc-format" -> List("html"))
private def scaladocStringSettingNames = List("-doc-title", "-doc-version", "-doc-footer", "-doc-no-compile", "-doc-source-url", "-doc-generator", "-skip-packages")
private def scaladocStringSettingNames = List("-doc-title", "-doc-version", "-doc-footer", "-doc-no-compile", "-doc-source-url", "-doc-generator", "-skip-packages", "jdk-api-doc-base")
Copy link
Member

Choose a reason for hiding this comment

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

the other strings begin with a hyphen but the new one doesn't, was that an oversight?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I'll change that

@SethTisue
Copy link
Member

@ashawley does this look plausible to you?

@SethTisue SethTisue mentioned this pull request Feb 4, 2020
Closed
@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Feb 4, 2020
@SethTisue SethTisue modified the milestones: 2.13.2, 2.13.3 Feb 6, 2020
@SethTisue
Copy link
Member

(retargeted for 2.13.3, but that doesn't mean that it can't still make 2.13.2)

@steven-barnes
Fix broken Scaladoc external Java references
8e5004a 
The problem was that PlainNioFile.underlyingSource was expanding "/modules/java.base" into a full path, where as the lookup in findExternalLink was using relative paths ("/modules/java.base.jmod")

Added -jdk-api-doc-base for specifying the base URL for Java API references
@ennru ennru mentioned this pull request Apr 15, 2020
Closed
@SethTisue
Copy link
Member

@lrytz wdyt about merging this for 2.13.2?

retronym
retronym approved these changes Apr 16, 2020
View reviewed changes
Copy link
Member

@retronym retronym left a comment

Choose a reason for hiding this comment

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

I think this is a great first step. I got a bit sidetracked when reviewing considering making things more general or performant, but we can refine this in a later release.

@scala scala deleted a comment from steven-barnes Apr 16, 2020
@scala scala deleted a comment from steven-barnes Apr 16, 2020
@lrytz lrytz modified the milestones: 2.13.3, 2.13.2 Apr 16, 2020
@ashawley
Copy link
Member

I wasn't able to look at this closely, but I recall it only supports mappings to the java.base module. Merging this as a first step is fine, but it doesn't fix scala/bug#11839 entirely, so it should stay open.

@SethTisue SethTisue merged commit 98c1648 into scala:2.13.x Apr 16, 2020
@SethTisue
Copy link
Member

I updated the PR description to include the java.base caveat

@SethTisue
Copy link
Member

thank you @steven-barnes!

SethTisue added a commit to SethTisue/scala that referenced this pull request Apr 21, 2020
@SethTisue
fix buggy Java version check in Scaladoc tool
412346f 
this is a post-2.13.1 regression, due to scala#8663

the bug was that the Java version might be e.g. "14", with
no . characters at all, causing StringIndexOutOfBoundsException

this was caught by the Scala community build
@SethTisue SethTisue mentioned this pull request Apr 21, 2020
Merged
SethTisue added a commit to SethTisue/scala that referenced this pull request Apr 21, 2020
@SethTisue
fix buggy Java version check in Scaladoc tool
c45a5f0 
this is a post-2.13.1 regression, due to scala#8663

the bug was that the Java version might be e.g. "14", with
no . characters at all, causing StringIndexOutOfBoundsException

this was caught by the Scala community build
SethTisue added a commit to SethTisue/scala that referenced this pull request Apr 21, 2020
@SethTisue
fix buggy Java version check in Scaladoc tool
8983b69 
this is a post-2.13.1 regression, due to scala#8663

the bug was that the Java version might be e.g. "14", with
no . characters at all, causing StringIndexOutOfBoundsException

this was caught by the Scala community build
This was referenced Apr 24, 2020
Closed
Closed
@ashawley
Copy link
Member

This change broke Scaladoc on Windows on JDK11, see scala/bug#11955

@steven-barnes
Copy link
Contributor Author

We have discussed a possible solution here, however I do not have a Windows machine to test.

@ashawley
Copy link
Member

ashawley commented Apr 24, 2020 •
edited

The discussion should continue at scala/bug#11955 and maybe someone could assign you the ticket.

@ochrons ochrons mentioned this pull request May 12, 2020
Closed
@steven-barnes steven-barnes deleted the issue/11839 branch May 12, 2020 23:29
@eed3si9n eed3si9n mentioned this pull request Aug 16, 2020
Closed
@mdedetrich mdedetrich mentioned this pull request Jul 17, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SethTisue SethTisue SethTisue left review comments

@retronym retronym retronym approved these changes

@lrytz lrytz lrytz approved these changes

Assignees
No one assigned
Labels
release-notes worth highlighting in next release notes
Projects
None yet
Milestone
2.13.2
6 participants
@steven-barnes @SethTisue @ashawley @retronym @lrytz @scala-jenkins