RFR 8170162: jshell tool: no mechanism to programmatically launch

Sat Dec 10 04:27:37 UTC 2016

Here is version 2, which includes the the doc changes made in v1 plus 
moving from a ServiceLoader based SPI to a simple sole-instance SPI. --

API:

     http://cr.openjdk.java.net/~rfield/8170162v2.jshellAPI/

Webrev:

     http://cr.openjdk.java.net/~rfield/8170162v2.webrev/

Specdiff:

http://cr.openjdk.java.net/~rfield/8170162v2.specdiff/overview-summary.html

The root workspace javadoc make delta is the same as v1:

http://cr.openjdk.java.net/~rfield/8170195v1.webrev/make/Javadoc.gmk.sdiff.html

The jdk workspace launching delta is the same as the original v0:

http://cr.openjdk.java.net/~rfield/8170194v0.webrev/make/launcher/Launcher-jdk.jshell.gmk.sdiff.html

Since it has been a while, the bugs --

Bug:

     8170162: jshell tool: no mechanism to programmatically launch
     https://bugs.openjdk.java.net/browse/JDK-8170162

Sub-Tasks (tiny tweaks to other repos):

     8170194: jshell tool (jdk repo): launch tool from JShellToolProvider
     https://bugs.openjdk.java.net/browse/JDK-8170194

     8170195: jshell tool (make): update javadoc generation for jdk.jshell
     https://bugs.openjdk.java.net/browse/JDK-8170195

Discussion --

The simplest of the options below (3) was chosen -- builder() factory on 
JavaShellToolBuilder.

http://cr.openjdk.java.net/~rfield/8170162v2.jshellAPI/jdk/jshell/tool/package-summary.html

The uses/provides changes to module-info.java have been removed, leaving 
the addition of "exports jdk.jshell.tool", and, to work with the 
merged-in jigsaw, the java.prefs requires is now transitive:

http://cr.openjdk.java.net/~rfield/8170162v2.webrev/src/jdk.jshell/share/classes/module-info.java.sdiff.html

JavaShellToolProvider has been removed.

Before you ask, I have left the internal JShellToolProvider, which will 
make more sense when javax.tools.Tool is hooked in, but for now it 
uniformly channels all jshell tool launching through the builder (rather 
than entering via JShellTool).

Note: specdiff doesn't see the changes to overview.html (removal) and 
module docs, which are now:

http://cr.openjdk.java.net/~rfield/8170162v2.jshellAPI/jdk.jshell-summary.html

Thanks,
Robert

On 12/08/16 19:45, Robert Field wrote:
> More on sole-instance vs ServiceLoader loader look-up...
>
> The choice is between:
>   * A fixed sole-instance of the jshell tool baked into the platform, 
> with a very simple access to that instance
>   * A ServiceLoader-based look-up of the tool, with a couple more 
> methods in the interface, and a couple more calls to get the instance
>
> The philosophy of KISS (Keep It Simple Stupid) would argue for 
> sole-instance unless it foreclosed or severely constrained on an 
> interesting future.  It does not, in that the ServiceLoader approach 
> could, if needed, be layered on top of what would become 
> standard-instance access.
>
> Given that, there a few options of what the interface should be --
>
> (1) As current, but replace the look-up with access:
>
>   public interface JavaShellToolProvider {
>     String name();
>     JavaShellToolBuilder builder();
>     static JavaShellToolProvider instance();
>   }
>
> (2) But, the name will always return the same thing, and then the only 
> interesting thing you can do is call the builder, so it could go 
> directly to the builder:
>
>   public interface JavaShellToolProvider {
>     static JavaShellToolBuilder builder();
>   }
>
> (3) But, an interface with one static method seems silly, rather, the 
> factory could sit directly on the builder:
>
>   public interface JavaShellToolBuilder {
>
>     ...
>
>     static JavaShellToolBuilder builder();
>   }
>
> Thoughts?
>
> -Robert
>
>
> On 12/08/16 07:37, Robert Field wrote:
>> Updated per review --
>>
>> Webrevs:
>>
>>     http://cr.openjdk.java.net/~rfield/8170162v1.webrev/
>>
>>     http://cr.openjdk.java.net/~rfield/8170195v1.webrev/
>>
>> API:
>>
>>     http://cr.openjdk.java.net/~rfield/8170162v1.jshellAPI/
>>
>> SpecDiff:
>>
>> http://cr.openjdk.java.net/~rfield/8170162v1.specdiff/overview-summary.html 
>>
>>
>> References in-line...
>>
>> On 12/07/16 20:26, Robert Field wrote:
>>> Thank you for the review, Jon!
>>>
>>> Responses in-line below.
>>>
>>> On 12/07/16 18:50, Jonathan Gibbons wrote:
>>>> Many of the new files in the langtools webrev do not have the 
>>>> correct, required, legal header. This is a MUST-FIX,
>>>
>>> Oops!!
>>
>> http://cr.openjdk.java.net/~rfield/8170162v1.webrev/src/jdk.jshell/share/classes/jdk/internal/jshell/tool/JShellToolBuilder.java.html 
>>
>> http://cr.openjdk.java.net/~rfield/8170162v1.webrev/src/jdk.jshell/share/classes/jdk/internal/jshell/tool/JShellToolProvider.java.html 
>>
>> http://cr.openjdk.java.net/~rfield/8170162v1.webrev/src/jdk.jshell/share/classes/jdk/internal/jshell/tool/PersistentStorage.java.html 
>>
>>
>>>
>>>>
>>>> In the new package-info file, this comment is a little bit hokey 
>>>> and/or out of place.
>>>>
>>>>   52  * Note: The Java™ shell tool is built on the {@link 
>>>> jdk.jshell} API,
>>>>   53  * but the tool is at a different level, and this launching 
>>>> SPI does not
>>>>   54  * interact with the {@link jdk.jshell} API.
>>>
>>> Not sure what is hokey, but the additional documentation I added to 
>>> overview,html clarifies the relationship between the layers more 
>>> clearly and is more appropriately placed than this, so, I will remove.
>>
>> http://cr.openjdk.java.net/~rfield/8170162v1.jshellAPI/jdk/jshell/tool/package-summary.html 
>>
>>
>> http://cr.openjdk.java.net/~rfield/8170162v1.jshellAPI/jdk.jshell-summary.html 
>>
>>
>>> BTW: Seems I should dump overview.html, putting that in 
>>> module-info.java?
>>
>> Removed --
>>
>> http://cr.openjdk.java.net/~rfield/8170195v1.webrev/make/Javadoc.gmk.sdiff.html 
>>
>>
>> http://cr.openjdk.java.net/~rfield/8170162v1.webrev/
>>
>>
>>>
>>>>
>>>> At a minimum, I suggest making it an @apiNote, but I would 
>>>> recommend trying to rethink/reconsider
>>>> what you are trying to say here.
>>>>
>>>> I think you should figure a way and place to specify the non-null 
>>>> name used by the standard implementation
>>>> of the JShellToolProvider. That being said ....
>>>
>>> "The name of the standard implementation is 'jshell'" ???
>>> In jdk.jshell.tool.JavaShellToolProvider.name()
>>
>> http://cr.openjdk.java.net/~rfield/8170162v1.jshellAPI/jdk/jshell/tool/JavaShellToolProvider.html 
>>
>>
>>>
>>>>
>>>> I guess I'm surprised you need to define a JShellToolProvider PAI 
>>>> -- do you expect there to be multiple
>>>> implementations of JShell on any system?
>>>
>>> Maybe not.  But who knows, we already have an external fork..
>>> I suppose we could have experimental versions in incubator modules...
>>> I could also imagine a future with the jshell tool implementation 
>>> broken into a separate module and slimmed-down runtime images built.
>>>
>>>>   Why not just have
>>>>     static JShellToolProvider.instance()
>>>> to return an instance of the one true jshell implementation?
>>>
>>> Certainly an option (depending on the above).  Though the difference 
>>> in interface is:
>>>
>>>     findFirst(String)
>>>     name()
>>>
>>> vs
>>>
>>>     instance()
>>>
>>> So, we get future-proofing almost for free.
>>
>> No changes made here until we come to a fixed-point.
>>
>> Note: an additional option, rather than "instance()" is a static 
>> "builder()" method on the provider since "instance().builder()" is 
>> the only interesting thind you could do with it.   Could add while 
>> leaving or removing the other methods.
>>
>> Thanks,
>> Robert
>>
>>>
>>> The interfaces and implementation classes are separate, but I'd want 
>>> this anyhow, since I very intentionally have them in separate packages.
>>>
>>> -Robert
>>>
>>>
>>>>
>>>> -- Jon
>>>>
>>>>
>>>> On 11/26/2016 02:19 PM, Robert Field wrote:
>>>>> Goldilocks and the three launching mechanisms...
>>>>>
>>>>> She lay down in the first bed, but it was too hard.
>>>>>     java.util.spi.ToolProvider (8169821) has a severe impedance 
>>>>> mismatch: the use of Writers, no InputStream, and no flexibility.
>>>>>
>>>>> Then she lay in the second bed, but it was too soft.
>>>>>     javax.tools.ToolProvider (8170044) would require significant 
>>>>> changes that are not possible at this time, it is limited, and is 
>>>>> old mechanism.
>>>>>
>>>>> Then she lay down in the third bed and it was just right. 
>>>>> Goldilocks fell asleep.
>>>>>     jdk.jshell.tool (8170162) provides a matching, clean, and 
>>>>> configurable launching mechanism
>>>>>
>>>>> Please review...
>>>>>
>>>>> Bug:
>>>>>
>>>>>     8170162: jshell tool: no mechanism to programmatically launch
>>>>>     https://bugs.openjdk.java.net/browse/JDK-8170162
>>>>>
>>>>> Sub-Tasks (tiny tweaks to other repos):
>>>>>
>>>>>     8170194: jshell tool (jdk repo): launch tool from 
>>>>> JShellToolProvider
>>>>>     https://bugs.openjdk.java.net/browse/JDK-8170194
>>>>>
>>>>>     8170195: jshell tool (make): update javadoc generation for 
>>>>> jdk.jshell
>>>>>     https://bugs.openjdk.java.net/browse/JDK-8170195
>>>>>
>>>>> Webrevs:
>>>>>
>>>>>     http://cr.openjdk.java.net/~rfield/8170162v0.webrev/
>>>>>
>>>>>     http://cr.openjdk.java.net/~rfield/8170194v0.webrev/
>>>>>
>>>>>     http://cr.openjdk.java.net/~rfield/8170195v0.webrev/
>>>>>
>>>>> API:
>>>>>
>>>>> http://cr.openjdk.java.net/~rfield/8170162v0.jshellAPI/
>>>>>
>>>>> SpecDiff:
>>>>>
>>>>> http://cr.openjdk.java.net/~rfield/8170162v0.specdiff/overview-summary.html 
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Robert
>>>>>
>>>>
>>>
>>
>