RFR : 8219958 : Automatically load taglets from a jar file
Jonathan Gibbons
jonathan.gibbons at oracle.com
Wed Mar 20 18:26:38 UTC 2019
Priya,
There may be a latent error here that needs addressing. Using /othervm
is an OK workaround, for now, but it would be good to understand and try
to fix the underlying issue.
First, can you recreate the issue locally on Windows, by removing
/othervm and maybe running just the javadoc tests?
Almost certainly the problem is caused by the classloader used to load
the taglets ... that classloader probably needs to be closed, either
directly or maybe by closing the file manager.
-- Jon
On 03/19/2019 08:38 PM, Priya Lakshmi Muthuswamy wrote:
>
> Hi,
>
> When the ran the tests in mach5, I got an error in windows-x64
> platform (Error. failed to clean up files after test).
> In all other environments its working fine.
> windows is failing to delete the jar file .
>
> So I have specified @run main/othervm for the test.
>
> Thanks,
> Priya
>
> On 3/16/2019 6:40 AM, Jonathan Gibbons wrote:
>>
>> TagletManager, line 230, change "Add" to "Adds".
>>
>> Then you're OK.
>>
>> -- Jon
>>
>>
>> On 03/14/2019 10:11 PM, Priya Lakshmi Muthuswamy wrote:
>>>
>>> Hi Jon,
>>>
>>> updated the webrev with suggested comments.
>>>
>>> webrev : http://cr.openjdk.java.net/~pmuthuswamy/8219958/webrev.02/
>>>
>>> Thanks,
>>> Priya
>>>
>>> On 3/15/2019 3:51 AM, Jonathan Gibbons wrote:
>>>>
>>>> In the properties file, for doclet.not_standard_file_manager,
>>>> change the comma to a semicolon or colon.
>>>>
>>>> In TagletManager,
>>>>
>>>> line 207: no doc comment on the new method
>>>>
>>>> line 245: fix capitalization "Loads taglets from a taglet path..."
>>>>
>>>> line 247: @throws does not match actual "throws" and is empty
>>>>
>>>> line 251: creating a spliterator to count the items on an iterable
>>>> to see if it is emprty is overkill, and neglects the possibility
>>>> that the iterable may be null. The if statement may not be strictly
>>>> necessary, but if it is, I recommend
>>>> 251 if (location != null && location.iterator().hasNext()) {
>>>> line 263: the convention for javadoc comments on methods is to use
>>>> 3rd person declarative tense, not 2nd person imperative tense. In
>>>> other words, the verb should be "Registers".
>>>>
>>>> In the test, there's a lot of duplicated text, generating the two
>>>> source files; I suggest using a common method, taking a String or
>>>> int parameter that is used to differentiate the instances.
>>>>
>>>> While the test does load more than one taglet, that is not the
>>>> defining characteristic of the test; the defining characteristic
>>>> is that the taglets are loaded automatically, using ServiceLoader,
>>>> instead of explicitly, using command line args. I suggest you
>>>> rename the test to TestAutoLoadTaglets or something like that.
>>>>
>>>> -- Jon
>>>>
>>>> On 03/12/2019 04:33 AM, Priya Lakshmi Muthuswamy wrote:
>>>>>
>>>>> Hi Jon,
>>>>>
>>>>> I have updated the webrev with the suggested changes.
>>>>> In TagletManager.java, LoadTaglets method, have removed the
>>>>> condition on tagletpath, instead applied
>>>>> check on filemanager.getLocation.
>>>>>
>>>>> updated webrev:
>>>>> http://cr.openjdk.java.net/~pmuthuswamy/8219958/webrev.01/
>>>>>
>>>>> Thanks,
>>>>> Priya
>>>>>
>>>>> On 3/9/2019 5:58 AM, Jonathan Gibbons wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 03/06/2019 02:56 AM, Priya Lakshmi Muthuswamy wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Kindly review the changes for automatically loading taglets from
>>>>>>> a service provider jar.
>>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8219958
>>>>>>> webrev: http://cr.openjdk.java.net/~pmuthuswamy/8219958/webrev.00/
>>>>>>> CSR: https://bugs.openjdk.java.net/browse/JDK-8219959
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Priya
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> Priya,
>>>>>>
>>>>>> *BaseConfiguration:*
>>>>>>
>>>>>> You've moved the filemanager setup from TagletManager:216-226
>>>>>> into BaseConfiguration.
>>>>>> I agree it needs to be moved from its current position, but I
>>>>>> think it should stay in TagletManager.
>>>>>> I suggest that you create a new method in TagletManager to init
>>>>>> the taglet path.
>>>>>>
>>>>>> The beginning of BaseConfiguration.initTagletManager would then
>>>>>> be something like:
>>>>>> 837 tagletManager = tagletManager == null ?
>>>>>> 838 new TagletManager(nosince, showversion, showauthor, javafx, this) :
>>>>>> 839 tagletManager;
>>>>>> tagletManager.initTagletPath(tagletPath);
>>>>>> 840 for (List<String> args : customTagStrs) {
>>>>>> 841 if (args.get(0).equals("-taglet")) {
>>>>>> 842 tagletManager.addCustomTag(args.get(1), getFileManager(),
>>>>>> tagletpath);
>>>>>> 843 continue;
>>>>>> 844 }
>>>>>>
>>>>>> As part of the suggestion to move stuff into BaseConfiguration,
>>>>>> you had suggested using
>>>>>> `try ... catch (Exception e)`. That would have been overly
>>>>>> broad; if we were to continue
>>>>>> to do that, it would have been better to narrow the caught
>>>>>> exception class to IOException.
>>>>>>
>>>>>> *doclets.properties:*
>>>>>>
>>>>>> The new error message needs improving. Think of how it would
>>>>>> appear to an end-user:
>>>>>>
>>>>>> /Error: could not set location for /some/path/input/by/the/user/
>>>>>>
>>>>>> What is that going to mean to a user? You also don't give any
>>>>>> additional diagnostic information
>>>>>> that might be of assistance to remedy the situation. I don't like
>>>>>> having to resort to using `e.toString()`
>>>>>> but when that's the only option, it's all we can do, and better
>>>>>> than nothing.
>>>>>>
>>>>>> I suggest the message should be something like:
>>>>>> /
>>>>>> //Error: could not set the taglet path: {0}/
>>>>>>
>>>>>> where the provided value is the exception message.
>>>>>> *
>>>>>> **TagletManager.java:*
>>>>>>
>>>>>> I've already suggested that you should move the init of the
>>>>>> taglet path back into TagletManager.
>>>>>> The init code on lines 216-226 is somewhat curious, in that it
>>>>>> gives precedence to the TAGLET_PATH
>>>>>> in the file manager if it has been set ... that may be because
>>>>>> the code was executed repeatedly,
>>>>>> because addCustomTag is called in a loop.
>>>>>>
>>>>>> We should refactor the init, to honor the taglet path option if
>>>>>> it is set, and the TAGLET_PATH if it
>>>>>> is set when calling javadoc through the javax.tools API.
>>>>>>
>>>>>> Put that all together, and the initTagletPath code should look
>>>>>> something like the following (in pseudo-code)
>>>>>>
>>>>>> void initTagletManager(String tagletPath) {
>>>>>> if (fileManager instanceof StandardJavaFileManager) {
>>>>>> Standard JavaFileManager sfm =
>>>>>> (StandardJavaFileManager) fileMaanger;
>>>>>> if (tagletPath != null) {
>>>>>> sfm.setLocation(TAGLET_PATH,
>>>>>> tagletPath.splitAsStream(File.pathSeparator).collect(Collectors.toList()));
>>>>>> } else if (!sfm.hasLocation(TAGLET_PATH)) {
>>>>>> sfm.setLocation(TAGLET_PATH,
>>>>>> Collections.emptyList());
>>>>>> }
>>>>>> } else if (tagletPath != null) {
>>>>>> messages.error("cannot set taglet path; the file
>>>>>> manager is not a StandardJavaFileManager");
>>>>>> }
>>>>>> }
>>>>>>
>>>>>> In addCustomTag, what exceptions are you catching? It may be that
>>>>>> you can just catch
>>>>>> ReflectiveOperationException instead of Exception. If there are
>>>>>> other exceptions being thrown,
>>>>>> consider using a multi-catch.
>>>>>>
>>>>>> In loadTagletsJar ... the method name is weird as well as the doc
>>>>>> comments.
>>>>>> I suggest ...
>>>>>>
>>>>>> * calling the method "loadTaglets" or "loadRegisteredTaglets"
>>>>>> or something like that.
>>>>>> * the javadoc should be "Loads taglets from the taglet path
>>>>>> using ServiceLoader."
>>>>>>
>>>>>> Remove the parameter "tagletPath" and remove the "if (tagletPath
>>>>>> != null)" surrounding the
>>>>>> use of ServiceLoader. There is no need for the user to use
>>>>>> -tagletpath ... the user could be
>>>>>> calling javadoc through the javax.tools API with a preconfigured
>>>>>> filemanager that already
>>>>>> has the TAGLET_PATH set up. Also, it would be better to split
>>>>>> lines 234,235 after the '='
>>>>>> instead of before the '(' to avoid splitting the method call.
>>>>>>
>>>>>> 234 ServiceLoader<jdk.javadoc.doclet.Taglet> serviceLoader =
>>>>>> 235 fileManager.getServiceLoader(TAGLET_PATH,
>>>>>> jdk.javadoc.doclet.Taglet.class);
>>>>>>
>>>>>>
>>>>>> In registerTaglet, you should not need to pass in the class name,
>>>>>> since it is just the class name
>>>>>> of the instance parameter, isn't it?
>>>>>>
>>>>>> Also in registerTaglet, although you haven't modified it, the
>>>>>> body seems excessively verbose.
>>>>>> How about this:
>>>>>>
>>>>>> 245 /**
>>>>>> 246 * Registers the Taglet.
>>>>>> 247 * @param instance the Taglet instance
>>>>>> 249 */
>>>>>> 250 private void registerTaglet(jdk.javadoc.doclet.Taglet instance) {
>>>>>> 251 instance.init(docEnv, doclet);
>>>>>> 252 Taglet newLegacy = new UserTaglet(instance);
>>>>>> 258 allTaglets.put(newLegacy.getName(), newLegacy);
>>>>>> 259 messages.notice("doclet.Notice_taglet_registered",
>>>>>> instance.getClass().getName());
>>>>>> 260 }
>>>>>>
>>>>>> It's pretty icky having two classes with simple name Taglet,
>>>>>> forcing the use of fully-qualified
>>>>>> names. At some point (not necessarily in this review), we should
>>>>>> rename the internal one
>>>>>> to something else. (We can't change the name of the one in the
>>>>>> public API.)
>>>>>>
>>>>>> -- Jon
>>>>>>
>>>>>>
>>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/javadoc-dev/attachments/20190320/10723f00/attachment-0001.html>
More information about the javadoc-dev
mailing list