WIP: 8154714: JShell API: addExports support [for Java 9.X]

Mon Sep 5 13:49:39 UTC 2016

Hi Robert,
Thank you for the review.

I totally agree with you.
I also think that availableModules and availablePackages(module) are
worthless excepting for the command completion feature.
I'd like to avoid adding these APIs which have not been speculated and
revised because if API is once added, it is too difficult to remove than
adding.

So currently, I'll remove the completion feature for /set or /retain
exports and make the issue to JBS to add completion feature for them in
future.

I'll update the webrev. So please review it again later.
When the silently evaluation feature is added, I'd like to work for the
completion feature again.

Thank you,
shinyafox(Shinya Yoshida)

2016-09-05 15:00 GMT+09:00 Robert Field <robert.field at oracle.com>:

>
> On 09/03/16 05:07, ShinyaYoshida wrote:
>
> Hi all,
> I've updated my patch, new webrev is here:
> webrev.01.langtools: http://cr.openjdk.java.net/~shinyafox/kulla/8154714/
> webrev.01.langtools/
> webrev.01.jdk: http://cr.openjdk.java.net/~shinyafox/
> kulla/8154714/webrev.01.jdk/ (Same as previous)
>
> In this version,
> - rename /set add-exports to /set exports
>
>
> OK
>
> - add /retain exports <module> <package>
>
>
> The /retain implementation looks good.
>
> - add tests
>
>
> Well tested
>
> - move EXT_CMD_XXX into RemoteCodes
> - make RemoteCodes public and move it into jdk.jshell.execution.code which
> is module-private
>
>
> So, here, in addition to addExports, there are two more methods added to
> the API availableModules() and availablePackage(String module).
> I see how in rare cases they could be useful.  And the rare case that
> causes them to be present is for command completion in the tool.
> What are the options:
>
>   (1) Add them to the API.  If it is in the API then it is a first-class
> form of functionality and should be treated as such in execution support
> (as a command)
>   (2) Not support them and not support command completion for /set
> exports.  Command completion is not supported in much more common cases.
> Then we could support command completion in the future (see (3)).
>   (3) What these commands do is simply execute an expression on the remote
> side.  But we already have a mechanism to execute expressions on the remote
> side: Snippets.
>         That leaves a problem: we wouldn't want these Snippets to be seen
> by the user, but we have a solution for that the tool uses the idGenerator.
>         Another problem is that, by default and with no current way around
> the default, expressions generate temp-variables.  My intention was to make
> this configurable -- see Eval.shouldGenTempVar(...).
>
> One goal I've been using in the design of the API is to keep it as small
> and simple as possible.  This improves maintainability, but much more
> importantly makes the API easier to learn and use.
> So, for something to warrant inclusion it should be generally useful and
> have no other way of being achieved -- neither is true of these two
> queries.
>
> My suggestion, particularly in light of how late in the release this is,
> would be (2).  Thoughts?
>
> -Robert
>
>
>
>
>
>
> Regards,
> shinyafox(Shinya Yoshida)
>
> 2016-09-03 18:15 GMT+09:00 ShinyaYoshida <bitterfoxc at gmail.com>:
>
>> Hi Robert,
>> Thank you for reviewing.
>> I'll update my patch later.
>>
>> 2016-09-02 11:38 GMT+09:00 Robert Field <robert.field at oracle.com>:
>>
>>> Thanks shiyafox! This is a significant piece of code!
>>>
>>> As to your questions --
>>>
>>> > I wonder if  you could work on the documentation of the API and the
>>> tool instead?
>>>
>>> Yes, I will do API and tool docs.
>>
>> Thank you!
>>
>> > Subcommand of /set is reasonable? Similar command, /classpath, to make
>>> users available to use new packages is not a subcommand of /set.
>>>
>>> Yes, I think /set exports is best.  And, yes, /classpath is
>>> inconsistent, it should be /set classpath, that can be a separate task,
>>> unless you want to roll that in.
>>
>> I see, I've created the issue to move /classpath to /set classpath:
>> https://bugs.openjdk.java.net/browse/JDK-8165405
>>
>>
>>> > Should we need retain feature to this?
>>>
>>> All /set have a /retain counterpart -- we should maintain that
>>>
>> Ok, I'll add /retain exports.
>> I've also create the issue for retaining classpath:
>> https://bugs.openjdk.java.net/browse/JDK-8165406
>>
>>
>>> As to the code review: Looks good to me, just minor questions/issues --
>>>
>>> JShellTool.java:
>>>
>>> * As discussed /set add-exports => /set exports
>>>
>>> * Nit: a comment got moved out of place line 1194(old)
>>>
>>> JShell.java:
>>>
>>> * EXT_CMD_AVAILABLE_MODULES and EXT_CMD_AVAILABLE_PACKAGES, these should
>>> be referenced via RemoteCodes.java rather than be coped.
>>>
>> Absolutely! I'd like to move these into RemoteCodes but it is
>> package-private class and cannot be referred from JShell.
>> Can I make RemoteCodes public and move it to another module-private
>> package like jdk.jshell.execution.code?
>>
>>
>>> * Which leads me to another question: why is addExports a command and
>>> these two are extensions.  It seems fine, just curious.  I notice that
>>> extensions are a lot less code -- ;-)
>>
>> Exporting module and package is similar to adding classpath to platform,
>> so I made the addExports a command like setClasspath.
>> And it should be added as API because it is very basic feature.
>>
>>
>>> TaskFactory.java:
>>>
>>> * Question: the "/" separator between module and package is that
>>> platform independent?
>>>
>> It seems to me that "{module}/{path}" format for --add-exports option of
>> javac is platform independent.
>>
>> Regards,
>> shinyafox(Shinya Yoshida)
>>
>>
>>> DefaultLoaderDelegate.java:
>>>
>>> * Jan, can you look at this code?
>>>
>>>
>>> -Robert
>>>
>>>
>>> On 09/01/16 05:36, ShinyaYoshida wrote:
>>>
>>>> Hi all,
>>>> I've implemented this feature to JShell API and Tool with full
>>>> completion:
>>>> Bugs: https://bugs.openjdk.java.net/browse/JDK-8154714
>>>>
>>>> jdk: http://cr.openjdk.java.net/~shinyafox/kulla/8154714/webrev.00.jdk/
>>>> <http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webrev.00.jdk/>
>>>> langtools: http://cr.openjdk.java.net/~shinyafox/kulla/8154714/webrev.0
>>>> 0.langtools/ <http://cr.openjdk.java.net/%7
>>>> Eshinyafox/kulla/8154714/webrev.00.langtools/>
>>>>
>>>> This is missing the train of Java 9.0 but it could catch the train of
>>>> Java 9.1 or later.
>>>> I want to get your comments for this.
>>>>
>>>> jshell> /set add-exports jdk.jconsole sun.tools.jconsole
>>>>
>>>> jshell> import sun.tools.jconsole.*
>>>>
>>>> jshell> LocalVirtualMachine.getAllVirtualMachines().forEach((k, v) ->
>>>> System.out.println(k + ":" + v))
>>>> 10778:jdk.jshell/jdk.internal.jshell.tool.JShellTool
>>>> 10826:jdk.jshell.execution.RemoteExecutionControl 35548
>>>> 28447:org.netbeans.Main --cachedir /home/bitter_fox/.cache/netbeans/8.0.1
>>>> --userdir /home/bitter_fox/.netbeans/8.0.1 --branding nb
>>>>
>>>> Currently, there is no testing, so it will be required to add test.
>>>>
>>>> Regards,
>>>> shinyafox(Shinya Yoshida)
>>>>
>>>
>>>
>>
>
>