Re: RFR: JDK-8085822 JEP 223: New Version-String Scheme (initial integration)

[Alan Bateman]

On 08/06/2015 13:37, Magnus Ihse Bursie wrote:

:
The API functions in Version.java and jvm.h are not finished. The specification 
in the JEP talks about a java.util.Version, that I presume will replace the 
sun.misc.Version, and that will fully implement an API to access the version 
string and all it's parts, according to the JEP definition. Also, the native 
interface will have to be changed to accommodate a version number with an 
arbitrarily number of dot separated parts. These changes will be done later on 
in the verona/stage forest.

Are you ok with addressing these concerns at such a later time?

Sure, esp. since my comments are indeed in the area that isn't complete 
in this webrev.


-Alan

Re: RFR: JDK-8085822 JEP 223: New Version-String Scheme (initial integration)

[Alan Bateman]

On 05/06/2015 15:07, Magnus Ihse Bursie wrote:
This review request covers the main part of the work for JEP-223, the 
new version string format [1]. Basically, we'll call this release Java 
9, instead of Java 1.9.0.


This patch is a folding of all work that has been done so far in the 
branch JEP-223-branch in jdk9/sandbox. As you can see, it mostly 
covers build changes, with some code changes in hotspot, jdk, nashorn 
and langtools that either are corresponding changes in the product 
code due to the compiler define flags changing from the build, or 
follow-up changes to handle the new format.


The JEP-223 work is not finished by this patch. In fact, there are 
known issues remaining even after this patch, typically by code that 
reads the java.version property and tries to parse it. However, this 
patch is not directly destined for jdk9/dev, but will go into the 
special verona/stage forest. As for all patches destined for 
verona/stage it will be code reviewed as if going to jdk9/dev. Once in 
verona/stage it will bide its time, and it will be complemented with 
follow-up patches to address remaining issues. When all such issues 
are resolved and JEP-223 is fully implemented, all changes will be 
pushed at once (without further code reviews) into jdk9/dev.


This patch has been contributed by Magnus Ihse Bursie, Kumar 
Srinivasan and Alejandro Murillo.


Bug: https://bugs.openjdk.java.net/browse/JDK-8085822
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8085822-JEP-223-initial-patch/webrev.01


I looked through the code changes, skipping most of the make files :-)

Version.java.template - the comment in jvmSecurityVersion() still talks 
about 1.6 and newer. Can this be replaced to just say that it returns 
the security version?


Will the update_version and special_update_version fields eventually be 
dropped from the jvm_version_info stricture? Related, there seems to be 
a typo in the comment in jdk_util.c where it has specia_update_version.


The webrev shows a change to this comment in jvm.h:
 Third, this file contains various I/O and network operations needed 
by the standard Java I/O and network APIs.
I think this comment can be removed because those JVM_* functions were 
removed some time ago.


Otherwise looks okay to me.

-Alan.

Re: RFR: JDK-8080679: Include jline in JDK for Java and JavaScript REPLs

[Alan Bateman]

On 25/06/2015 17:25, Jan Lahoda wrote:

Hello,

Based on the feedback I've received so far, I've uploaded an updated 
version of the patch:

http://cr.openjdk.java.net/~jlahoda/8080679/webrev.01/full/

Notable changes:
-avoided the dependency on java.desktop and java.datatransfer
-adjusted the native library build script as per Erik's recommendations
-the module name is now changed to jdk.internal.le

Any comments are welcome!
The new home and the dropping of the dependences on java.desktop  
java.datatransfer look good.


Since there only 2 tests then it might be better to add 
jdk/internal/jline to the mixed bag that is jdk_other. That way you 
don't need to update update jprt.properties and other places where the 
groups are used.


The native method readKeyEvent seems to do a FindClass per key event. 
Maybe this is from the upstream project but I would think it would be 
more efficient to cache this in a global ref. It would also be more 
efficient if the INPUT_RECORD were just returned and avoid the upstream 
to create the object but that might be too much to change when you are 
trying to keep the code in sync with upstream.


-Alan

Re: RFR 8134260: jjs in jre directory fails with Could not find or load main class jdk.nashorn.tools.jjs.Main

[Alan Bateman]

On 25/08/2015 14:24, Sundararajan Athijegannathan wrote:

Hi,

Please review http://cr.openjdk.java.net/~sundar/8134260/ for 
https://bugs.openjdk.java.net/browse/JDK-8134260


jdk.nashorn.tools.jjs and jdk.internal.le should exist in jre's 
appmodules.jimage with this change and so jjs would work in JRE. Also, 
jjs launcher tool was created from wrong makefile - correct module 
name is used now.

This looks okay to me.

-Alan

Re: Review request for JDK-8135251: Use Unsafe.defineAnonymousClass for loading Nashorn script code

[Alan Bateman]

On 16/09/2015 14:04, Magnus Ihse Bursie wrote:


Does this mean that updates to modules.xml between now and the 
integration-to-come of jigsaw/jake do not anymore need specific approval?
I think we should keep the status quo and CC jigsaw-dev when there are 
changes to modules.xml. If nothing else, it creates aware of the changes 
because they require adjustment when jigsaw/jake is sync'ed up from 
jdk9/jdk9. These sync ups are non-trivial at times. Aside from merging 
almost every repo, then often require looking at the changes to 
modules.xml in the promoted build to ensure that the equivalent changes 
are make to the module declarations.


-Alan

Re: Review request for JDK-8135251: Use Unsafe.defineAnonymousClass for loading Nashorn script code

[Alan Bateman]

On 16/09/2015 14:06, David M. Lloyd wrote:


Also I could be wrong but it looks to me like the jigsaw/jake forest's 
equivalent module graph has evolved somewhat, and thus doesn't quite 
match this file anyway.
jigsaw/jake is currently at jdk9-b81. There may be changes backed up in 
jdk9/dev that aren't in jake yet, we'll get those once those changes get 
to master and there is a sync up. As I mentioned in the reply to Magnus 
then these sync ups often require changes to the module declarations.


Other than the pipeline there is one new module in jigsaw/jake 
(jdk.jlink), a few additional exports, and a few adjustments to 
qualified exports to take account of the wide changes in the jake 
forest. The nice thing about working in jake is that it will catch a lot 
more at compile time compared to working in jdk9/dev.


-Alan.

Re: RFR 8145750: jjs fails to run simple scripts with security manager turned on

[Alan Bateman]

On 18/12/2015 12:23, Sundararajan Athijegannathan wrote:
Please review http://cr.openjdk.java.net/~sundar/8145750/webrev.00/ 
for https://bugs.openjdk.java.net/browse/JDK-8145750


Adding missing permissions for jdk.dynalink module. Note that it used 
to be part of jdk.scripting.nashorn module in the past and therefore 
got AllPermission.
Is it really necessary to grant it AllPermission? Just wondering how 
hard it would be to figure out the permissions that it really needs.


If test/tools/jjs is new then it would be good to this test directory 
into one of the test groups so that the tests are run, maybe jdk_other.


-Alan

8049422: Remove @jdk.Exported

[Alan Bateman]

It's time to remove @jdk.Exported.

As background, this annotation was introduced by JEP 179 in JDK 8 to 
document the supportness of JDK-specific APIs. It will become redundant 
and confusing once we bring the module system into JDK 9. The proposal 
in JEP 261 is to remove it and it seems saner to remove this in jdk9/dev 
in advance rather than trying to carry this in jigsaw/jake.


The webrevs (top, jdk, langtools and nashorn repos) are here:
  http://cr.openjdk.java.net/~alanb/8049422/

The changes are trivial with all usages removed with
   find src -name "*.java" -exec sed -i '/@jdk.Exported/d' {} \;

Skimming the .patch files should be enough to check for any blunders.

The only "real" changes are the hg rm of Exported.java in the jdk repo, 
and the updates to modules.xml and the package list for the javadoc 
build in the top-level repo


Note that jdeps has already been changed to not use this annotation so 
there are no jdeps changes required, only residual comments in one test.


I think I got everything. Joe Darcy created this annotation and knows 
this removal is coming, I'll at least wait for Joe to review before 
pushing this change.


-Alan.

Re: RFR 8145750: jjs fails to run simple scripts with security manager turned on

[Alan Bateman]

On 18/12/2015 12:55, Sundararajan Athijegannathan wrote:


May be, not. But I tried giving only sun.reflect package access -- 
didn't work. There are a few doPrivileged blocks in dynalink code as 
well. This needs further analysis.
But dynalink code was part of nashorn and so was getting AllPermission 
so far - so there is no permission enhancement by adding this missing 
permission block.


That said, we can revisit reduced permission set for dynalink module. 
I'd prefer to track that as separate bug.

Sure, a separate bug is fine.




If test/tools/jjs is new then it would be good to this test directory 
into one of the test groups so that the tests are run, maybe jdk_other.


okay, I'll find out that.
Looks looks core_tools will execute tools/** so I think what you have 
here is okay.


-Alan.

Re: Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

[Alan Bateman]

On 23/11/2015 15:43, Sundararajan Athijegannathan wrote:
But, in addition to providing service, jdk.scripting.nashorn module 
also "exports" nashorn specific APIs in jdk.nashorn.api.* packages.

Sure, it could go in either but we mostly treat it as a service provider.

-Alan.

Re: Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

[Alan Bateman]

On 23/11/2015 15:27, Attila Szegedi wrote:

Folks,

I integrated the changes Mandy suggested; please review these (build related) 
changes:
jdk9 top level: 

jdk9/jdk: 


For the sake of completeness, the jdk/nashorn changes are here: 
 but they have already 
been reviewed by Hannes and Sundar; only the above two (jdk9 and jdk9/jdk) have been 
modified compared to the original review request.

Sundar was kind enough to verify that JDK9 builds fine with these changes.


The jdk repo looks okay (just had to change your link to find it :-)

In the top-level repo then you've moved jdk.scripting.nashorn from 
PROVIDER_MODULES to MAIN_MODULES. The reason that we've had it in 
PROVIDER_MODULES is because we treat it as a service provider module (it 
provides an implementation of javax.script.ScriptEngineFactory).


-Alan.

Re: Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

[Alan Bateman]

On 23/11/2015 16:07, Attila Szegedi wrote:

:
Whichever is the stronger criteria for deciding whether to place it in MAIN or 
PROVIDER is fine with me. Intuitively “provider” seems like a weaker category 
(exposes a service provider but doesn’t have its own API), so (without knowing 
the particulars of the use of these *_MODULES variables) it seems to me Mandy’s 
suggestion is correct to reclassify Nashorn into a MAIN module.

We need to do a bit of clean-up in Images.gmk to make things clearer as 
this MAIN vs. PROVIDER topic has caused confusion on a few cases. If we 
can keep the lists separate to the list of modules for the compact 
profile builds then there is no reason why they can't be combined as 
JRE_MODULES.


In this case then jdk.scripting.nashorn.shell is already listed in 
MAIN_MODULES so this will ensure than Nashorn is linked into any 
run-time image that has the the jjs tool/shell. It doesn't matter if 
jdk.scripting.nashorn is listed or not.


-Alan.

Re: Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

[Alan Bateman]

On 19/11/2015 23:15, Attila Szegedi wrote:

Please review JDK-8141338 "Move jdk.internal.dynalink package to jdk.dynalink" for 
. This is basically the implementation 
step for integrating JEP 276. This changeset will introduce a new public API that has CCC 
approval (request 8075866), and is also the implementation step of JEP 276 which is now 
targeted for 9 and thus can be integrated.

The changes in this changeset fall into several categories:
- renaming of jdk.internal.dynalink.* package to jdk.dynalink.* package, with 
ripple effects in Nashorn classes that import from these packages
- changes to modules.xml and some build files to accommodate a new public 
module and a dependency of Nashorn on it
- new tests

  I’m sending this webrev to several lists

Probably build-dev instead of jdk9-dev.

I'm curious if it's strictly necessary for module jdk.dynalink to be in 
the nashorn repo now, I assume not but it's probably convenient when 
working Nashorn.


In any case, the module name and the changes to modules.xml look okay to 
me. As Mandy noted, this isn't a service provider API so in Images.gmk 
then you can add it to MAIN_MODULES rather than PROVIDER_MODULES.


Your webrevs don't have the changes to the jdk repo but I assume that 
make/src/classes/build/tools/module/ext.modules has been updated to list 
jdk.dynalink. That is, I assume it needs to be defined to the ext loader 
because jdk.scripting.nashorn uses it. The ext.modules file is temporary 
and goes away when we bring in the module system.


-Alan.

Re: RFR 8158131: Nashorn should not use jdk.internal.module.Modules API

[Alan Bateman]

On 30/05/2016 11:43, Sundararajan Athijegannathan wrote:

Updated nashorn webrev:
http://cr.openjdk.java.net/~sundar/8158131/nashorn/webrev.01/

* Added @link in ModuleGraphManipulator.java's javadoc

* Using Context.class.getModule() in all places where nashorn module is
needed

* Using  all caps names for static final variables  - nashornModule,
javaBaseModule, myModule  in generated


I looked over the changes and good to see it using the API to create 
dynamic modules. I assume folks familiar with the Nashorn code will do 
the detailed review.


-Alan

Re: RFR 8152268: jjs tool makefile should use --addmods ALL-SYSTEM

[Alan Bateman]

On 23/03/2016 08:59, Sundararajan Athijegannathan wrote:

Hi,

Please review http://cr.openjdk.java.net/~sundar/8152268/webrev.00/ for
https://bugs.openjdk.java.net/browse/JDK-8152268


This looks okay, similar to what we had to do with appletviewer.

-Alan.

Re: RFR 8154192: Deprivilege java.scripting module

[Alan Bateman]

On 18/05/2016 08:55, Sundararajan Athijegannathan wrote:

Please review the updated webrevs.

* Fixed Modules.gmk for order of modules:

http://cr.openjdk.java.net/~sundar/8154192/top/webrev.01/

* From quick reading of j.u.ServiceLoader: AccessControlContext is
captured in ServiceLoader constructor & used for iteration
(RestrictedIterator). So, ScriptEngineManager calling only ServiceLoader
constructor inside doPrivileged block seems fine. Also, I turned
ProviderTest javax.script API test to use security manager - this tests
loads a dummy script engine from a jar file in classpath. This test
passes with security manager on.

http://cr.openjdk.java.net/~sundar/8154192/jdk/webrev.01/

Yes, we can still revisit AllPermission for java.scripting module in a
future change.

One suggestion is to run ProviderTest twice, both with and without SM. 
The rest looks okay.


-Alan.

Re: RFR 8165595: Main class should be set for nashorn modules

[Alan Bateman]

On 07/09/2016 13:10, Sundararajan Athijegannathan wrote:


Please review http://cr.openjdk.java.net/~sundar/8165595/webrev.01/ for
https://bugs.openjdk.java.net/browse/JDK-8165595

I'm not sure that asking users to use -m is right here, esp. when the 
modules contains launchers. Also CreateJmods.gmk may not be the right 
place for this, it feels more like something that should be configured 
in a make file that is specific to the module.



-Alan

Re: RFR 8165595: Main class should be set for nashorn modules

[Alan Bateman]

On 07/09/2016 13:27, Sundararajan Athijegannathan wrote:


jjs does not yet support module related options. So, user modules can
not be scripted directly with jjs as of now. Only way is to use to
launcher with -mp along with -m for jjs main class. With the change,
only module name needs to be specified.

Also, jjs tool is not shipped for embedded platforms (compact1). But,
jdk.nashorn.tools.Shell (used to be the jjs main in jdk8u) is compact1
compliant. So user can use

 java -m jdk.scripting.nashorn

on embedded platforms [and use functionality reduced jjs there]

Yes, I tried to see if I can get it per-module config from somewhere -
but couldn't. I'm open to suggestions on how to do that in the current
scheme.

Is there an issue tracking the update to `jjs`?

Also I think you should wait to hear from Erik as to where to put this 
in the build. The concern with CreateJMods.gmk is that isn't not going 
to scale once we eventually get to sorting out the issues with modules 
that have entry points.


-Alan

Re: RFR 8071588: The spec for javax.script.ScriptEngineFactory.getProgram() should specify NPEs thrown

[Alan Bateman]

On 19/10/2016 06:06, Sundararajan Athijegannathan wrote:


Please review.

Bug: https://bugs.openjdk.java.net/browse/JDK-8071588

jdk webrev: http://cr.openjdk.java.net/~sundar/8071588/jdk/webrev.00/

nashorn webrev:
http://cr.openjdk.java.net/~sundar/8071588/nashorn/webrev.00/

This looks okay (except the spurious blank line after the class 
comment). It's a spec change that will need to be tracked.


-Alan

Re: RFR 8170099: Nashorn test failures with stricter reflection access checks in jake forest

[Alan Bateman]

On 21/11/2016 14:14, Sundararajan Athijegannathan wrote:
Please review http://cr.openjdk.java.net/~sundar/8170099/webrev.00/ 
for https://bugs.openjdk.java.net/browse/JDK-8170099
This looks okay although you could replace the setAccessible methods in 
Reflector that take Method and Constructor with one that takes an 
Executable.


-Alan

Re: RFR 8167614: Avoid module dependency from jdk.dynalink to jdk.internal.module of java.base module

[Alan Bateman]

On 12/10/2016 16:11, Sundararajan Athijegannathan wrote:


Bug: https://bugs.openjdk.java.net/browse/JDK-8167614

jdk webrev: http://cr.openjdk.java.net/~sundar/8167614/jdk/webrev.00/

nashorn webrev:
http://cr.openjdk.java.net/~sundar/8167614/nashorn/webrev.00/

In jdk/nashorn/internal/runtime/Context.java then it checks if the 
java.sql and java.sql.rowset are observable. This does not mean they are 
in the boot layer, I think you want 
Layer.boot().findModule("java.sql").ifPresent(...). However, the this 
code in Nashorn makes me wonder if this is actually needed or why these 
two are special cased.


-Alan

Re: RFR 8167614: Avoid module dependency from jdk.dynalink to jdk.internal.module of java.base module

[Alan Bateman]

On 12/10/2016 17:31, Sundararajan Athijegannathan wrote:


Updated nashorn webrev:
http://cr.openjdk.java.net/~sundar/8167614/nashorn/webrev.01/

Changed to use Layer.boot().findModule.

That loos okay to me. I assume that once we fix the issues in java.sql 
that this code can be removed.


-Alan

Re: RFR 8193779: Fix copyright header in nashorn builtin scripts

[Alan Bateman]

On 19/12/2017 10:38, Sundararajan Athijegannathan wrote:

Please review.

Bug: https://bugs.openjdk.java.net/browse/JDK-8193799
Webrev: http://cr.openjdk.java.net/~sundar/8193799/webrev.00/

Looks fine. Will you push this to jdk/jdk10?

-Alan

Re: RFR: JDK-8203827: Upgrade JLine to 2.14.6

[Alan Bateman]

On 30/05/2018 16:06, Jan Lahoda wrote:

Hi,

An updated webrev is here:
http://cr.openjdk.java.net/~jlahoda/8203827/webrev.01/complete/

A webrev showing changes from the previous revision is here:
http://cr.openjdk.java.net/~jlahoda/8203827/webrev.01/delta/

The changes are:
-updated src/jdk.internal.le/share/legal/jline.md
-the problem with automatically adding space after completion is resolved
-a few tweaks to tests

Does this look good?
I see Hannes has replied and I assume you've tested jshell so I think 
this is upgrade is good to go.


-Alan.

Re: RFR: JDK-8203827: Upgrade JLine to 2.14.6

[Alan Bateman]

On 25/05/2018 21:20, Jan Lahoda wrote:

Hi,

I'd like to upgrade the JLine used by JShell and jjs from 2.12.1 to 
2.14.6.


The complete webrev is here:
http://cr.openjdk.java.net/~jlahoda/8203827/webrev.00/complete/

To simplify reviewing, there is:
-an antipatch that removes the JDK-specific changes and restores the 
vanilla 2.12.1 content:

http://cr.openjdk.java.net/~jlahoda/8203827/webrev.00/undo-jdk-extras/
-a patch that replaces the 2.12.1 content with 2.14.6:
http://cr.openjdk.java.net/~jlahoda/8203827/webrev.00/upgrade-jline/
-a patch that re-applies the JDK-specific changes (like including 
adjusting packages, and removal/commenting out of usage of features 
that would require undesirable dependencies, and any changes that had 
to be done to other modules):

http://cr.openjdk.java.net/~jlahoda/8203827/webrev.00/adding-jdk-extras/

JBS entry: https://bugs.openjdk.java.net/browse/JDK-8203827

How does this look?
The refresh looks okay and I see the package name has been adjusted for 
the new files.


Is there an update to src/jdk.internal.le/share/legal/jlink.md? 
Minimally I would assume the version at the top of the file needs to be 
updated.


-Alan