Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module [v7]

[David Alayachew]

On Wed, 8 May 2024 08:30:39 GMT, Raffaello Giulietti  
wrote:

>> Move all random generators mandated in package `java.util.random` and 
>> currently implemented in module `jdk.random` to module `java.base`, and 
>> remove module `jdk.random`.
>
> Raffaello Giulietti has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Removed empty line.

oh lol, makes sense then.

It's wild though -- I always thought that deleting an entire module would
count as a backwards incompatible thing.


On Wed, Jun 5, 2024 at 1:00 AM Alan Bateman ***@***.***>
wrote:

> Hey, I'm just curious -- why was the solution to remove an entire module?
> I understand the point of moving the relevant code over to java.base, but
> I don't understand why removing the random module made sense.
>
> With the implementations moved, the jdk.random module didn't contain any
> classes. A placeholder module could have been left in place for this using
> jlink that might have a script that adds this module but it didn't seem
> worth it to do this for a release or two. It was a different answer for
> jdk.cryto.ec before it was removed. That module existed since JDK 9 so
> more likely be used in jlink scripts.
>
> —
> Reply to this email directly, view it on GitHub
> , or
> unsubscribe
> 
> .
> You are receiving this because you commented.Message ID:
> ***@***.***>
>

-

PR Comment: https://git.openjdk.org/jdk/pull/18932#issuecomment-2148883348

Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module [v7]

[Alan Bateman]

On Wed, 5 Jun 2024 04:41:48 GMT, David Alayachew  wrote:

> Hey, I'm just curious -- why was the solution to remove an entire module? I 
> understand the point of moving the relevant code over to `java.base`, but I 
> don't understand why removing the random module made sense.

With the implementations moved, the jdk.random module didn't contain any 
classes. A placeholder module could have been left in place for this using 
jlink that might have a script that adds this module but it didn't seem worth 
it to do this for a release or two. It was a different answer for jdk.cryto.ec 
before it was removed. That module existed since JDK 9 so more likely be used 
in jlink scripts.

-

PR Comment: https://git.openjdk.org/jdk/pull/18932#issuecomment-2148858435

Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module [v7]

[David Alayachew]

On Wed, 8 May 2024 08:30:39 GMT, Raffaello Giulietti  
wrote:

>> Move all random generators mandated in package `java.util.random` and 
>> currently implemented in module `jdk.random` to module `java.base`, and 
>> remove module `jdk.random`.
>
> Raffaello Giulietti has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Removed empty line.

Hey, I'm just curious -- why was the solution to remove an entire module? I 
understand the point of moving the relevant code over to `java.base`, but I 
don't understand why removing the random module made sense.

-

PR Comment: https://git.openjdk.org/jdk/pull/18932#issuecomment-2148842095

Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module [v7]

[Raffaello Giulietti]

> Move all random generators mandated in package `java.util.random` and 
> currently implemented in module `jdk.random` to module `java.base`, and 
> remove module `jdk.random`.

Raffaello Giulietti has updated the pull request incrementally with one 
additional commit since the last revision:

  Removed empty line.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18932/files
  - new: https://git.openjdk.org/jdk/pull/18932/files/8a5598ea..d2bdf337

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=18932&range=06
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18932&range=05-06

  Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod
  Patch: https://git.openjdk.org/jdk/pull/18932.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18932/head:pull/18932

PR: https://git.openjdk.org/jdk/pull/18932

Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module [v6]

[Jaikiran Pai]

On Sat, 4 May 2024 18:29:25 GMT, Raffaello Giulietti  
wrote:

>> Move all random generators mandated in package `java.util.random` and 
>> currently implemented in module `jdk.random` to module `java.base`, and 
>> remove module `jdk.random`.
>
> Raffaello Giulietti has updated the pull request with a new target base due 
> to a merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains seven additional 
> commits since the last revision:
> 
>  - Merge branch 'master' into 8330005
>  - Restrict RandomGenerator service providers to those loadable by the 
> platform class loader.
>  - Typo.
>  - Added @uses javadoc tag for j.u.r.RandomGenerator in java.base.
>  - Terminology changes.
>  - Renamed package jdk.random to jdk.internal.random.
>  - 8330005: RandomGeneratorFactory.getDefault() throws exception when the 
> runtime image only has java.base module

Hello Alan,

> RandomTestCoverage should be testing these APIs but if there are gaps then it 
> would be good to add more tests (separate PR of course)

I missed that test case. It does appear to cover most of these APIs on 
`RandomGeneratorFactory`. There's a method in that test which even calls 
`RandomGeneratorFactory.getDefault()` but that doesn't seem to be exercised.

-

Marked as reviewed by jpai (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18932#pullrequestreview-2044738539

Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module [v6]

[Alan Bateman]

On Wed, 8 May 2024 05:29:03 GMT, Jaikiran Pai  wrote:

> Hello Raffaello, the proposed changes look OK to me. Do you think we should 
> add a jtreg test for this?
> 
> In general, the test coverage for these APIs appears to be missing. I think 
> that can be addressed separately in some of the upcoming changes.

RandomTestCoverage should be testing these APIs but if there are gaps then it 
would be good to add more tests (separate PR of course).  There is a second 
update coming once this PR is integrated, that will change the implementation 
to maybe not use SL.

-

PR Comment: https://git.openjdk.org/jdk/pull/18932#issuecomment-2099815847

Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module [v6]

[Alan Bateman]

On Sat, 4 May 2024 18:29:25 GMT, Raffaello Giulietti  
wrote:

>> Move all random generators mandated in package `java.util.random` and 
>> currently implemented in module `jdk.random` to module `java.base`, and 
>> remove module `jdk.random`.
>
> Raffaello Giulietti has updated the pull request with a new target base due 
> to a merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains seven additional 
> commits since the last revision:
> 
>  - Merge branch 'master' into 8330005
>  - Restrict RandomGenerator service providers to those loadable by the 
> platform class loader.
>  - Typo.
>  - Added @uses javadoc tag for j.u.r.RandomGenerator in java.base.
>  - Terminology changes.
>  - Renamed package jdk.random to jdk.internal.random.
>  - 8330005: RandomGeneratorFactory.getDefault() throws exception when the 
> runtime image only has java.base module

Marked as reviewed by alanb (Reviewer).

src/java.base/share/classes/module-info.java line 426:

> 424: java.util.Random,
> 425: java.util.SplittableRandom,
> 426: 

Can you remove this blank files, otherwise the list of service types for this 
provide declaration are all together?

-

PR Review: https://git.openjdk.org/jdk/pull/18932#pullrequestreview-2044707607
PR Review Comment: https://git.openjdk.org/jdk/pull/18932#discussion_r1593435059

Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module [v6]

[Jaikiran Pai]

On Sat, 4 May 2024 18:29:25 GMT, Raffaello Giulietti  
wrote:

>> Move all random generators mandated in package `java.util.random` and 
>> currently implemented in module `jdk.random` to module `java.base`, and 
>> remove module `jdk.random`.
>
> Raffaello Giulietti has updated the pull request with a new target base due 
> to a merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains seven additional 
> commits since the last revision:
> 
>  - Merge branch 'master' into 8330005
>  - Restrict RandomGenerator service providers to those loadable by the 
> platform class loader.
>  - Typo.
>  - Added @uses javadoc tag for j.u.r.RandomGenerator in java.base.
>  - Terminology changes.
>  - Renamed package jdk.random to jdk.internal.random.
>  - 8330005: RandomGeneratorFactory.getDefault() throws exception when the 
> runtime image only has java.base module

Hello Raffaello, the proposed changes look OK to me. Do you think we should add 
a jtreg test for this?

In general, the test coverage for these APIs appears to be missing. I think 
that can be addressed separately in some of the upcoming changes.

-

PR Review: https://git.openjdk.org/jdk/pull/18932#pullrequestreview-2044656083

Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module [v6]

[Alan Bateman]

On Sun, 5 May 2024 12:05:48 GMT, Raffaello Giulietti  
wrote:

>> src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java 
>> line 147:
>> 
>>> 145: 
>>> FactoryMapHolder.class.getModule().addUses(RandomGenerator.class);
>>> 146: return ServiceLoader
>>> 147: .load(RandomGenerator.class, 
>>> ClassLoader.getPlatformClassLoader())
>> 
>> SecurityManager is still a supported execution mode so you'll need to get 
>> the platform class loader in a privileged block until the SM feature is 
>> removed.
>
> Yes, I considered the interactions with a security manager.
> 
> But here the call to `getPlatformClassLoader()` is done from a platform 
> class, namely `FactoryMapHolder` itself. According to its documentation, the 
> call succeeds in this case because the security manager is not even consulted.
> 
> When experimenting with the following code and the default manager, as with 
> `-Djava.security.manager=default`, no exceptions are thrown, neither with the 
> full JDK nor with the minimal image that just includes `java.base`. There's 
> only a warning about future removal of `SecurityManager`, as expected from 
> JEP 411.
> 
> 
> import java.util.random.*;
> 
> public class Foo {
> public static void main(final String[] args) throws Exception {
> RandomGeneratorFactory.all().forEach(g -> 
> System.out.println(g.name()));
> final RandomGeneratorFactory rgf = 
> RandomGeneratorFactory.getDefault();
> System.out.println("Got " + rgf);
> }
> }
> 
> 
> But if the call to `getPlatformClassLoader()` is done directly from an app 
> loaded by the system class loader, then an exception is thrown when the 
> default security manager is active.

Thanks, I'd forgotten the check in that method is caller sensitive.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18932#discussion_r1590318688

Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module [v6]

[Raffaello Giulietti]

On Sun, 5 May 2024 05:28:07 GMT, Alan Bateman  wrote:

>> Raffaello Giulietti has updated the pull request with a new target base due 
>> to a merge or a rebase. The incremental webrev excludes the unrelated 
>> changes brought in by the merge/rebase. The pull request contains seven 
>> additional commits since the last revision:
>> 
>>  - Merge branch 'master' into 8330005
>>  - Restrict RandomGenerator service providers to those loadable by the 
>> platform class loader.
>>  - Typo.
>>  - Added @uses javadoc tag for j.u.r.RandomGenerator in java.base.
>>  - Terminology changes.
>>  - Renamed package jdk.random to jdk.internal.random.
>>  - 8330005: RandomGeneratorFactory.getDefault() throws exception when the 
>> runtime image only has java.base module
>
> src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line 
> 147:
> 
>> 145: 
>> FactoryMapHolder.class.getModule().addUses(RandomGenerator.class);
>> 146: return ServiceLoader
>> 147: .load(RandomGenerator.class, 
>> ClassLoader.getPlatformClassLoader())
> 
> SecurityManager is still a supported execution mode so you'll need to get the 
> platform class loader in a privileged block until the SM feature is removed.

Yes, I considered the interactions with a security manager.

But here the call to `getPlatformClassLoader()` is done from a platform class, 
namely `FactoryMapHolder` itself. According to its documentation, the call 
succeeds in this case because the security manager is not even consulted.

When experimenting with the following code and the default manager, as with 
`-Djava.security.manager=default`, no exceptions are thrown, neither with the 
full JDK nor with the minimal image that just includes `java.base`. There's 
only a warning about future removal of `SecurityManager`, as expected from JEP 
411.


import java.util.random.*;

public class Foo {
public static void main(final String[] args) throws Exception {
RandomGeneratorFactory.all().forEach(g -> System.out.println(g.name()));
final RandomGeneratorFactory rgf = 
RandomGeneratorFactory.getDefault();
System.out.println("Got " + rgf);
}
}


But if the call to `getPlatformClassLoader()` is done directly from an app 
loaded by the system class loader, then an exception is thrown when the default 
security manager is active.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18932#discussion_r1590295056

Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module [v6]

[Alan Bateman]

On Sat, 4 May 2024 18:29:25 GMT, Raffaello Giulietti  
wrote:

>> Move all random generators mandated in package `java.util.random` and 
>> currently implemented in module `jdk.random` to module `java.base`, and 
>> remove module `jdk.random`.
>
> Raffaello Giulietti has updated the pull request with a new target base due 
> to a merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains seven additional 
> commits since the last revision:
> 
>  - Merge branch 'master' into 8330005
>  - Restrict RandomGenerator service providers to those loadable by the 
> platform class loader.
>  - Typo.
>  - Added @uses javadoc tag for j.u.r.RandomGenerator in java.base.
>  - Terminology changes.
>  - Renamed package jdk.random to jdk.internal.random.
>  - 8330005: RandomGeneratorFactory.getDefault() throws exception when the 
> runtime image only has java.base module

src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line 
147:

> 145: 
> FactoryMapHolder.class.getModule().addUses(RandomGenerator.class);
> 146: return ServiceLoader
> 147: .load(RandomGenerator.class, 
> ClassLoader.getPlatformClassLoader())

SecurityManager is still a supported execution mode so you'll need to get the 
platform class loader in a privileged block until the SM feature is removed.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18932#discussion_r1590212532

Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module [v6]

[Raffaello Giulietti]

> Move all random generators mandated in package `java.util.random` and 
> currently implemented in module `jdk.random` to module `java.base`, and 
> remove module `jdk.random`.

Raffaello Giulietti has updated the pull request with a new target base due to 
a merge or a rebase. The incremental webrev excludes the unrelated changes 
brought in by the merge/rebase. The pull request contains seven additional 
commits since the last revision:

 - Merge branch 'master' into 8330005
 - Restrict RandomGenerator service providers to those loadable by the platform 
class loader.
 - Typo.
 - Added @uses javadoc tag for j.u.r.RandomGenerator in java.base.
 - Terminology changes.
 - Renamed package jdk.random to jdk.internal.random.
 - 8330005: RandomGeneratorFactory.getDefault() throws exception when the 
runtime image only has java.base module

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18932/files
  - new: https://git.openjdk.org/jdk/pull/18932/files/59c86b43..8a5598ea

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=18932&range=05
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18932&range=04-05

  Stats: 22890 lines in 1641 files changed: 7834 ins; 9521 del; 5535 mod
  Patch: https://git.openjdk.org/jdk/pull/18932.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18932/head:pull/18932

PR: https://git.openjdk.org/jdk/pull/18932

Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module [v5]

[Raffaello Giulietti]

On Tue, 30 Apr 2024 10:21:49 GMT, Raffaello Giulietti  
wrote:

>> Move all random generators mandated in package `java.util.random` and 
>> currently implemented in module `jdk.random` to module `java.base`, and 
>> remove module `jdk.random`.
>
> Raffaello Giulietti has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Typo.

This PR's goal is to fix one specific problem: being able to link a minimal 
image containing only `java.base` while still having all _mandated_ algorithms 
available at runtime.
The fix does not improve on the discoverability and participation of other 
random generator algorithms, but it minimally contributes to solve the linking 
issue.
Thus, I think the proposed fix, apart from possible minor details, has a value 
on its own.

Work on evolving the _design aspects_ to allow additional algorithms to 
participate in service discovery should be done in a future PR.

-

PR Comment: https://git.openjdk.org/jdk/pull/18932#issuecomment-2090383759

Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module [v5]

[Alan Bateman]

On Tue, 30 Apr 2024 10:21:49 GMT, Raffaello Giulietti  
wrote:

>> Move all random generators mandated in package `java.util.random` and 
>> currently implemented in module `jdk.random` to module `java.base`, and 
>> remove module `jdk.random`.
>
> Raffaello Giulietti has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Typo.

I assume this PR should be returned to Draft until the complete set of changes 
is ready.

-

PR Comment: https://git.openjdk.org/jdk/pull/18932#issuecomment-2090344307

Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module [v5]

[Raffaello Giulietti]

> Move all random generators mandated in package `java.util.random` and 
> currently implemented in module `jdk.random` to module `java.base`, and 
> remove module `jdk.random`.

Raffaello Giulietti has updated the pull request incrementally with one 
additional commit since the last revision:

  Typo.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18932/files
  - new: https://git.openjdk.org/jdk/pull/18932/files/d502b245..59c86b43

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=18932&range=04
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18932&range=03-04

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/18932.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18932/head:pull/18932

PR: https://git.openjdk.org/jdk/pull/18932

Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module [v4]

[Hannes Greule]

On Fri, 26 Apr 2024 15:24:30 GMT, Raffaello Giulietti  
wrote:

>> Move all random generators mandated in package `java.util.random` and 
>> currently implemented in module `jdk.random` to module `java.base`, and 
>> remove module `jdk.random`.
>
> Raffaello Giulietti has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Added @uses javadoc tag for j.u.r.RandomGenerator in java.base.

Note that this also fixes a "bug" when calling `RandomGenerator.getDefault()` 
(or similar) from an unnamed module that does not have the application class 
loader as a(n indirect) parent. That previously resulted in the service loader 
only finding `Random`, `SecureRandom`, and `SplittableRandom`. As those are 
cached 
(https://github.com/openjdk/jdk/blob/fd84a28ba0aa82b2f48f247c3ec7e13acdcd6ff1/src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java#L137),
 even calls from somewhere else afterwards wouldn't find the correct 
implementation anymore. So I'm welcoming this change.

-

PR Comment: https://git.openjdk.org/jdk/pull/18932#issuecomment-2080464407

Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module [v3]

[Jaikiran Pai]

On Fri, 26 Apr 2024 13:45:39 GMT, Alan Bateman  wrote:

> How useful is it to deploy with additional RandomGenerator implementations on 
> the class path or module path?

It doesn't look like in its current form it's possible (without an 
`--add-exports`) to have additional `java.util.random.RandomGenerator` 
implementations in the class path (or module path). I tried some experiment 
today to introduce a trivial `java.util.random.RandomGenerator` implementation 
as follows:


package foo;

import java.util.Random;
import java.util.random.RandomGenerator;

public class DummyRandomGenerator implements RandomGenerator {

private final Random random = new Random();

@Override
public long nextLong() {
System.out.println("nextLong from " + this.getClass());
return random.nextLong();
}
}

Package this `DummyRandomGenerator` into a jar file and in that jar file 
include a `META-INF/services/java.util.RandomGenerator` which points to the 
`foo.DummyRandomGenerator` class. Then launch a main application with this jar 
in the classpath:


import java.util.random.RandomGeneratorFactory;

public class Main {
public static void main(String[] args) {
System.out.println("available RandomGenerator(s):");

RandomGeneratorFactory.all().map(RandomGeneratorFactory::name).forEach(System.out::println);
}
}

Running this `Main` application with that jar in the classpath doesn't list the 
`DummyRandomGenerator`. It appears that such application/library specific 
implementations of `java.util.random.RandomGenerator` are expected to be 
annotated to an annotation that belongs to the internals of the java.base 
module 
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java#L391.

Changing the `DummyRandomGenerator` to have it annotated with:


@jdk.internal.util.random.RandomSupport.RandomGeneratorProperties(name="Dummy")
public class DummyRandomGenerator implements RandomGenerator {
...

and then compiling that application class with:


javac --add-exports java.base/jdk.internal.util.random=ALL-UNNAMED ...

and then re-packaging that jar with this class and rerunning the Main with that 
jar in the classpath will this time return the `DummyRandomGenerator` from a 
call to `RandomGeneratorFactory.all()`.

The API docs of `java.util.random.RandomGeneratorFactory` don't make a mention 
of the necessity of this internal 
`jdk.internal.util.random.RandomSupport.RandomGeneratorProperties` annotation.

-

PR Comment: https://git.openjdk.org/jdk/pull/18932#issuecomment-208042

Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module [v4]

[Raffaello Giulietti]

> Move all random generators mandated in package `java.util.random` and 
> currently implemented in module `jdk.random` to module `java.base`, and 
> remove module `jdk.random`.

Raffaello Giulietti has updated the pull request incrementally with one 
additional commit since the last revision:

  Added @uses javadoc tag for j.u.r.RandomGenerator in java.base.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18932/files
  - new: https://git.openjdk.org/jdk/pull/18932/files/fd84a28b..d502b245

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=18932&range=03
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18932&range=02-03

  Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod
  Patch: https://git.openjdk.org/jdk/pull/18932.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18932/head:pull/18932

PR: https://git.openjdk.org/jdk/pull/18932

Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module [v3]

[Raffaello Giulietti]

On Fri, 26 Apr 2024 09:41:21 GMT, Raffaello Giulietti  
wrote:

>> Move all random generators mandated in package `java.util.random` and 
>> currently implemented in module `jdk.random` to module `java.base`, and 
>> remove module `jdk.random`.
>
> Raffaello Giulietti has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Terminology changes.

I don't think that there's any intent

> to deploy with additional RandomGeneratorFactory implementations

as this is a final class.

The service provider interface `RandomGenerator` is exposed in the module by 
means of a `uses` clause. For some reason, however, it wasn't documented by a 
`@uses` javadoc tag. Added in last commit.

-

PR Comment: https://git.openjdk.org/jdk/pull/18932#issuecomment-2079603835

Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module [v3]

[Alan Bateman]

On Fri, 26 Apr 2024 09:41:21 GMT, Raffaello Giulietti  
wrote:

>> Move all random generators mandated in package `java.util.random` and 
>> currently implemented in module `jdk.random` to module `java.base`, and 
>> remove module `jdk.random`.
>
> Raffaello Giulietti has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Terminology changes.

One thing that bothers me is that the use of ServiceLoader is specified as 
"Implementation Requirements". I wonder if we can check the original CSR and 
discussions on the PR to see if we can dig up more of the original intent. How 
useful is it to deploy with additional RandomGeneratorFactory implementations 
on the class path or module path? If a standard provider interface then I would 
have expected to see more in the javadoc and to see the `@uses` tags in the 
module declaration.

-

PR Comment: https://git.openjdk.org/jdk/pull/18932#issuecomment-2079429549

Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module [v3]

[Raffaello Giulietti]

> Move all random generators mandated in package `java.util.random` and 
> currently implemented in module `jdk.random` to module `java.base`, and 
> remove module `jdk.random`.

Raffaello Giulietti has updated the pull request incrementally with one 
additional commit since the last revision:

  Terminology changes.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18932/files
  - new: https://git.openjdk.org/jdk/pull/18932/files/ba6d052c..fd84a28b

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=18932&range=02
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18932&range=01-02

  Stats: 12 lines in 2 files changed: 2 ins; 3 del; 7 mod
  Patch: https://git.openjdk.org/jdk/pull/18932.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18932/head:pull/18932

PR: https://git.openjdk.org/jdk/pull/18932

Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module [v2]

[Alan Bateman]

On Wed, 24 Apr 2024 15:27:23 GMT, Raffaello Giulietti  
wrote:

>> Move all random generators mandated in package `java.util.random` and 
>> currently implemented in module `jdk.random` to module `java.base`, and 
>> remove module `jdk.random`.
>
> Raffaello Giulietti has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Renamed package jdk.random to jdk.internal.random.

I wish I could remember, or find the JBS issue or PR with the discussion, to 
remind us as to why the implementations were put into a separate module in the 
first place.

The changes in the PR looks okay. I'm just wondering if we also need to 
re-examine the wording in RandomGeneratorFactory methods where it documents  
"uses the service provider API". I'm surprised this is documented as an 
implementation requirement. Also "service provider API" should really be 
ServiceLoader API.

One other thing is that we'll need a release note for this change. It's 
possible there are scripts somewhere in the wild that use jlink and specify the 
jdk.random module in the sets of modules to include. These scrips will break 
with this change. I don't think it's worth leaving a hollowed out module in its 
place but we should at least document that it has been removed.

-

PR Comment: https://git.openjdk.org/jdk/pull/18932#issuecomment-2077043094

Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module

[Raffaello Giulietti]

On Wed, 24 Apr 2024 13:50:56 GMT, Raffaello Giulietti  
wrote:

> Move all random generators mandated in package `java.util.random` and 
> currently implemented in module `jdk.random` to module `java.base`, and 
> remove module `jdk.random`.

In terms of .jmod, the footprint of java.base.jmod is 20'542'561 bytes in the 
OpenJDK 22.0.1 build, and 20'552'354 bytes on my local 23 build, a difference 
of about 10 KB, or around 0.05%.

Just renamed the package to `jdk.internal.random`.

-

PR Comment: https://git.openjdk.org/jdk/pull/18932#issuecomment-2075209956

Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module [v2]

[Raffaello Giulietti]

> Move all random generators mandated in package `java.util.random` and 
> currently implemented in module `jdk.random` to module `java.base`, and 
> remove module `jdk.random`.

Raffaello Giulietti has updated the pull request incrementally with one 
additional commit since the last revision:

  Renamed package jdk.random to jdk.internal.random.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18932/files
  - new: https://git.openjdk.org/jdk/pull/18932/files/7f45c525..ba6d052c

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=18932&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18932&range=00-01

  Stats: 30 lines in 11 files changed: 0 ins; 0 del; 30 mod
  Patch: https://git.openjdk.org/jdk/pull/18932.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18932/head:pull/18932

PR: https://git.openjdk.org/jdk/pull/18932

Re: RFR: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module

[Alan Bateman]

On Wed, 24 Apr 2024 13:50:56 GMT, Raffaello Giulietti  
wrote:

> Move all random generators mandated in package `java.util.random` and 
> currently implemented in module `jdk.random` to module `java.base`, and 
> remove module `jdk.random`.

What is the footprint implications for this? I'm trying to recall what the 
reasons were for doing this split in the first place.

If the implementations are moving to java.base then I suppose they go into 
jdk.internal.random rather than jdk.random.

-

PR Comment: https://git.openjdk.org/jdk/pull/18932#issuecomment-2075112923