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

Fix javadoc urls on java 11 #84

Merged
merged 6 commits into from
Apr 7, 2020

Conversation

dpsutton
Copy link
Contributor

the core-java-api url prefix used from clojure.java.javadoc includes
the java.base module so we just strip that out and use the module as
we had been doing

Before submitting a PR make sure the following things have been done:

  • The commits are consistent with our contribution guidelines
  • You've added tests to cover your change(s)
  • All tests are passing
  • The new code is not generating reflection warnings

Keep in mind that new orchard builds are automatically deployed to Clojars
once a PR is merged, but only if the CI build is green.

Thanks!

@@ -363,7 +363,9 @@
(some (let [classname (.replaceAll path "/" ".")]
(fn [[prefix url]]
(when (.startsWith classname prefix)
(str url path))))
(str (cond-> url
(= 11 misc/java-api-version) (.replaceFirst "/java.base" ""))
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this a problem on newer JDK releases as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah. the latest version fixes this up.

@@ -363,7 +363,9 @@
(some (let [classname (.replaceAll path "/" ".")]
(fn [[prefix url]]
(when (.startsWith classname prefix)
(str url path))))
(str (cond-> url
Copy link
Member

Choose a reason for hiding this comment

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

Might be a good idea to add some comments explaining the need for the special handling.

@bbatsov
Copy link
Member

bbatsov commented Feb 23, 2020

The changes look good. You should also mention the bug fix in the changelog.

@dpsutton
Copy link
Contributor Author

There are some test failures in clojure 1.8. Working through those

10 "https://docs.oracle.com/javase/10/docs/api/"
11 "https://docs.oracle.com/en/java/javase/11/docs/api/"
12 "https://docs.oracle.com/en/java/javase/12/docs/api/"
13 "https://docs.oracle.com/en/java/javase/13/docs/api/"})
Copy link
Member

Choose a reason for hiding this comment

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

Probably we should also add some fallback for unreleased versions - I assume they are going to have the same URL format as JDK 11+.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked at 14 is quite different:

14:
https://download.java.net/java/GA/jdk14/docs/api/java.base/java/lang/String.html

13:
https://docs.oracle.com/en/java/javase/13/docs/api/java.base/java/lang/String.html

I'm not sure if 14 is still in some kind of preview or if this is a new schema

(some (let [classname (.replaceAll path "/" ".")]
(fn [[prefix url]]
(fn [[prefix url|version->url]]
Copy link
Member

Choose a reason for hiding this comment

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

Haven't seen such a naming convention yet. :-)

@bbatsov
Copy link
Member

bbatsov commented Feb 25, 2020

Seems something's broken for Java 8.

@dpsutton
Copy link
Contributor Author

Fixed! Thanks @bbatsov for looking over this. Should be good to go.

@@ -345,26 +345,36 @@
(first ms)
{:candidates (zipmap (map :class ms) ms)})))))

(def backported-javadoc-bases
Copy link
Member

Choose a reason for hiding this comment

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

Probably we can name this javadoc-base-urls. I think backported is a bit confusing in this context.

@bbatsov
Copy link
Member

bbatsov commented Feb 29, 2020

@dpsutton Looks good to me. Just check my tiny remark, add some changelog entry and squash the related commits together.

@bbatsov
Copy link
Member

bbatsov commented Feb 29, 2020

Seems you also have to rebase on top of master.

the core-java-api url prefix used from clojure.java.javadoc includes
the java.base module so we just strip that out and use the module as
we had been doing
clojure.java.javadoc uses dynamic vars but stuffs them in a map so
its effectively no longer dynamic. Further, clojure 1.8 didn't have
javadoc url prefixes for jdk 11 so we just port over all the
information it was trying to do but in a better manner.

We could try to reuse the machinery in there but we would be updating
6 of 11 keys to whatever the proper api is so its easier to just copy
them over.
In order to do so, need to use clojure.java.javadoc/*remove-javadocs*
but we update it with a proper version of the javadoc root. Some
issues are that clojure 1.8 has an entry for java 7 and for java 8 so
running clojure 1.8 on java 11 can't use javadocs without this
hack. Clojure 1.10.1 doesn't have a javadoc source for java 13 so this
ensures a consistent presence across all versions.
If we have an unrecognized java version, presumably it is higher than
11. It should have a notion of module name like 11 and 13. Running
this test on 8 breaks the test because there is no module name so that
part is missing from the slug. Seems the unrecognized versions will
most likely follow this path so we fallback to java 11 javadocs with a
module name rather than java 8 without them.
@dpsutton dpsutton force-pushed the fix-javadoc-modules branch from 87d0364 to 50ec3aa Compare February 29, 2020 17:30
@affan-salman
Copy link

affan-salman commented Mar 4, 2020

Hi @dpsutton, is this complete from your perspective? I'm working on a Java-interop-heavy project and will appreciate any form of early access if this can't be released soon.

@affan-salman
Copy link

Hi @bbatsov, while I'm not familiar with orchard, I'm happy to test, provide feedback or help out if it can be useful.

@dpsutton
Copy link
Contributor Author

dpsutton commented Mar 4, 2020

@affan-salman I think its done but being in orchard, it needs a new release here and then a release of cider-nrepl to require it.

In the meantime you can test it and use it locally by the following:

user> (apropos "resolve-javadoc")
(cider.nrepl.inlined-deps.orchard.v0v5v6.orchard.java/resolve-javadoc-path)
user> (dir cider.nrepl.inlined-deps.orchard.v0v5v6.orchard.java)
Reflected
cache
class-info
class-info*
javadoc-url
jdk-find
jdk-sources
jdk-tools
member-info
module-name
reflect-info
resolve-class
resolve-javadoc-path
resolve-member
resolve-symbol
source-info
type-info
typesym
nil
user> (in-ns 'cider.nrepl.inlined-deps.orchard.v0v5v6.orchard.java)
#namespace[cider.nrepl.inlined-deps.orchard.v0v5v6.orchard.java]
cider.nrepl.inlined-deps.orchard.v0v5v6.orchard.java> 
(def javadoc-base-urls
  "Copied from clojure.java.javadoc. These are the base urls for
  javadocs from `clojure.java.javadoc/*core-java-api*`. It is here for
  two reasons:
  1. add java 13 to this list
  2. backport for older Clojures"
  {8 "https://docs.oracle.com/javase/8/docs/api/"
   9 "https://docs.oracle.com/javase/9/docs/api/"
   10 "https://docs.oracle.com/javase/10/docs/api/"
   11 "https://docs.oracle.com/en/java/javase/11/docs/api/"
   12 "https://docs.oracle.com/en/java/javase/12/docs/api/"
   13 "https://docs.oracle.com/en/java/javase/13/docs/api/"})

(defn resolve-javadoc-path
  "Resolve a relative javadoc path to a URL and return as a map. Prefer javadoc
  resources on the classpath; then use online javadoc content for core API
  classes. If no source is available, return the relative path as is."
  [^String path]
  (or (resource/resource-full-path path)
      (some (let [classname (.replaceAll path "/" ".")]
              (fn [[prefix url]]
                (when (.startsWith classname prefix)
                  (str url path))))
            (into @javadoc/*remote-javadocs*
                  ;; clojure 1.8 has no javadoc for anything beyond java
                  ;; 8. clojure 1.10.1 doesn't have 13. We just backport them
                  ;; regardless of clojure version
                  (zipmap ["java." "javax." "org.ietf.jgss." "org.omg." "org.w3c.dom." "org.xml.sax"]
                          (repeat (or (javadoc-base-urls misc/java-api-version)
                                      (javadoc-base-urls 11))))))
      path))
#'cider.nrepl.inlined-deps.orchard.v0v5v6.orchard.java/javadoc-base-urls#'cider.nrepl.inlined-deps.orchard.v0v5v6.orchard.java/resolve-javadoc-path
cider.nrepl.inlined-deps.orchard.v0v5v6.orchard.java> 

@affan-salman
Copy link

affan-salman commented Mar 4, 2020

Fantastic @dpsutton! I've tested atop AdoptOpenJDK 11.0.6 and I was able to see javadoc from docs.oracle.com for classes under java.util instead of seeing HTTP 404.

The same test also worked correctly with AdoptOpenJDK 1.8.0_242, of course.

@dpsutton
Copy link
Contributor Author

dpsutton commented Apr 7, 2020

@bbatsov ping :) should be good to merge.

@bbatsov bbatsov merged commit 393f91f into clojure-emacs:master Apr 7, 2020
@bbatsov
Copy link
Member

bbatsov commented Apr 7, 2020

Thanks for the reminder! :-)

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.

3 participants