Fix JavadocTool import for Java9 and up #2853
Conversation
|
Thanks for the PR! Could you also submit an issue about this so that we get it listed in the release notes? |
|
Didn't mean to close this. Having "Close and comment" and "Comment" buttons next to each others is annoying... |
|
Sure thing #2854 PS I am about to do a PR on the maven plugin too, for the same reasons. |
ghost
mentioned this pull request
|
Hopefully we could get @jussimalinen to fix the problem in Mavan plugin and create a new release. I'll ping him on Slack as well. |
|
I've been trying to test this locally but haven't really succeeded. What I've done:
How did you @Aquator test this? Is there a replacement for tools.jar when using Java 9+? |
|
Nope, it have been removed. Classes from it are moved to new modules, the Javadoc part is in the jdk.javadoc module, that is on the classpath by default, but it is not opened for others to access. JDK9 should give you a warning about that, but allow anyway, but later versions will fail. That is the trick I do in robotframework/MavenPlugin@99108b2 with the So with Java10 you probably also need to provide this argument, otherwise it will fail. I confess I had troubles running the tests, as invoke needed Python3 to create the jar, but tests used 2.7, and it ended up in a mess - so eventually tested the jar with the maven plugin. |
|
Do I get it right that with Java 10 you don't need tools.jar anymore (and it's not part of the distribution either) but then you need to use I haven't yet tested the aforementioned option, but based on my tests Libdoc works with Java 10 if using tools.jar. It's not too convenient if you don't have tools.jar otherwise because it's not part of the distribution. This might be something to take into account with this PR, though, because the proposed change would break using tools.jar with Java 10. Perhaps it should be changed so that the current import is tried first and then another import is used if it fails. It's true that at one point building RF (including the jar distribution) required Python 3.6 but running tests required Python 2.7. Luckily nowadays it's Python 3.6 (or newer) everywhere. |
|
Yup, thats the case. Not sure how libdoc works, if it has access to the java process - running a fork or something - we can add the arguments of course. If not, then the runner must provide that from java10 I am afraid.
Hmm. Not sure it breaks if you have the tools.jar on classpath, it will just load the classes from the java module instead. You will need the add opens in that case of course.
Your suggestion might be more safe - trying the new import when the other fails - but I think that is unlikely that you have jdk10 and you are fiddling with tools.jar. I agree having tools.jar and not the opens can work with java10 with this trick. Can have a better error message in that case too.
On the other hand, there is a more generic way of getting the JavadocTool via non-deprecated API, but that would involve a more intrusive change - If I recall there are some providers involved, and it returns an instance of a tool interface imlplementation, so no direct access to the class. Have no clue how it behaves with Jython. I can do some digging if you’d like that better.
Original plan was to make the Maven Plugin work, so that is the reason I tried to keep changes at a minimum level, but I am happy to assist to provide something better - add opens might be removed in future versions too. For jdk9 it is okay, for 10 you need the opens, who knows whats coming next - as I see Java world is still struggling with jigsaw.
|
|
Using non-deprecated APIs would obviously be a lot better. It would be great if you could take a look at that. Because using tools.jar is the only way to create docs nowadays, and it also works with Java 10, we should avoid breaking that. If generating docs requires strange command line options to be passed via Jython to Java, that needs to be separately documented. |
|
Okay, give me a few hours, I'll look into it. I'll also try to run the tests with Python2 this time :) |
|
It is heavily modified in 9 but I’ll try to come up with something this week. ClassDoc is deprecated too, I am trying to gather the same data with the new API. Of course no straightforward migration docs available :) |
|
Well, here is the thing: I think it can not be done with Jython at the moment. I can access the new ToolProvider and get a DocumentationTool instance, but if I want to go further and invoke anything on the doctool instance, the Jython will try to use reflection on jdk.javadoc.internal.api.JavadocTool - an implementation of the DocumentationTool. That will fail of course, since that class is not exported. Considering this, I think the best way we can make it work is your suggestion on importing order, and on jdk10 the --add-opens must be set in the running side, when invoking java. Unless of course there is a way to convince Jython not to access the direct type when calling the function. Could not stand it to use Jython's reflection to call Java's reflection ;) All hope is not lost just yet. |
|
Okay, I think I got this. Some more typing is needed to cover the constructor, method and parameter parts, but in general it is working. Hopefully I can update the PR with the changes early next week, and you can review things. |
|
This is another attempt. Please pardon my Python.... The base idea is to hide all the changes from libdoc. It might be a better idea to pull that stuff out to it's own file, I wanted to keep impact on minimum. Did some smoke tests, but an excessive feature test of the docs should be run - not sure how deeply atest tests this. I have 39 test failures with Can you review the changes? This should be more future-proof, and no need to tinkle with command line args. |
|
Heya, @pekkaklarck can you give a heads-up when you think you'll have time to look at it? - no rush, just preparing for summer holiday season :D |
|
I hopefully have time to look at this later today. Summer holiday season, most importantly kids who are not in school or kindergarten, is somewhat reducing my effectiveness.... |
|
Haha, I get you, we still have a couple of extra kindergarden days before all hell's loose. Thanks for making time for it. |
|
Finally got time to really look at this. You must have done a lot of digging to find replacements for JavaDoc tool. Luckily the code doesn't look too complicated, and I got few ideas how it possibly could be improved. A problem with the PR is that when I try to run Libdoc acceptance tests using Java 10 with these changes, plenty of tests related to handling Java based libraries fail. How have you tested the changes? Did you try running the existing acceptance tests? If you haven't looked at the acceptance tests earlier, you can find more information about them in The command I used was and this is the output I got: |
|
Thanks, I'll dig into those specific tests. I've run the tests, but I have some failures no matter what, even with Java8. I hoped it is something on my machine, locale or other badly configured Python thingie. But I can definitely check out these tomorrow. |
|
Okay, I was careless with varargs, used
|
|
@pekkaklarck can you take a look at it this week? Then I'll be off for the rest of the month. |
|
@pekkaklarck hey, it is just a gentle ping on this after holiday season ;) |
|
Sorry, haven't had time to look at this due to concentrating on implementing automatic type conversion. Once that's done, hopefully soon, I plan to the long overdue create 3.1 alpha 2 release and after that look at this and the other open pull requests. |
|
Hey @pekkaklarck this is another gentle poke. We are going java11 this month, and all my advocated robot tests will spin in mayhem (could hack with arguments for java9 but those times are over now). Is there any chance this will be pulled in, like soon, or should I roll my own versions of robot and maven plugin for the time being? |
|
Same question as @Aquator , I'm also blocked from moving our test project to java 10 due to this tools.jar issue. (My problem is on the MavenPlugin actually, as I cannot remove the dependency myself the same way I did with the other libs that were doing the same import). |
|
Finally time to look at this. The code for getting the information via Constructing if JYTHON:
if JAVA_VERSION < '1.9':
from .javabuilder import JavaDocBuilder
else:
from .java9builder import JavaDocBuilder
else:
def JavaDocBuilder():
raise DataError('Documenting Java test libraries requires Jython.')If the |
|
Absolutely. I have basically zero Python experience, so just shoveled everything together as a proof of concept. If that looks good I can make it tidy. Not sure java8 has those classes, it was a while ago. Can’t be used with all supported java as far as I can remember, gonna check that too in a couple of days, now I am enjoying virus bingo from the kids and agonizing in sick leave.
… Finally time to look at this. The code for getting the information via javax.tools.DocumentationTool looks somewhat complicated, but we cannot really improve JDK APIs. We just need to get the information we need and preferably get it via a stable API.
Constructing ClassDocLike based on the information we get doesn't feel like a good idea when in the end we just want to construct Libdoc's LibraryDoc instance. Wouldn't it be easier to have a separate builder that directly uses the info we get via javax.tools and constructs LibradyDoc instances based on that? It could reside in a separate module like javaxbuilder.py or java9builder.py and builder.py changed to contain:
if JYTHON:
if JAVA_VERSION < '1.9':
from .javabuilder import JavaDocBuilder
else:
from .java9builder import JavaDocBuilder
else:
def JavaDocBuilder():
raise DataError('Documenting Java test libraries requires Jython.')
If the javax.tools approach works also with Java 8, could consider using it also with it, though.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
That sounds good @Aquator. Except for that virus bingo. Hoping swift recovery! I try to get RF 3.1 beta 1 released today, them I'm on holiday next week, and aim for beta 2 (or rc 1) the week after. It would be great if you had time to look at this before that. |
|
@pekkaklarck did some refactoring, it indeed became smaller and better separated. Tested with Java7, Java8, Java10, Java11, all test passing (atest/run.py jython atest/robot/libdoc/java_library.robot) Since it is hard to have all these javas with jython and remain sane, I created some docker images, let me know if you'd like to have them for testing. (Based on OpenJDK) - there are no traces of Java9 left, so I did not test on that version. Hope I am still in time for release ;) |
|
Looking at the latest changes now. First thing I noticed was that |
|
Fair enough, using the new method now. Did an update on my branch from master, hope it's not a problem. |
|
Tested this locally and code seems to work pretty well. Noticed one test failing due to private constructors not being ignored. The failing test is After a bit more investigation noticed that private keywords aren't ignored either. The reason no test failed was that we didn't have any test for private Java keywords with Libdoc! Added two explicit tests in 1645e9d. |
|
I've added the filtering for constructors and methods, and moved the field filtering to the same place from the _get_attr so now only public things are returned in the first place. |
|
I would assume that fixing the aforementioned problem with private keywords and constructors isn't too complicated. After that all functionality ought to be there, but there are two more things to do:
|
|
I'll look into PEP-8, but I'd be happy to pass the documentation part. Is that okay to push the PEP-8 tomorrow? |
|
Confirmed locally that all Libdoc tests now pass with Java 10. Great work! |
|
OK, I'll take care of the documentation updates after this PR is merged. Code cleanup waiting until tomorrow is fine. I need to leave now anyway and may actually not have time for coding/reviews tomorrow either. I'm at the Agile Testing Days conference next week but ought to have time for reviews at some point. More actively coding the week after and trying to get RC 1 out then as well. |
|
Was not too many, so fixed those too, at least I hope |
|
Looks a lot better! I'll add some more detailed comments later today or tomorrow. |
There was a problem hiding this comment.
As discussed earlier, the code ought to work fine already. These comments are thus mainly about style and some of them are somewhat nitpickey. Please take a look and change those you think are worth changing. Feel free to ask for details here or on Slack if some of the comments aren't clear.
src/robot/libdocpkg/java9builder.py
Outdated
| if doc_comment is None: | ||
| return '' | ||
| else: | ||
| return '\n'.join(line.strip() for line in doc_comment.splitlines()) |
There was a problem hiding this comment.
Is this code needed to cleanup indentation? If yes, it might be possible to use use inspect.cleandoc here instead. Could then also be used by the old tools.jar code.
Some nitpickey comments about the style:
-
It is considered a bit more pythonic to use
if not doc_comment, notif doc_comment is Nonein cases like this. Personally I'd avoid the whole thing with something likedoc = elements.getDocComment(element) or ''. -
As a personal preference, I tend to omit
elsebranches when they aren't needed. Here theifbranch returns soelseis unnecessary. Obviously the wholeif/elseis removed if possibleNoneis handled withor ''.
There was a problem hiding this comment.
cleandoc makes things different here with leading whitespaces (they are removed only when they are uniform in all lines) that results in a failing test, but agree with the other parts
|
Apart from cleandoc I've basically pulled in all your suggestions, and tried to kick it in shape. Should be PEP-8 too. |
|
I would actually say |
|
Didn't find anything worth changing and merged this PR. Thanks for your hard work deciphering how to use the related Java APIs @Aquator! I'll update docs and consider taking |
JavadocTool, Messager and ModifierFilter is moved to a different package since Jigsaw.
Checked with
OpenJDK Runtime Environment (build 10.0.1+10-Ubuntu-3ubuntu1)
Java(TM) SE Runtime Environment (build 9.0.4+11)
Java(TM) SE Runtime Environment (build 9.0.4+11)