RFR 8170162: jshell tool: no mechanism to programmatically launch

Fri Dec 9 03:45:57 UTC 2016

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
>>>>
>>>
>>
>