Re: RFR: 8261845: File permissions of packages built by jpackage
On Thu, 4 Mar 2021 03:59:27 GMT, Alexander Matveev wrote: > - Fixed by adding write permissions to .exe package. Marked as reviewed by asemenyuk (Committer). - PR: https://git.openjdk.java.net/jdk/pull/2822
Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v7]
> Modify the `unmodifiable*` methods in `java.util.Collections` to be > idempotent. That is, when given an immutable collection from > `java.util.ImmutableCollections` or `java.util.Collections`, these methods > will return the reference instead of creating a new immutable collection that > wraps the existing one. Ian Graves has updated the pull request incrementally with one additional commit since the last revision: Revert "Avoid wrapping subclasses where relevant. Updated tests." This reverts commit dda174c582590827cad3f4006a8ff9e7dd8a51ab. - Changes: - all: https://git.openjdk.java.net/jdk/pull/2596/files - new: https://git.openjdk.java.net/jdk/pull/2596/files/dda174c5..88127ed5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2596=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2596=05-06 Stats: 46 lines in 2 files changed: 0 ins; 42 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/2596.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2596/head:pull/2596 PR: https://git.openjdk.java.net/jdk/pull/2596
Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v5]
On Thu, 4 Mar 2021 03:57:35 GMT, Stuart Marks wrote: >> The `@implNote` additions are good, and the test rewrite looks good too. > > Hm. I had thought of this previously but I was a bit suspicious, and it > didn't seem like it would make much difference, so I didn't say anything. But > thinking about this further, the following issues arose: > > 1. Suppose you have an UnmodifiableSortedSet and call unmodifiableSet() on > it, it returns its argument, and then you hand it out. Its static type is Set > but it's really a SortedSet, which means somebody can downcast it and get its > comparator. This leaks some information. Offhand this doesn't look dangerous, > but it's a bit of a surprise. > > 2. Thinking about this further, this allows heap pollution. (Ian, turns out > our conversation from the other day wasn't just an idle digression.) If we > have class X and class Y extends X, then `SortedSet` cannot safely be cast > to an unmodifiable `SortedSet`. That's because comparator() will return > `Comparator` which is incorrect, since the actual comparator might > be of type `Comparator`. Actually the headSet(), subSet(), and tailSet() > methods also cause problems, because they both consume and produce the > collection element type E. > > 3. This can actually happen in practice with code in the JDK! > PriorityBlockingQueue's copy constructor does exactly the above. It takes a > Collection and does an instanceof check to see if it's a SortedSet; if it is, > it does a downcast and uses its comparator. Thus if we do the following: > > SortedSet set1 = new TreeSet<>(Integer::compare); > Set set2 = Collections.unmodifiableSet(set1); // hypothetical > version that returns its argument > PriorityBlockingQueue pbq = new PriorityBlockingQueue<>(set2); > pbq.addAll(Arrays.asList(1.0, 2.0)); > > this compiles without warnings, but it results in ClassCastException. The > culprit is the new upcast that potentially allows `SortedSet` to > be cast to `Set`, which is slipped in under the already-existing warnings > suppression. > > In any case, the extra checking in the unmodifiableSortedSet and -Map methods > needs to be taken out. Offhand I don't know if there's a similar issue > between unmodifiableSortedSet and a NavigableSet (resp., Map), but on general > principle I'd say to take it out too. It's likely not buying much anyway. > > The UnmodifiableList and UnmodifiableRandomAccessList stuff should stay, > since that's how the RandomAccess marker interface is preserved. Good thought on the heap pollution. I had only been considering point 1. I was thinking that perhaps if we could catch some of the wrapping of subclasses we might be able to guard against situations where rewrapping could occur if we were interleaving calls between subclass and superclass wrapper methods. That seems like a bit of a reach and in light of your point about heap pollution I think it makes sense to walk back the changes and stay with the original. Likewise agree on the point about Lists. - PR: https://git.openjdk.java.net/jdk/pull/2596
RFR: 8261845: File permissions of packages built by jpackage
- Fixed by adding write permissions to .exe package. - Commit messages: - 8261845: File permissions of packages built by jpackage Changes: https://git.openjdk.java.net/jdk/pull/2822/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2822=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261845 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2822.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2822/head:pull/2822 PR: https://git.openjdk.java.net/jdk/pull/2822
Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v5]
On Fri, 26 Feb 2021 21:37:14 GMT, Stuart Marks wrote: >> Ian Graves has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Test refactoring. Adding implNote to modified methods > > The `@implNote` additions are good, and the test rewrite looks good too. Hm. I had thought of this previously but I was a bit suspicious, and it didn't seem like it would make much difference, so I didn't say anything. But thinking about this further, the following issues arose: 1. Suppose you have an UnmodifiableSortedSet and call unmodifiableSet() on it, it returns its argument, and then you hand it out. Its static type is Set but it's really a SortedSet, which means somebody can downcast it and get its comparator. This leaks some information. Offhand this doesn't look dangerous, but it's a bit of a surprise. 2. Thinking about this further, this allows heap pollution. (Ian, turns out our conversation from the other day wasn't just an idle digression.) If we have class X and class Y extends X, then `SortedSet` cannot safely be cast to an unmodifiable `SortedSet`. That's because comparator() will return `Comparator` which is incorrect, since the actual comparator might be of type `Comparator`. Actually the headSet(), subSet(), and tailSet() methods also cause problems, because they both consume and produce the collection element type E. 3. This can actually happen in practice with code in the JDK! PriorityBlockingQueue's copy constructor does exactly the above. It takes a Collection and does an instanceof check to see if it's a SortedSet; if it is, it does a downcast and uses its comparator. Thus if we do the following: SortedSet set1 = new TreeSet<>(Integer::compare); Set set2 = Collections.unmodifiableSet(set1); // hypothetical version that returns its argument PriorityBlockingQueue pbq = new PriorityBlockingQueue<>(set2); pbq.addAll(Arrays.asList(1.0, 2.0)); this compiles without warnings, but it results in ClassCastException. The culprit is the new upcast that potentially allows `SortedSet` to be cast to `Set`, which is slipped in under the already-existing warnings suppression. In any case, the extra checking in the unmodifiableSortedSet and -Map methods needs to be taken out. Offhand I don't know if there's a similar issue between unmodifiableSortedSet and a NavigableSet (resp., Map), but on general principle I'd say to take it out too. It's likely not buying much anyway. The UnmodifiableList and UnmodifiableRandomAccessList stuff should stay, since that's how the RandomAccess marker interface is preserved. - PR: https://git.openjdk.java.net/jdk/pull/2596
Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v6]
> Modify the `unmodifiable*` methods in `java.util.Collections` to be > idempotent. That is, when given an immutable collection from > `java.util.ImmutableCollections` or `java.util.Collections`, these methods > will return the reference instead of creating a new immutable collection that > wraps the existing one. Ian Graves has updated the pull request incrementally with one additional commit since the last revision: Avoid wrapping subclasses where relevant. Updated tests. - Changes: - all: https://git.openjdk.java.net/jdk/pull/2596/files - new: https://git.openjdk.java.net/jdk/pull/2596/files/8109e158..dda174c5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2596=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2596=04-05 Stats: 46 lines in 2 files changed: 42 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/2596.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2596/head:pull/2596 PR: https://git.openjdk.java.net/jdk/pull/2596
Re: RFR: 8173970: jar tool should have a way to extract to a directory
Hello Lance, On 03/03/21 9:14 pm, Lance Andersen wrote: Some other things needed to be defined and agreed upon in order to move forward * The behavior if the path does not exist * If the option is specified more than once on the command line * Clarify the behavior if any of the files exist in the specified target directory. One of my previous reply included the details of how I think it should behave for 2 of the above cases. I'll paste that here again for easier visibility. As for how it should behave if the option is specified more than once, I'll spend some time today to see how the jar tool currently behaves for some of the other options in this aspect and send back my response. Thank you for your help so far. Pasting below my proposal from a previous reply for the other 2 cases: There are other discussion points around the behavior when the target directory exists or does not exist, to ensure there is some consistency with main stream tools. I'm guessing you mean the behaviour of creating a directory (or a hierarchy of directories) if the target directory is not present? My testing with the tar tool (both on MacOS and CentOS) shows that if the specified target directory doesn't exist, then the extract fails. The tar extract command doesn't create the target directory during extract. On the other hand, the unzip tool, does create the directory if it doesn't exist. However, interestingly, the unzip tool creates only one level of that directory if it doesn't exist. Specifically, if you specify: unzip foo.zip -d /tmp/blah/ and if "blah/" isn't a directory inside /tmp/ directory, then it creates the "blah/" directory inside /tmp/ and then extracts the contents of the zip into it. However, unzip foo.zip -d /tmp/blah/hello/ and if "blah/" isn't a directory inside /tmp/ directory, then this command fails with an error and it doesn't create the hierarchy of the target directories. Coming to the jimage and the jmod commands, both these commands create the entire directory hierarchy if the target directory specified during extract, using --dir, doesn't exist. So a command like: jimage extract --dir /tmp/blah/foo/bar/ jdkmodules will create the blah/foo/bar/ directory hierarchy if blah doesn't exist in /tmp/, while extracting the "jdkmodules" image. From the user point of view, I think this behaviour of creating the directories if the target directory doesn't exist, is probably the most intuitive and useful and if we did decide to use this approach for this new option for jar extract command, then it would align with what we already do in jimage and jmod commands. One another minor detail, while we are at this, is that, IMO we should let the jar extract command to continue to behave the way it currently does when it comes to overwriting existing files. If the jar being extracted contains a file by the same name, in the target directory (hierarchy) then it should continue to overwrite that file. In other words, I don't think we should change the way the jar extract command currently behaves where it overwrites existing files when extracting. -Jaikiran
Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v5]
On Thu, 4 Mar 2021 02:01:02 GMT, Ian Graves wrote:
>> src/java.base/share/classes/java/util/Collections.java line 1168:
>>
>>> 1166: */
>>> 1167: public static SortedSet unmodifiableSortedSet(SortedSet
>>> s) {
>>> 1168: if (s.getClass() == UnmodifiableSortedSet.class) {
>>
>> Should a check like this also included "|| ==
>> UnmodifiableNavigableSet.class" or was there an explicit decision that the
>> cost/benefit is not worthwhile, unlike in the case of unmodifiableList below?
>
> This is a good point. The case of unmodifiableList is such because the method
> can return two different classes depending the nature of the argument. I feel
> as though if we made this change here, we should consider doing the same
> check for vanilla unmodifiableSet to ensure it, too, doesn't wrap its
> subclasses. I'm amenable to this.
To the second part of the question, there was no explicit cost/benefit analysis
RE List or this case.
-
PR: https://git.openjdk.java.net/jdk/pull/2596
Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v5]
On Wed, 3 Mar 2021 23:29:33 GMT, Joe Darcy wrote:
>> Ian Graves has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Test refactoring. Adding implNote to modified methods
>
> src/java.base/share/classes/java/util/Collections.java line 1168:
>
>> 1166: */
>> 1167: public static SortedSet unmodifiableSortedSet(SortedSet
>> s) {
>> 1168: if (s.getClass() == UnmodifiableSortedSet.class) {
>
> Should a check like this also included "|| == UnmodifiableNavigableSet.class"
> or was there an explicit decision that the cost/benefit is not worthwhile,
> unlike in the case of unmodifiableList below?
This is a good point. The case of unmodifiableList is such because the method
can return two different classes depending the nature of the argument. I feel
as though if we made this change here, we should consider doing the same check
for vanilla unmodifiableSet to ensure it, too, doesn't wrap its subclasses. I'm
amenable to this.
-
PR: https://git.openjdk.java.net/jdk/pull/2596
Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v5]
On Fri, 26 Feb 2021 20:15:19 GMT, Ian Graves wrote:
>> Modify the `unmodifiable*` methods in `java.util.Collections` to be
>> idempotent. That is, when given an immutable collection from
>> `java.util.ImmutableCollections` or `java.util.Collections`, these methods
>> will return the reference instead of creating a new immutable collection
>> that wraps the existing one.
>
> Ian Graves has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Test refactoring. Adding implNote to modified methods
src/java.base/share/classes/java/util/Collections.java line 1168:
> 1166: */
> 1167: public static SortedSet unmodifiableSortedSet(SortedSet
> s) {
> 1168: if (s.getClass() == UnmodifiableSortedSet.class) {
Should a check like this also included "|| == UnmodifiableNavigableSet.class"
or was there an explicit decision that the cost/benefit is not worthwhile,
unlike in the case of unmodifiableList below?
-
PR: https://git.openjdk.java.net/jdk/pull/2596
Integrated: 8262927: Explicitly state fields examined for BigDecimal.hashCode
On Wed, 3 Mar 2021 22:20:45 GMT, Joe Darcy wrote: > The class BigDecimal can be and sometimes is subclassed. The spec of > BigDecimal.hashCode is improved slightly by explicitly stating it is a > function of the unscaled value and the scale of the BigDecimal, the same > fields examined by equals. > > It is a conscious choice to *not* explicitly state what the exact hash > function is. > > As the behavior of hashCode is mostly implied from equals, I don't think a > CSR is necessary in this case. This pull request has now been integrated. Changeset: 28489389 Author:Joe Darcy URL: https://git.openjdk.java.net/jdk/commit/28489389 Stats: 7 lines in 1 file changed: 5 ins; 0 del; 2 mod 8262927: Explicitly state fields examined for BigDecimal.hashCode Reviewed-by: bpb - PR: https://git.openjdk.java.net/jdk/pull/2817
RFR: 8262989: Vectorize VectorShuffle checkIndexes, wrapIndexes and laneIsValid methods
The hot path of VectorShuffle checkIndexes, wrapIndexes and laneIsValid methods can be implemented using Vector API methods. For the attached jmh TestSlice.java, performance improves as below. Before: Benchmark (size) Mode Cnt Score Error Units TestSlice.vectorSliceOrigin 1024 thrpt5 1224.698 ± 53.825 ops/ms TestSlice.vectorSliceUnsliceOrigin 1024 thrpt5 657.895 ± 31.945 ops/ms After: Benchmark (size) Mode Cnt Score Error Units TestSlice.vectorSliceOrigin 1024 thrpt5 11221.532 ± 88.616 ops/ms TestSlice.vectorSliceUnsliceOrigin 1024 thrpt5 6509.519 ± 18.102 ops/ms - Commit messages: - 8262989: Vectorize VectorShuffle checkIndexes, wrapIndexes and laneIsValid methods Changes: https://git.openjdk.java.net/jdk/pull/2819/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2819=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8262989 Stats: 43 lines in 8 files changed: 7 ins; 9 del; 27 mod Patch: https://git.openjdk.java.net/jdk/pull/2819.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2819/head:pull/2819 PR: https://git.openjdk.java.net/jdk/pull/2819
Re: RFR: 8262927: Explicitly state fields examined for BigDecimal.hashCode
On Wed, 3 Mar 2021 22:20:45 GMT, Joe Darcy wrote: > The class BigDecimal can be and sometimes is subclassed. The spec of > BigDecimal.hashCode is improved slightly by explicitly stating it is a > function of the unscaled value and the scale of the BigDecimal, the same > fields examined by equals. > > It is a conscious choice to *not* explicitly state what the exact hash > function is. > > As the behavior of hashCode is mostly implied from equals, I don't think a > CSR is necessary in this case. Looks fine. - Marked as reviewed by bpb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2817
RFR: 8262927: Explicitly state fields examined for BigDecimal.hashCode
The class BigDecimal can be and sometimes is subclassed. The spec of BigDecimal.hashCode is improved slightly by explicitly stating it is a function of the unscaled value and the scale of the BigDecimal, the same fields examined by equals. It is a conscious choice to *not* explicitly state what the exact hash function is. As the behavior of hashCode is mostly implied from equals, I don't think a CSR is necessary in this case. - Commit messages: - 8262927: Explicitly state fields examined for BigDecimal.hashCode Changes: https://git.openjdk.java.net/jdk/pull/2817/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2817=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8262927 Stats: 7 lines in 1 file changed: 5 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2817.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2817/head:pull/2817 PR: https://git.openjdk.java.net/jdk/pull/2817
[PING] RE: [PING] RE: 8250678: ModuleDescriptor.Version parsing treats empty segments inconsistently
Hello,
Please reply if anyone can be a sponsor.
Regards,
Masanori Yano
> -Original Message-
> From: Yano, Masanori
> Sent: Tuesday, January 12, 2021 4:32 PM
> To: 'core-libs-dev@openjdk.java.net'
> Subject: RE: [PING] RE: 8250678: ModuleDescriptor.Version parsing treats empty
> segments inconsistently
>
> Hello,
>
> Please reply if anyone can be a sponsor.
>
> Regards,
> Masanori Yano
>
> > -Original Message-
> > From: Yano, Masanori
> > Sent: Wednesday, December 23, 2020 5:01 PM
> > To: 'core-libs-dev@openjdk.java.net'
> > Subject: [PING] RE: 8250678: ModuleDescriptor.Version parsing treats
> > empty segments inconsistently
> >
> > Hello,
> >
> > Please reply if anyone can be a sponsor.
> >
> > Regards,
> > Masanori Yano
> >
> > > -Original Message-
> > > From: Yano, Masanori
> > > Sent: Wednesday, November 4, 2020 6:03 PM
> > > To: 'core-libs-dev@openjdk.java.net'
> > >
> > > Subject: 8250678: ModuleDescriptor.Version parsing treats empty
> > > segments inconsistently
> > >
> > > Hello.
> > >
> > > I would like to contribute for JDK-8250678.
> > >
> > > The 'parse' method of ModuleDescriptor.Version class behaves
> > > incorrectly when the input string contains consecutive delimiters.
> > >
> > > The 'parse' method treats strings as three sections, but the parsing
> > > method differs between the method for the version sections and the ones
> > > for
> others.
> > > In version sections, the 'parse' method takes a single character in
> > > a loop and determines whether it is a delimiter. In pre and build
> > > sections, the parse method takes two characters in a loop and
> > > determines whether
> > the second character is the delimiter.
> > > Therefore, if the string contains a sequence of delimiters in pre or
> > > build section, the 'parse' method treats character at the
> > > odd-numbered position as a token and the one at even-numbered as a
> > > delimiter
> > >
> > > A string containing consecutive delimiters is an incorrect version
> > > string, but this behavior does not follow the API specification.
> > > https://download.java.net/java/early_access/jdk16/docs/api/java.base
> > > /j
> > > ava/lang/
> > > module/ModuleDescriptor.Version.html
> > >
> > > Therefore, I propose to fix the parsing method of the pre section
> > > and the build section in the same way as the version.
> > >
> > > Please sponsor the following change.
> > >
> > > diff -r bdc20ee1a68d
> > > src/java.base/share/classes/java/lang/module/ModuleDescriptor.java
> > > --- a/src/java.base/share/classes/java/lang/module/ModuleDescriptor.java
> > > Fri Sep 04 23:51:26 2020 -0400
> > > +++ b/src/java.base/share/classes/java/lang/module/ModuleDescriptor.
> > > +++ ja
> > > +++ va
> > > Wed Oct 28 17:06:47 2020 +0900
> > > @@ -1053,13 +1053,6 @@
> > >
> > > while (i < n) {
> > > c = v.charAt(i);
> > > -if (c >= '0' && c <= '9')
> > > -i = takeNumber(v, i, pre);
> > > -else
> > > -i = takeString(v, i, pre);
> > > -if (i >= n)
> > > -break;
> > > -c = v.charAt(i);
> > > if (c == '.' || c == '-') {
> > > i++;
> > > continue;
> > > @@ -1068,6 +1061,10 @@
> > > i++;
> > > break;
> > > }
> > > +if (c >= '0' && c <= '9')
> > > +i = takeNumber(v, i, pre);
> > > +else
> > > +i = takeString(v, i, pre);
> > > }
> > >
> > > if (c == '+' && i >= n) @@ -1075,17 +1072,14 @@
> > >
> > > while (i < n) {
> > > c = v.charAt(i);
> > > +if (c == '.' || c == '-' || c == '+') {
> > > +i++;
> > > +continue;
> > > +}
> > > if (c >= '0' && c <= '9')
> > > i = takeNumber(v, i, build);
> > > else
> > > i = takeString(v, i, build);
> > > -if (i >= n)
> > > -break;
> > > -c = v.charAt(i);
> > > -if (c == '.' || c == '-' || c == '+') {
> > > -i++;
> > > -continue;
> > > -}
> > > }
> > >
> > > this.version = v;
> > > diff -r bdc20ee1a68d test/jdk/java/lang/module/VersionTest.java
> > > --- a/test/jdk/java/lang/module/VersionTest.java Fri Sep 04 23:51:26 2020
> > > -0400
> > > +++ b/test/jdk/java/lang/module/VersionTest.java Wed Oct 28 17:06:47
> > > 2020 +0900
> > > @@ -148,6 +148,8 @@
> > > { "1", "1.0.0" },
> > > { "1.0", "1.0.0" },
> > > { "1.0-beta", "1.0.0-beta" },
> > > +{ "1.0-1.1", "1.0-1..1" },
> > > +{ "1.0-1+1", "1.0-1.+1" },
> > >
Re: RFR: JDK-8261518: jpackage looks for main module in current dir when there is no module-path [v2]
On Wed, 3 Mar 2021 13:56:01 GMT, Andy Herrick wrote:
>> test/jdk/tools/jpackage/share/jdk/jpackage/tests/NoMPathRuntimeTest.java
>> line 125:
>>
>>> 123: .addArguments("-cvf", "junk.jar",
>>> 124: "-C", tmpdir.toString(), "Hello.class")
>>> 125: .execute();
>>
>> Single line `HelloApp.createBundle("junk.jar:Hello", tmpdir);` would compile
>> source class and put it into "junk.jar" in `tmpdir` folder. It can be used
>> to replace lines from [109, 125] range.
>>
>> What is the point to build "junk.jar"? I don't see how it is used in the
>> test.
>
> The bug is that when --module-path option is not used in a modular app,
> jpackage uses a module-path with "." on it.
> Having a non-modular jar in the modular path is an error.
> So with this non-modular Hello.jar in the current directory the jpackage
> command failed before the fix, and succeeds after the fix.
>
> I can create the non-modular Hello.jar in the current directory with one line:
> HelloApp.createBundle(JavaAppDesc.parse("junk.jar:Hello"), Path.of("."))
Another precondition for the test is that Java runtime used with jpackage
command should include module with app main class, right?
Test arguments are: `List.of("Hello", "com.foo/com.foo.main.Aloha");`. The
first argument is non-modular app, it is not used with jlink. What is the point
to run the test for non-modular app?
-
PR: https://git.openjdk.java.net/jdk/pull/2781
Re: RFR: JDK-8261518: jpackage looks for main module in current dir when there is no module-path [v4]
On Wed, 3 Mar 2021 14:50:07 GMT, Andy Herrick wrote:
>> when the app modules have already been jlinked with the runtime, and there
>> is no need for module-path, jpackage was acting as if the module-path was
>> "." and picking up jars in the current directory.
>
> Andy Herrick has updated the pull request incrementally with one additional
> commit since the last revision:
>
> JDK-8261518: jpackage looks for main module in current dir when there is no
> module-path
Changes requested by asemenyuk (Committer).
test/jdk/tools/jpackage/share/jdk/jpackage/tests/NoMPathRuntimeTest.java line
105:
> 103:
> 104: // create a non-modular jar in the current directory
> 105: HelloApp.createBundle(JavaAppDesc.parse("junk.jar:Hello"),
> Path.of("."));
Test files must be created in test work directory (`TKit.workDir()`) and not in
the current directory. Test harness creates empty work directory for every test
run. In case of running this test multiple times and if previous test execution
was aborted "junk.jar" will be in the current directory from the previous test
run and the next test run will fail.
Running test multiple times without running clean up is the case when the test
is executed in IDE under debugger and debugging session is aborted.
If you absolutely must create "junk.jar" in the current directory, you need to
remove it manually. Something like this:
Path junkJar = null;
try {
junkJar = HelloApp.createBundle(JavaAppDesc.parse("junk.jar:Hello"),
Path.of("."));
...
cmd.executeAndAssertHelloAppImageCreated();
} finally {
if (junkJar != null) {
TKit.deleteIfExists(junkJar);
}
}
-
PR: https://git.openjdk.java.net/jdk/pull/2781
Re: Question about java.system.class.loader and the module system
Hi Alan, thanks for the quick response. Please find my answers inline On Wed, Mar 3, 2021 at 1:38 PM Alan Bateman wrote: > > On 03/03/2021 10:43, Volker Simonis wrote: > > : > > > > My question now is if this is an inherent property of the module > > system or merely an implementation detail? I.e. would it be possible > > to put the application module into its own layer and initialize that > > only later when the custom system class loader will be available? I > > think this would be relatively easy if the custom class loader can be > > found on the class path. If the custom class loader is in a module > > itself, that module would have to be in the boot layer to make it > > accessible to the default system class loader. > The ability to set a custom class loader as the system class loader is > somewhat legacy feature. There was consideration given to deprecating > and removing it a few years ago but we ended up leaving it "as is" in > case there are still application servers using it. So it should work the > same in JDK 16 as it did in JDK 8/older releases. That is, it will be > created with the built-in application class loader as parent and the > custom system class loader will be the default parent class loader for > delegation. > > You are correct that an initial module on the application module path > will be loaded by the built-in application class loader. All modules on > the application module path are mapped to the built-in application class > loader. If an initial module is specified to the java launcher (--module > or -m) then it just locates the module in the boot layer and invokes its > main class. There is no role for a custom system class loader here. > > If you are looking to deploy a custom system class loader in a module on > the module path then it should work as long as you export the package > with the custom class loader to java.base. I'm not sure if you really > want to do this or whether it's a means to an end. Maybe it would be > easier to start with a summary on what you are looking it do? Are you > looking to intercept all attempts to load classes? Is there a java agent > in the picture too? I have currently no specific use case in mind. I was just doing some experiments and was hoping that I can intercept the loading of all application classes by defining a custom system class loader. I'm aware that I can use the JVMTI "ClassFileLoadHook" to intercept the loading of ALL classes, but that's just a little more complicated :) > > The current API documentation of the system class loader [1] mentions > > that the system class loader "is typically the class loader used to > > start the application". Shouldn't that be updated to mention that this > > will not be the case for modular applications? > The javadoc is correct because it will be rare to deploy with > java.system.class.loader set to a custom class loader. Assuming it is > not set then you should find that the initial class will be defined by > this class loader. This goes for both the class path and module path cases. Maybe I should have been more specific. I meant that the paragraph on "java.system.class.loader" should contain one more sentence mentioning that setting it will only have the desired effects for the loading of application classes for non-modularized applications. Thank you and best regards, Volker > > > -Alan > >
Integrated: 8259267: Refactor LoaderLeak shell test as java test.
On Wed, 2 Dec 2020 20:23:13 GMT, Ivan Šipka wrote: > Refactor `test/jdk/java/lang/annotation/loaderLeak/LoaderLeak.sh` as java > test. This pull request has now been integrated. Changeset: 75aa1546 Author:Ivan Šipka Committer: Igor Ignatyev URL: https://git.openjdk.java.net/jdk/commit/75aa1546 Stats: 440 lines in 6 files changed: 135 ins; 305 del; 0 mod 8259267: Refactor LoaderLeak shell test as java test. Reviewed-by: rriggs, iignatyev, dfuchs - PR: https://git.openjdk.java.net/jdk/pull/1577
Re: RFR: 8259267: Refactor LoaderLeak shell test as java test. [v15]
On Wed, 3 Mar 2021 19:56:07 GMT, Ivan Šipka wrote: >> Refactor `test/jdk/java/lang/annotation/loaderLeak/LoaderLeak.sh` as java >> test. > > Ivan Šipka has updated the pull request incrementally with one additional > commit since the last revision: > > 8166026: Refactor java/lang shell tests to java Marked as reviewed by iignatyev (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1577
Re: RFR: 8259267: Refactor LoaderLeak shell test as java test. [v15]
> Refactor `test/jdk/java/lang/annotation/loaderLeak/LoaderLeak.sh` as java > test. Ivan Šipka has updated the pull request incrementally with one additional commit since the last revision: 8166026: Refactor java/lang shell tests to java - Changes: - all: https://git.openjdk.java.net/jdk/pull/1577/files - new: https://git.openjdk.java.net/jdk/pull/1577/files/8cad13df..b548e39f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1577=14 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1577=13-14 Stats: 6 lines in 1 file changed: 0 ins; 5 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1577.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1577/head:pull/1577 PR: https://git.openjdk.java.net/jdk/pull/1577
Re: RFR: 8259267: Refactor LoaderLeak shell test as java test. [v14]
On Wed, 3 Mar 2021 18:26:19 GMT, Igor Ignatyev wrote:
>> Ivan Šipka has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8166026: Refactor java/lang shell tests to java
>
> test/jdk/java/lang/annotation/LoaderLeakTest.java line 74:
>
>> 72: catch (NullPointerException npe) {
>> 73: throw new AssertionError("c.get() should never return
>> null", npe);
>> 74: }
>
> I don't think it's the right way to handle that, you don't know if this NPE
> is from `c.get`, so the exception messages might be misleading. I'd just
> remove try/catch, if NPE occurs jtreg will report the test as failed w/ NPE
> as a reason, so whoever analyzes it will be able to understand.
>
> alternatively, you can save c.get into a local variable which you nulls later
> one, e.g.
> ```
> static void doTest(boolean readAnn) throws Exception {
> ...
> var tmp = c.get();
> if (t == null) throw new AssertionError("c.get is null");
> if (t.getClassLoader() != loader) throw new AssertionError("wrong
> classloader");
> t = null;
@iignatev I just reemove the try catch I think the comments are informative and
will help with analysis with potential NPE
-
PR: https://git.openjdk.java.net/jdk/pull/1577
Integrated: 8261862: Expand discussion of rationale for BigDecimal equals/compareTo semantics
On Wed, 3 Mar 2021 07:20:03 GMT, Joe Darcy wrote: > I considered @stuart-marks previous suggestion during the code review of > JDK-8261123 to include a more explicit discussion of why, say, different > representations of 2 should not be regarded as equivalent. After > contemplating several alternatives, I didn't find anything simpler than > Stuart's 2/3 example so I used that as seen in the diff. > > A short digression, BigDecimal supports both fixed-point style and > floating-point style rounding. Floating-point rounding primarily replies on > the number of precision digits, regards of their scale, while fixed-point > style rounding prioritizes the scale. The number of digits of eventual output > is a function of number number of digits in the inputs and the number of > precision digits in a floating-point style rounding. A floating-point style > rounding has a preferred scale, rather than a fixed scale based on the > inputs. The fixed-point style divide method used in the example has a scale > based on the dividend, allowing a relatively simple expression to show a > distinction between 2.0 and 2.00. This pull request has now been integrated. Changeset: a1181852 Author:Joe Darcy URL: https://git.openjdk.java.net/jdk/commit/a1181852 Stats: 19 lines in 1 file changed: 11 ins; 0 del; 8 mod 8261862: Expand discussion of rationale for BigDecimal equals/compareTo semantics Reviewed-by: smarks, bpb - PR: https://git.openjdk.java.net/jdk/pull/2804
Re: RFR: 8261862: Expand discussion of rationale for BigDecimal equals/compareTo semantics [v2]
On Wed, 3 Mar 2021 18:19:33 GMT, Stuart Marks wrote:
>> Joe Darcy 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 three additional commits
>> since the last revision:
>>
>> - Respond to review feedback and reflow paragraphs.
>> - Merge branch 'master' into 8261862
>> - 8261862: Expand discussion of rationale for BigDecimal equals/compareTo
>> semantics
>
> src/java.base/share/classes/java/math/BigDecimal.java line 3155:
>
>> 3153: * {@code new BigDecimal("2.00").divide(BigDecimal.valueOf(3),
>> 3154: * HALF_UP)} which evaluates to 0.67.
>> 3155: *
>
> Should this be in an `@apiNote`?
>
> I'm not sure adding the boldface 0.**6**7 is helpful; 0.7 is self-evidently
> unequal to 0.67.
Changed as suggested in final version.
-
PR: https://git.openjdk.java.net/jdk/pull/2804
Re: RFR: 8261862: Expand discussion of rationale for BigDecimal equals/compareTo semantics [v2]
On Wed, 3 Mar 2021 17:49:26 GMT, Brian Burkhalter wrote:
>> Joe Darcy 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 three additional commits
>> since the last revision:
>>
>> - Respond to review feedback and reflow paragraphs.
>> - Merge branch 'master' into 8261862
>> - 8261862: Expand discussion of rationale for BigDecimal equals/compareTo
>> semantics
>
> src/java.base/share/classes/java/math/BigDecimal.java line 3146:
>
>> 3144: * method since the former has [{@code BigInteger},
>> 3145: * {@code scale}] components equal to [20, 1] while the latter has
>> 3146: * components equals to [200, 2].
>
> s/equals/equal/
Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/2804
Re: RFR: 8261862: Expand discussion of rationale for BigDecimal equals/compareTo semantics [v2]
> I considered @stuart-marks previous suggestion during the code review of > JDK-8261123 to include a more explicit discussion of why, say, different > representations of 2 should not be regarded as equivalent. After > contemplating several alternatives, I didn't find anything simpler than > Stuart's 2/3 example so I used that as seen in the diff. > > A short digression, BigDecimal supports both fixed-point style and > floating-point style rounding. Floating-point rounding primarily replies on > the number of precision digits, regards of their scale, while fixed-point > style rounding prioritizes the scale. The number of digits of eventual output > is a function of number number of digits in the inputs and the number of > precision digits in a floating-point style rounding. A floating-point style > rounding has a preferred scale, rather than a fixed scale based on the > inputs. The fixed-point style divide method used in the example has a scale > based on the dividend, allowing a relatively simple expression to show a > distinction between 2.0 and 2.00. Joe Darcy 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 three additional commits since the last revision: - Respond to review feedback and reflow paragraphs. - Merge branch 'master' into 8261862 - 8261862: Expand discussion of rationale for BigDecimal equals/compareTo semantics - Changes: - all: https://git.openjdk.java.net/jdk/pull/2804/files - new: https://git.openjdk.java.net/jdk/pull/2804/files/48b49386..75b27c3f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2804=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2804=00-01 Stats: 774 lines in 17 files changed: 703 ins; 13 del; 58 mod Patch: https://git.openjdk.java.net/jdk/pull/2804.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2804/head:pull/2804 PR: https://git.openjdk.java.net/jdk/pull/2804
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]
On Wed, 3 Mar 2021 17:46:41 GMT, Andrew Haley wrote: > A list of the bugs that our internal testing revealed so far ... Thank you! Some of them look like test issues, but a few need more serious consideration. I want to resolve https://bugs.openjdk.java.net/browse/JDK-8262903 at least, along with a few remaining comments. - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8259267: Refactor LoaderLeak shell test as java test. [v14]
On Wed, 3 Mar 2021 15:55:03 GMT, Ivan Šipka wrote:
>> Refactor `test/jdk/java/lang/annotation/loaderLeak/LoaderLeak.sh` as java
>> test.
>
> Ivan Šipka has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8166026: Refactor java/lang shell tests to java
test/jdk/java/lang/annotation/LoaderLeakTest.java line 74:
> 72: catch (NullPointerException npe) {
> 73: throw new AssertionError("c.get() should never return
> null", npe);
> 74: }
I don't think it's the right way to handle that, you don't know if this NPE is
from `c.get`, so the exception messages might be misleading. I'd just remove
try/catch, if NPE occurs jtreg will report the test as failed w/ NPE as a
reason, so whoever analyzes it will be able to understand.
alternatively, you can save c.get into a local variable which you nulls later
one, e.g.
```
static void doTest(boolean readAnn) throws Exception {
...
var tmp = c.get();
if (t == null) throw new AssertionError("c.get is null");
if (t.getClassLoader() != loader) throw new AssertionError("wrong classloader");
t = null;
-
PR: https://git.openjdk.java.net/jdk/pull/1577
Re: RFR: 8261862: Expand discussion of rationale for BigDecimal equals/compareTo semantics
On Wed, 3 Mar 2021 07:20:03 GMT, Joe Darcy wrote: > I considered @stuart-marks previous suggestion during the code review of > JDK-8261123 to include a more explicit discussion of why, say, different > representations of 2 should not be regarded as equivalent. After > contemplating several alternatives, I didn't find anything simpler than > Stuart's 2/3 example so I used that as seen in the diff. > > A short digression, BigDecimal supports both fixed-point style and > floating-point style rounding. Floating-point rounding primarily replies on > the number of precision digits, regards of their scale, while fixed-point > style rounding prioritizes the scale. The number of digits of eventual output > is a function of number number of digits in the inputs and the number of > precision digits in a floating-point style rounding. A floating-point style > rounding has a preferred scale, rather than a fixed scale based on the > inputs. The fixed-point style divide method used in the example has a scale > based on the dividend, allowing a relatively simple expression to show a > distinction between 2.0 and 2.00. Marked as reviewed by bpb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2804
Re: RFR: 8261862: Expand discussion of rationale for BigDecimal equals/compareTo semantics
On Wed, 3 Mar 2021 07:20:03 GMT, Joe Darcy wrote:
> I considered @stuart-marks previous suggestion during the code review of
> JDK-8261123 to include a more explicit discussion of why, say, different
> representations of 2 should not be regarded as equivalent. After
> contemplating several alternatives, I didn't find anything simpler than
> Stuart's 2/3 example so I used that as seen in the diff.
>
> A short digression, BigDecimal supports both fixed-point style and
> floating-point style rounding. Floating-point rounding primarily replies on
> the number of precision digits, regards of their scale, while fixed-point
> style rounding prioritizes the scale. The number of digits of eventual output
> is a function of number number of digits in the inputs and the number of
> precision digits in a floating-point style rounding. A floating-point style
> rounding has a preferred scale, rather than a fixed scale based on the
> inputs. The fixed-point style divide method used in the example has a scale
> based on the dividend, allowing a relatively simple expression to show a
> distinction between 2.0 and 2.00.
Marked as reviewed by smarks (Reviewer).
src/java.base/share/classes/java/math/BigDecimal.java line 3155:
> 3153: * {@code new BigDecimal("2.00").divide(BigDecimal.valueOf(3),
> 3154: * HALF_UP)} which evaluates to 0.67.
> 3155: *
Should this be in an `@apiNote`?
I'm not sure adding the boldface 0.**6**7 is helpful; 0.7 is self-evidently
unequal to 0.67.
-
PR: https://git.openjdk.java.net/jdk/pull/2804
Re: RFR: 8262277: java.net.URLClassLoader.getResource throws undocumented IllegalArgumentException
On Wed, 3 Mar 2021 18:10:25 GMT, Brent Christian wrote: >> This seems to be a long standing bug, maybe goes all the way back to JDK >> 1.2. Are you planning to add a regression test? > > Hi, Craig > The commented-out lines should be removed from the change. > > As Alan said, a regression test will be needed. At minimum, it should test a > method that returns a URL, as well as a method that returns an > Enumeration (which can also lead to an IAE, as I noted in the bug > report). > > Also, though the compatibility risk is low, it would be good to include a CSR > for this change to long-standing behavior. Also, the copyright year should be updated: 2019 -> 2021 - PR: https://git.openjdk.java.net/jdk/pull/2662
Re: RFR: 8262277: java.net.URLClassLoader.getResource throws undocumented IllegalArgumentException
On Wed, 3 Mar 2021 14:28:00 GMT, Alan Bateman wrote:
>> _NOTE_: I've reported this issue at https://bugreport.java.com/ (internal
>> review ID : 9069151)
>>
>> `java.net.URLClassLoader.getResource` can throw an undocumented
>> `IllegalArgumentException`.
>>
>> According to the javadoc for the `getResource` and `findResource` methods,
>> neither should be throwing `IllegalArgumentException` - they should return
>> null if the resource can't be resolved.
>>
>> Quoting the javadoc for
>> [`URLClassLoader.html#findResource`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/URLClassLoader.html#findResource(java.lang.String))
>>> Returns:
>>> a URL for the resource, or null if the resource could not be found, or
>>> if the loader is closed.
>>
>> And quoting the javadoc for
>> [`ClassLoader.html#getResource`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/ClassLoader.html#getResource(java.lang.String))
>>> Returns:
>>> URL object for reading the resource; null if the resource could not be
>>> found, a URL could not be constructed to locate the resource, the resource
>>> is in a package that is not opened unconditionally, or access to the
>>> resource is denied by the security manager.
>>
>> Neither mentions throwing `IllegalArgumentException` and both are clear that
>> when URL can't be constructed, `null` should be returned.
>>
>> Here's a stack trace:
>> java.lang.IllegalArgumentException: name
>> at
>> java.base/jdk.internal.loader.URLClassPath$Loader.findResource(URLClassPath.java:600)
>> at
>> java.base/jdk.internal.loader.URLClassPath.findResource(URLClassPath.java:291)
>> at java.base/java.net.URLClassLoader$2.run(URLClassLoader.java:655)
>> at java.base/java.net.URLClassLoader$2.run(URLClassLoader.java:653)
>> at java.base/java.security.AccessController.doPrivileged(Native
>> Method)
>> at
>> java.base/java.net.URLClassLoader.findResource(URLClassLoader.java:652)
>>
>> Looking at
>> [`URLClassPath.findResource`](https://github.com/openjdk/jdk/blob/2b00367e1154feb2c05b84a11d62fb5750e46acf/src/java.base/share/classes/jdk/internal/loader/URLClassPath.java#L603)
>> URL findResource(final String name, boolean check) {
>> URL url;
>> try {
>> url = new URL(base, ParseUtil.encodePath(name, false));
>> } catch (MalformedURLException e) {
>> throw new IllegalArgumentException("name");
>> }
>>
>> Instead of throwing `IllegalArgumentException`, that line should simply
>> return `null`.
>>
>> A similar issue exists at
>> [`URLClassPath.getResource`](https://github.com/openjdk/jdk/blob/2b00367e1154feb2c05b84a11d62fb5750e46acf/src/java.base/share/classes/jdk/internal/loader/URLClassPath.java#L639)
>> URL findResource(final String name, boolean check) {
>> URL url;
>> try {
>> url = new URL(base, ParseUtil.encodePath(name, false));
>> } catch (MalformedURLException e) {
>> throw new IllegalArgumentException("name");
>> }
>>
>> Instead of throwing `IllegalArgumentException`, that line should simply
>> return `null`.
>
> This seems to be a long standing bug, maybe goes all the way back to JDK 1.2.
> Are you planning to add a regression test?
Hi, Craig
The commented-out lines should be removed from the change.
As Alan said, a regression test will be needed. At minimum, it should test a
method that returns a URL, as well as a method that returns an Enumeration
(which can also lead to an IAE, as I noted in the bug report).
Also, though the compatibility risk is low, it would be good to include a CSR
for this change to long-standing behavior.
-
PR: https://git.openjdk.java.net/jdk/pull/2662
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v28]
> This PR is to introduce a new random number API for the JDK. The primary API > is found in RandomGenerator and RandomGeneratorFactory. Further description > can be found in the JEP https://openjdk.java.net/jeps/356 . > > javadoc can be found at > http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html > > old PR: https://github.com/openjdk/jdk/pull/1273 Jim Laskey has updated the pull request incrementally with one additional commit since the last revision: Use isAnnotationPresent - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/7439c2ba..345a17cc Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=27 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=26-27 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/1292.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1292/head:pull/1292 PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]
On Wed, 3 Mar 2021 17:39:28 GMT, Gerard Ziemski wrote: > A list of the bugs that our internal testing revealed so far: Are any of these blockers for integration? Some of them are to do with things like features that aren't yet supported, and we can't fix what we can't see. - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]
On Tue, 2 Mar 2021 11:05:20 GMT, Anton Kozlov wrote: >> For platform files that were copied from other ports to this port, if the >> file wasn't >> changed I presume the copyright years are left alone. If the file required >> changes >> for this port, I expect the year to be updated to 2021. How are you >> verifying that >> these copyright years are being properly managed on the new files? >> >> For the new W^X helpers, e.g., WXWriteFromExecSetter, a short comment >> explaining why one was landed in a particular place would help reviewers. >> Also see my comment about creating a new ThreadToNativeWithWXExecFromVM >> helper. >> >> I'm stopping my review with all the src/hotspot files done for now. > >> For platform files that were copied from other ports to this port, if the >> file wasn't >> changed I presume the copyright years are left alone. If the file required >> changes >> for this port, I expect the year to be updated to 2021. How are you >> verifying that >> these copyright years are being properly managed on the new files? > > There are no exact copies, based on > git -c diff.renameLimit=1000 diff --find-copies-harder -C75% > --name-status upstream/master... > > So every file changed in the branch potentially needs the copyright update. > All file diffs are not trivial, IMHO. > > I'll run the copyright update after we fix a few remaining issues with the > PR, to avoid updating copyright and changing/reverting the actual content. A list of the bugs that our internal testing revealed so far: https://bugs.openjdk.java.net/browse/JDK-8262952 https://bugs.openjdk.java.net/browse/JDK-8262894 https://bugs.openjdk.java.net/browse/JDK-8262895 https://bugs.openjdk.java.net/browse/JDK-8262896 https://bugs.openjdk.java.net/browse/JDK-8262897 https://bugs.openjdk.java.net/browse/JDK-8262898 https://bugs.openjdk.java.net/browse/JDK-8262899 https://bugs.openjdk.java.net/browse/JDK-8262900 https://bugs.openjdk.java.net/browse/JDK-8262901 https://bugs.openjdk.java.net/browse/JDK-8262903 https://bugs.openjdk.java.net/browse/JDK-8262904 - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8261862: Expand discussion of rationale for BigDecimal equals/compareTo semantics
On Wed, 3 Mar 2021 07:20:03 GMT, Joe Darcy wrote:
> I considered @stuart-marks previous suggestion during the code review of
> JDK-8261123 to include a more explicit discussion of why, say, different
> representations of 2 should not be regarded as equivalent. After
> contemplating several alternatives, I didn't find anything simpler than
> Stuart's 2/3 example so I used that as seen in the diff.
>
> A short digression, BigDecimal supports both fixed-point style and
> floating-point style rounding. Floating-point rounding primarily replies on
> the number of precision digits, regards of their scale, while fixed-point
> style rounding prioritizes the scale. The number of digits of eventual output
> is a function of number number of digits in the inputs and the number of
> precision digits in a floating-point style rounding. A floating-point style
> rounding has a preferred scale, rather than a fixed scale based on the
> inputs. The fixed-point style divide method used in the example has a scale
> based on the dividend, allowing a relatively simple expression to show a
> distinction between 2.0 and 2.00.
src/java.base/share/classes/java/math/BigDecimal.java line 3146:
> 3144: * method since the former has [{@code BigInteger},
> 3145: * {@code scale}] components equal to [20, 1] while the latter has
> 3146: * components equals to [200, 2].
s/equals/equal/
-
PR: https://git.openjdk.java.net/jdk/pull/2804
Re: RFR: 8259267: Refactor LoaderLeak shell test as java test. [v14]
On Wed, 3 Mar 2021 15:55:03 GMT, Ivan Šipka wrote: >> Refactor `test/jdk/java/lang/annotation/loaderLeak/LoaderLeak.sh` as java >> test. > > Ivan Šipka has updated the pull request incrementally with one additional > commit since the last revision: > > 8166026: Refactor java/lang shell tests to java LGTM - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1577
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v23]
On Tue, 2 Mar 2021 23:21:28 GMT, David Holmes wrote: > Note that `thread` can be NULL here if the signal handler is running in a > non-attached thread. If we then perform: > `ThreadWXEnable(WXMode new_mode, Thread* thread = NULL) : _thread(thread ? > thread : Thread::current()),` > we call Thread::current() on a non-attached thread and that will assert/crash > if we get NULL. Either avoid using WX when the thread is NULL, or else change > to use Thread::current_or_null_safe() and ensure all uses have a NULL check. https://bugs.openjdk.java.net/browse/JDK-8262903 tracks this issue. - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8259267: Refactor LoaderLeak shell test as java test. [v14]
> Refactor `test/jdk/java/lang/annotation/loaderLeak/LoaderLeak.sh` as java > test. Ivan Šipka has updated the pull request incrementally with one additional commit since the last revision: 8166026: Refactor java/lang shell tests to java - Changes: - all: https://git.openjdk.java.net/jdk/pull/1577/files - new: https://git.openjdk.java.net/jdk/pull/1577/files/80cc5f0b..8cad13df Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1577=13 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1577=12-13 Stats: 8 lines in 1 file changed: 7 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1577.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1577/head:pull/1577 PR: https://git.openjdk.java.net/jdk/pull/1577
Re: RFR: 8173970: jar tool should have a way to extract to a directory
Hi Jaikiran,
It should not be too difficult to support the options listed below via
GNUStyleOptions.
Some other things needed to be defined and agreed upon in order to move forward
* The behavior if the path does not exist
* If the option is specified more than once on the command line
* Clarify the behavior if any of the files exist in the specified target
directory.
Once you have a chance to consider the above, please send your proposal back to
the alias to continue the discussion
Best
Lance
On Mar 3, 2021, at 5:14 AM, Jaikiran Pai
mailto:jai.forums2...@gmail.com>> wrote:
Thank you Lance. So I think there's some level of agreement on using -C and
--dir (or --directory) for the option names.
Anymore opinion on the directory creation semantics and any other aspects to
consider?
-Jaikiran
On 28/02/21 5:38 pm, Lance Andersen wrote:
Hi Jaikiran,
Thank you for this. I went through this myself yesterday in addition to what
you have below here are a few more:
7zip: -o
Info-zip: -d (MacOS version of zip)
Bandzip: -o:{dir}
jpackage: -d —dest
jlink —output
Thinking about this some more, I might suggest supporting
-C
—dir
—directory
Best
Lance
On Feb 27, 2021, at 11:19 PM, Jaikiran Pai
mailto:jai.forums2...@gmail.com>> wrote:
Hello Alan,
On 27/02/21 2:23 pm, Alan Bateman wrote:
Yes, the option name will need to be agreed. It would be useful to enumerate
the options that the other tools are using to specify the location where to
extract. If you see JBS issues mentioning tar -C not supporting chdir when
extracting then it might be Solaris tar, which isn't the same as GNU tar which
has different options. It might be better to look at more main stream tools,
like unzip although jar -d is already taken. It would be nice if there were
some consistency with other tools in the JDK that doing extracting (The jmod
and jimage extract commands use use --dir for example).
I had a look at both tar and unzip commands on MacOS and Linux (CentOS) setup
that I had access to.
--
tar on MacOS:
--
tar --version
bsdtar 3.3.2 - libarchive 3.3.2 zlib/1.2.11 liblzma/5.0.5 bz2lib/1.0.6
The version of this tool has:
-C directory, --cd directory, --directory directory
In c and r mode, this changes the directory before adding the
following files.
In x mode, change directories after opening the archive but before
extracting
entries from the archive.
A command like "tar -xzf foo.tar.gz -C /tmp/bar/" works fine and extracts the
foo.tar.gz from current directory to a target directory /tmp/bar/
--
tar on CentOS:
--
tar --version
tar (GNU tar) 1.26
This version of the tool has:
Common options:
-C, --directory=DIR
change to directory DIR
Although the wording isn't clear that, when used with -x, it extracts to the
directory specified in -C, it does indeed behave that way.
Specifically, a command like "tar -xzf foo.tar.gz -C /tmp/bar/" works fine and
extracts the foo.tar.gz from current directory to a target directory /tmp/bar/
---
unzip on both MacOS and CentOS:
---
unzip -v
UnZip 6.00 of 20 April 2009, by Info-ZIP. Maintained by C. Spieler.
This version of the tool has:
[-d exdir]
An optional directory to which to extract files. By default, all
files and sub-
directories are recreated in the current directory; the -d option
allows extrac-
tion in an arbitrary directory (always assuming one has
permission to write to
the directory). This option need not appear at the end of the
command line; it
is also accepted before the zipfile specification (with the
normal options),
immediately after the zipfile specification, or between the
file(s) and the -x
option. The option and directory may be concatenated without
any white space
between them, but note that this may cause normal shell
behavior to be sup-
pressed. In particular, ``-d ~'' (tilde) is expanded by Unix C
shells into the
name of the user's home directory, but ``-d~'' is treated as a
literal subdirec-
tory ``~'' of the current directory.
unzip foo.zip -d /tmp/bar/ works fine and extracts the foo.zip from current
directory to /tmp/bar/
---
jimage and jmod
---
The jimage and jmod as you note use the --dir option for extracting to that
specified directory.
Those were the tools I looked at. I think using the -d option with -x for the
jar command is ruled out since it already is used for a different purpose,
although for a different "main" operation of the jar command.
As for using --dir for this new feature, I don't think it alone will be enough.
Specifically, I couldn't find a "short form" option for the --dir option in the
jimage or jmod
Re: RFR: JDK-8261518: jpackage looks for main module in current dir when there is no module-path [v2]
On Wed, 3 Mar 2021 00:32:24 GMT, Alexey Semenyuk wrote:
>> Andy Herrick has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> JDK-8261518: jpackage looks for main module in current dir when there is
>> no module-path
>
> test/jdk/tools/jpackage/share/jdk/jpackage/tests/NoMPathRuntimeTest.java line
> 66:
>
>> 64: final Path tmpdir = TKit.createTempDirectory("tmpdir");
>> 65: try {
>> 66: Files.createFile(tmpdir.resolve("tmpfile"));
>
> Is this leftover from something or on purpose?
yes - was leftover - removed
-
PR: https://git.openjdk.java.net/jdk/pull/2781
Re: RFR: JDK-8261518: jpackage looks for main module in current dir when there is no module-path [v4]
> when the app modules have already been jlinked with the runtime, and there is > no need for module-path, jpackage was acting as if the module-path was "." > and picking up jars in the current directory. Andy Herrick has updated the pull request incrementally with one additional commit since the last revision: JDK-8261518: jpackage looks for main module in current dir when there is no module-path - Changes: - all: https://git.openjdk.java.net/jdk/pull/2781/files - new: https://git.openjdk.java.net/jdk/pull/2781/files/a96d962c..fa5b3622 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2781=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2781=02-03 Stats: 6 lines in 1 file changed: 0 ins; 6 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/2781.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2781/head:pull/2781 PR: https://git.openjdk.java.net/jdk/pull/2781
Re: RFR: JDK-8261518: jpackage looks for main module in current dir when there is no module-path [v3]
> when the app modules have already been jlinked with the runtime, and there is > no need for module-path, jpackage was acting as if the module-path was "." > and picking up jars in the current directory. Andy Herrick has updated the pull request incrementally with one additional commit since the last revision: JDK-8261518: jpackage looks for main module in current dir when there is no module-path - Changes: - all: https://git.openjdk.java.net/jdk/pull/2781/files - new: https://git.openjdk.java.net/jdk/pull/2781/files/f800c99f..a96d962c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2781=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2781=01-02 Stats: 18 lines in 1 file changed: 1 ins; 15 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2781.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2781/head:pull/2781 PR: https://git.openjdk.java.net/jdk/pull/2781
Re: RFR: 8262277: java.net.URLClassLoader.getResource throws undocumented IllegalArgumentException
On Sat, 20 Feb 2021 19:20:48 GMT, Craig Andrews
wrote:
> _NOTE_: I've reported this issue at https://bugreport.java.com/ (internal
> review ID : 9069151)
>
> `java.net.URLClassLoader.getResource` can throw an undocumented
> `IllegalArgumentException`.
>
> According to the javadoc for the `getResource` and `findResource` methods,
> neither should be throwing `IllegalArgumentException` - they should return
> null if the resource can't be resolved.
>
> Quoting the javadoc for
> [`URLClassLoader.html#findResource`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/URLClassLoader.html#findResource(java.lang.String))
>> Returns:
>> a URL for the resource, or null if the resource could not be found, or
>> if the loader is closed.
>
> And quoting the javadoc for
> [`ClassLoader.html#getResource`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/ClassLoader.html#getResource(java.lang.String))
>> Returns:
>> URL object for reading the resource; null if the resource could not be
>> found, a URL could not be constructed to locate the resource, the resource
>> is in a package that is not opened unconditionally, or access to the
>> resource is denied by the security manager.
>
> Neither mentions throwing `IllegalArgumentException` and both are clear that
> when URL can't be constructed, `null` should be returned.
>
> Here's a stack trace:
> java.lang.IllegalArgumentException: name
> at
> java.base/jdk.internal.loader.URLClassPath$Loader.findResource(URLClassPath.java:600)
> at
> java.base/jdk.internal.loader.URLClassPath.findResource(URLClassPath.java:291)
> at java.base/java.net.URLClassLoader$2.run(URLClassLoader.java:655)
> at java.base/java.net.URLClassLoader$2.run(URLClassLoader.java:653)
> at java.base/java.security.AccessController.doPrivileged(Native
> Method)
> at
> java.base/java.net.URLClassLoader.findResource(URLClassLoader.java:652)
>
> Looking at
> [`URLClassPath.findResource`](https://github.com/openjdk/jdk/blob/2b00367e1154feb2c05b84a11d62fb5750e46acf/src/java.base/share/classes/jdk/internal/loader/URLClassPath.java#L603)
> URL findResource(final String name, boolean check) {
> URL url;
> try {
> url = new URL(base, ParseUtil.encodePath(name, false));
> } catch (MalformedURLException e) {
> throw new IllegalArgumentException("name");
> }
>
> Instead of throwing `IllegalArgumentException`, that line should simply
> return `null`.
>
> A similar issue exists at
> [`URLClassPath.getResource`](https://github.com/openjdk/jdk/blob/2b00367e1154feb2c05b84a11d62fb5750e46acf/src/java.base/share/classes/jdk/internal/loader/URLClassPath.java#L639)
> URL findResource(final String name, boolean check) {
> URL url;
> try {
> url = new URL(base, ParseUtil.encodePath(name, false));
> } catch (MalformedURLException e) {
> throw new IllegalArgumentException("name");
> }
>
> Instead of throwing `IllegalArgumentException`, that line should simply
> return `null`.
This seems to be a long standing bug, maybe goes all the way back to JDK 1.2.
Are you planning to add a regression test?
-
PR: https://git.openjdk.java.net/jdk/pull/2662
Re: RFR: 8259267: Refactor LoaderLeak shell test as java test. [v13]
> Refactor `test/jdk/java/lang/annotation/loaderLeak/LoaderLeak.sh` as java > test. Ivan Šipka has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: 8166026: Refactor java/lang shell tests to java - Changes: - all: https://git.openjdk.java.net/jdk/pull/1577/files - new: https://git.openjdk.java.net/jdk/pull/1577/files/ad387755..80cc5f0b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1577=12 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1577=11-12 Stats: 9 lines in 1 file changed: 2 ins; 6 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1577.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1577/head:pull/1577 PR: https://git.openjdk.java.net/jdk/pull/1577
Re: RFR: 8262277: java.net.URLClassLoader.getResource throws undocumented IllegalArgumentException
On Sat, 20 Feb 2021 19:22:10 GMT, Craig Andrews
wrote:
>> _NOTE_: I've reported this issue at https://bugreport.java.com/ (internal
>> review ID : 9069151)
>>
>> `java.net.URLClassLoader.getResource` can throw an undocumented
>> `IllegalArgumentException`.
>>
>> According to the javadoc for the `getResource` and `findResource` methods,
>> neither should be throwing `IllegalArgumentException` - they should return
>> null if the resource can't be resolved.
>>
>> Quoting the javadoc for
>> [`URLClassLoader.html#findResource`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/URLClassLoader.html#findResource(java.lang.String))
>>> Returns:
>>> a URL for the resource, or null if the resource could not be found, or
>>> if the loader is closed.
>>
>> And quoting the javadoc for
>> [`ClassLoader.html#getResource`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/ClassLoader.html#getResource(java.lang.String))
>>> Returns:
>>> URL object for reading the resource; null if the resource could not be
>>> found, a URL could not be constructed to locate the resource, the resource
>>> is in a package that is not opened unconditionally, or access to the
>>> resource is denied by the security manager.
>>
>> Neither mentions throwing `IllegalArgumentException` and both are clear that
>> when URL can't be constructed, `null` should be returned.
>>
>> Here's a stack trace:
>> java.lang.IllegalArgumentException: name
>> at
>> java.base/jdk.internal.loader.URLClassPath$Loader.findResource(URLClassPath.java:600)
>> at
>> java.base/jdk.internal.loader.URLClassPath.findResource(URLClassPath.java:291)
>> at java.base/java.net.URLClassLoader$2.run(URLClassLoader.java:655)
>> at java.base/java.net.URLClassLoader$2.run(URLClassLoader.java:653)
>> at java.base/java.security.AccessController.doPrivileged(Native
>> Method)
>> at
>> java.base/java.net.URLClassLoader.findResource(URLClassLoader.java:652)
>>
>> Looking at
>> [`URLClassPath.findResource`](https://github.com/openjdk/jdk/blob/2b00367e1154feb2c05b84a11d62fb5750e46acf/src/java.base/share/classes/jdk/internal/loader/URLClassPath.java#L603)
>> URL findResource(final String name, boolean check) {
>> URL url;
>> try {
>> url = new URL(base, ParseUtil.encodePath(name, false));
>> } catch (MalformedURLException e) {
>> throw new IllegalArgumentException("name");
>> }
>>
>> Instead of throwing `IllegalArgumentException`, that line should simply
>> return `null`.
>>
>> A similar issue exists at
>> [`URLClassPath.getResource`](https://github.com/openjdk/jdk/blob/2b00367e1154feb2c05b84a11d62fb5750e46acf/src/java.base/share/classes/jdk/internal/loader/URLClassPath.java#L639)
>> URL findResource(final String name, boolean check) {
>> URL url;
>> try {
>> url = new URL(base, ParseUtil.encodePath(name, false));
>> } catch (MalformedURLException e) {
>> throw new IllegalArgumentException("name");
>> }
>>
>> Instead of throwing `IllegalArgumentException`, that line should simply
>> return `null`.
>
> Submitted a bug report at https://bugreport.java.com/
> Received internal review ID : 9069151
This one liner will reproduce this issue (you can easily use `jshell` to run it
and see the problem):
new java.net.URLClassLoader(new java.net.URL[]{new
java.net.URL("https://repo1.maven.org/anything-that-ends-with-slash/;)}).findResource("c:/windows");
The key parts to reproduce the issue are:
1. The `URLClassLoader` URL must not start with `file:` or `jar:` and must end
in `/`. See
https://github.com/openjdk/jdk/blob/2b00367e1154feb2c05b84a11d62fb5750e46acf/src/java.base/share/classes/jdk/internal/loader/URLClassPath.java#L493
2. The string passed to `findResource` must not be a valid URL component.
Windows paths (which include a `:` making them invalid URL components) are a
common and easy way to see this error.
-
PR: https://git.openjdk.java.net/jdk/pull/2662
Re: RFR: 8262277: java.net.URLClassLoader.getResource throws undocumented IllegalArgumentException
On Sat, 20 Feb 2021 19:20:48 GMT, Craig Andrews
wrote:
> _NOTE_: I've reported this issue at https://bugreport.java.com/ (internal
> review ID : 9069151)
>
> `java.net.URLClassLoader.getResource` can throw an undocumented
> `IllegalArgumentException`.
>
> According to the javadoc for the `getResource` and `findResource` methods,
> neither should be throwing `IllegalArgumentException` - they should return
> null if the resource can't be resolved.
>
> Quoting the javadoc for
> [`URLClassLoader.html#findResource`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/URLClassLoader.html#findResource(java.lang.String))
>> Returns:
>> a URL for the resource, or null if the resource could not be found, or
>> if the loader is closed.
>
> And quoting the javadoc for
> [`ClassLoader.html#getResource`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/ClassLoader.html#getResource(java.lang.String))
>> Returns:
>> URL object for reading the resource; null if the resource could not be
>> found, a URL could not be constructed to locate the resource, the resource
>> is in a package that is not opened unconditionally, or access to the
>> resource is denied by the security manager.
>
> Neither mentions throwing `IllegalArgumentException` and both are clear that
> when URL can't be constructed, `null` should be returned.
>
> Here's a stack trace:
> java.lang.IllegalArgumentException: name
> at
> java.base/jdk.internal.loader.URLClassPath$Loader.findResource(URLClassPath.java:600)
> at
> java.base/jdk.internal.loader.URLClassPath.findResource(URLClassPath.java:291)
> at java.base/java.net.URLClassLoader$2.run(URLClassLoader.java:655)
> at java.base/java.net.URLClassLoader$2.run(URLClassLoader.java:653)
> at java.base/java.security.AccessController.doPrivileged(Native
> Method)
> at
> java.base/java.net.URLClassLoader.findResource(URLClassLoader.java:652)
>
> Looking at
> [`URLClassPath.findResource`](https://github.com/openjdk/jdk/blob/2b00367e1154feb2c05b84a11d62fb5750e46acf/src/java.base/share/classes/jdk/internal/loader/URLClassPath.java#L603)
> URL findResource(final String name, boolean check) {
> URL url;
> try {
> url = new URL(base, ParseUtil.encodePath(name, false));
> } catch (MalformedURLException e) {
> throw new IllegalArgumentException("name");
> }
>
> Instead of throwing `IllegalArgumentException`, that line should simply
> return `null`.
>
> A similar issue exists at
> [`URLClassPath.getResource`](https://github.com/openjdk/jdk/blob/2b00367e1154feb2c05b84a11d62fb5750e46acf/src/java.base/share/classes/jdk/internal/loader/URLClassPath.java#L639)
> URL findResource(final String name, boolean check) {
> URL url;
> try {
> url = new URL(base, ParseUtil.encodePath(name, false));
> } catch (MalformedURLException e) {
> throw new IllegalArgumentException("name");
> }
>
> Instead of throwing `IllegalArgumentException`, that line should simply
> return `null`.
Submitted a bug report at https://bugreport.java.com/
Received internal review ID : 9069151
-
PR: https://git.openjdk.java.net/jdk/pull/2662
RFR: 8262277: java.net.URLClassLoader.getResource throws undocumented IllegalArgumentException
_NOTE_: I've reported this issue at https://bugreport.java.com/ (internal
review ID : 9069151)
`java.net.URLClassLoader.getResource` can throw an undocumented
`IllegalArgumentException`.
According to the javadoc for the `getResource` and `findResource` methods,
neither should be throwing `IllegalArgumentException` - they should return null
if the resource can't be resolved.
Quoting the javadoc for
[`URLClassLoader.html#findResource`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/URLClassLoader.html#findResource(java.lang.String))
> Returns:
> a URL for the resource, or null if the resource could not be found, or if
> the loader is closed.
And quoting the javadoc for
[`ClassLoader.html#getResource`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/ClassLoader.html#getResource(java.lang.String))
> Returns:
> URL object for reading the resource; null if the resource could not be
> found, a URL could not be constructed to locate the resource, the resource is
> in a package that is not opened unconditionally, or access to the resource is
> denied by the security manager.
Neither mentions throwing `IllegalArgumentException` and both are clear that
when URL can't be constructed, `null` should be returned.
Here's a stack trace:
java.lang.IllegalArgumentException: name
at
java.base/jdk.internal.loader.URLClassPath$Loader.findResource(URLClassPath.java:600)
at
java.base/jdk.internal.loader.URLClassPath.findResource(URLClassPath.java:291)
at java.base/java.net.URLClassLoader$2.run(URLClassLoader.java:655)
at java.base/java.net.URLClassLoader$2.run(URLClassLoader.java:653)
at java.base/java.security.AccessController.doPrivileged(Native Method)
at
java.base/java.net.URLClassLoader.findResource(URLClassLoader.java:652)
Looking at
[`URLClassPath.findResource`](https://github.com/openjdk/jdk/blob/2b00367e1154feb2c05b84a11d62fb5750e46acf/src/java.base/share/classes/jdk/internal/loader/URLClassPath.java#L603)
URL findResource(final String name, boolean check) {
URL url;
try {
url = new URL(base, ParseUtil.encodePath(name, false));
} catch (MalformedURLException e) {
throw new IllegalArgumentException("name");
}
Instead of throwing `IllegalArgumentException`, that line should simply return
`null`.
A similar issue exists at
[`URLClassPath.getResource`](https://github.com/openjdk/jdk/blob/2b00367e1154feb2c05b84a11d62fb5750e46acf/src/java.base/share/classes/jdk/internal/loader/URLClassPath.java#L639)
URL findResource(final String name, boolean check) {
URL url;
try {
url = new URL(base, ParseUtil.encodePath(name, false));
} catch (MalformedURLException e) {
throw new IllegalArgumentException("name");
}
Instead of throwing `IllegalArgumentException`, that line should simply return
`null`.
-
Commit messages:
- 8262277: java.net.URLClassLoader.getResource throws undocumented
IllegalArgumentException
Changes: https://git.openjdk.java.net/jdk/pull/2662/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2662=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8262277
Stats: 4 lines in 1 file changed: 2 ins; 0 del; 2 mod
Patch: https://git.openjdk.java.net/jdk/pull/2662.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2662/head:pull/2662
PR: https://git.openjdk.java.net/jdk/pull/2662
Re: RFR: JDK-8261518: jpackage looks for main module in current dir when there is no module-path [v2]
On Wed, 3 Mar 2021 00:31:34 GMT, Alexey Semenyuk wrote:
>> Andy Herrick has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> JDK-8261518: jpackage looks for main module in current dir when there is
>> no module-path
>
> test/jdk/tools/jpackage/share/jdk/jpackage/tests/NoMPathRuntimeTest.java line
> 125:
>
>> 123: .addArguments("-cvf", "junk.jar",
>> 124: "-C", tmpdir.toString(), "Hello.class")
>> 125: .execute();
>
> Single line `HelloApp.createBundle("junk.jar:Hello", tmpdir);` would compile
> source class and put it into "junk.jar" in `tmpdir` folder. It can be used to
> replace lines from [109, 125] range.
>
> What is the point to build "junk.jar"? I don't see how it is used in the test.
The bug is that when --module-path option is not used in a modular app,
jpackage uses a module-path with "." on it.
Having a non-modular jar in the modular path is an error.
So with this non-modular Hello.jar in the current directory the jpackage
command failed before the fix, and succeeds after the fix.
I can create the non-modular Hello.jar in the current directory with one line:
HelloApp.createBundle(JavaAppDesc.parse("junk.jar:Hello"), Path.of("."))
-
PR: https://git.openjdk.java.net/jdk/pull/2781
Re: Question about java.system.class.loader and the module system
On 03/03/2021 10:43, Volker Simonis wrote: : My question now is if this is an inherent property of the module system or merely an implementation detail? I.e. would it be possible to put the application module into its own layer and initialize that only later when the custom system class loader will be available? I think this would be relatively easy if the custom class loader can be found on the class path. If the custom class loader is in a module itself, that module would have to be in the boot layer to make it accessible to the default system class loader. The ability to set a custom class loader as the system class loader is somewhat legacy feature. There was consideration given to deprecating and removing it a few years ago but we ended up leaving it "as is" in case there are still application servers using it. So it should work the same in JDK 16 as it did in JDK 8/older releases. That is, it will be created with the built-in application class loader as parent and the custom system class loader will be the default parent class loader for delegation. You are correct that an initial module on the application module path will be loaded by the built-in application class loader. All modules on the application module path are mapped to the built-in application class loader. If an initial module is specified to the java launcher (--module or -m) then it just locates the module in the boot layer and invokes its main class. There is no role for a custom system class loader here. If you are looking to deploy a custom system class loader in a module on the module path then it should work as long as you export the package with the custom class loader to java.base. I'm not sure if you really want to do this or whether it's a means to an end. Maybe it would be easier to start with a summary on what you are looking it do? Are you looking to intercept all attempts to load classes? Is there a java agent in the picture too? The current API documentation of the system class loader [1] mentions that the system class loader "is typically the class loader used to start the application". Shouldn't that be updated to mention that this will not be the case for modular applications? The javadoc is correct because it will be rare to deploy with java.system.class.loader set to a custom class loader. Assuming it is not set then you should find that the initial class will be defined by this class loader. This goes for both the class path and module path cases. -Alan
Question about java.system.class.loader and the module system
Hi, I have a question about how changing the system class loader by setting "java.system.class.loader" interacts with the module system. I couldn't find a lot of information on this topic but if it has been discussed before please point me to the corresponding place. Traditionally, setting "java.system.class.loader" to the name of a class loader will instruct the JVM to use that class loader as the system class loader which will be used to load the application main class. But this class loader has to be loaded itself by a class loader. So the JVM defines its own "default system class loader" which will be used to load the custom system class loader and which will act as delegation parent for it [1]. By writing a custom class loader which intercepts calls to loadClass() and defining it as the system class loader by setting "java.system.class.loader" it can be easily verified that the JVM will load the applications main class with that custom class loader. This behaviour changes however if we use the new module syntax "-m /" to start an application. In that case, the main application class will always be loaded by the default system class loader, no matter if we define "java.system.class.loader" or not. >From what I've found by looking into the sources, this is because the application module will be added to the boot layer and associated with the default system class loader early in the JVM boot process (i.e. "initPhase2") when only classes from java.base can be loaded and the custom class loader is therefore not available. The system class loader is only initialized later in "initiPhase3" at which it seems to be already to late to associate it with the application module (is this really the case?). My question now is if this is an inherent property of the module system or merely an implementation detail? I.e. would it be possible to put the application module into its own layer and initialize that only later when the custom system class loader will be available? I think this would be relatively easy if the custom class loader can be found on the class path. If the custom class loader is in a module itself, that module would have to be in the boot layer to make it accessible to the default system class loader. The current API documentation of the system class loader [1] mentions that the system class loader "is typically the class loader used to start the application". Shouldn't that be updated to mention that this will not be the case for modular applications? Thank you and best regards, Volker [1] https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/ClassLoader.html#getSystemClassLoader()
Re: RFR: 8173970: jar tool should have a way to extract to a directory
Thank you Lance. So I think there's some level of agreement on using -C
and --dir (or --directory) for the option names.
Anymore opinion on the directory creation semantics and any other
aspects to consider?
-Jaikiran
On 28/02/21 5:38 pm, Lance Andersen wrote:
Hi Jaikiran,
Thank you for this. I went through this myself yesterday in addition
to what you have below here are a few more:
7zip: -o
Info-zip: -d (MacOS version of zip)
Bandzip: -o:{dir}
jpackage: -d —dest
jlink —output
Thinking about this some more, I might suggest supporting
-C
—dir
—directory
Best
Lance
On Feb 27, 2021, at 11:19 PM, Jaikiran Pai > wrote:
Hello Alan,
On 27/02/21 2:23 pm, Alan Bateman wrote:
Yes, the option name will need to be agreed. It would be useful to
enumerate the options that the other tools are using to specify the
location where to extract. If you see JBS issues mentioning tar -C
not supporting chdir when extracting then it might be Solaris tar,
which isn't the same as GNU tar which has different options. It
might be better to look at more main stream tools, like unzip
although jar -d is already taken. It would be nice if there were
some consistency with other tools in the JDK that doing extracting
(The jmod and jimage extract commands use use --dir for example).
I had a look at both tar and unzip commands on MacOS and Linux
(CentOS) setup that I had access to.
--
tar on MacOS:
--
tar --version
bsdtar 3.3.2 - libarchive 3.3.2 zlib/1.2.11 liblzma/5.0.5 bz2lib/1.0.6
The version of this tool has:
-C directory, --cd directory, --directory directory
In c and r mode, this changes the directory before
adding the following files.
In x mode, change directories after opening the archive
but before extracting
entries from the archive.
A command like "tar -xzf foo.tar.gz -C /tmp/bar/" works fine and
extracts the foo.tar.gz from current directory to a target directory
/tmp/bar/
--
tar on CentOS:
--
tar --version
tar (GNU tar) 1.26
This version of the tool has:
Common options:
-C, --directory=DIR
change to directory DIR
Although the wording isn't clear that, when used with -x, it extracts
to the directory specified in -C, it does indeed behave that way.
Specifically, a command like "tar -xzf foo.tar.gz -C /tmp/bar/" works
fine and extracts the foo.tar.gz from current directory to a target
directory /tmp/bar/
---
unzip on both MacOS and CentOS:
---
unzip -v
UnZip 6.00 of 20 April 2009, by Info-ZIP. Maintained by C. Spieler.
This version of the tool has:
[-d exdir]
An optional directory to which to extract files. By
default, all files and sub-
directories are recreated in the current directory; the
-d option allows extrac-
tion in an arbitrary directory (always assuming one has
permission to write to
the directory). This option need not appear at the
end of the command line; it
is also accepted before the zipfile specification
(with the normal options),
immediately after the zipfile specification, or
between the file(s) and the -x
option. The option and directory may be concatenated
without any white space
between them, but note that this may cause normal
shell behavior to be sup-
pressed. In particular, ``-d ~'' (tilde) is expanded
by Unix C shells into the
name of the user's home directory, but ``-d~'' is
treated as a literal subdirec-
tory ``~'' of the current directory.
unzip foo.zip -d /tmp/bar/ works fine and extracts the foo.zip from
current directory to /tmp/bar/
---
jimage and jmod
---
The jimage and jmod as you note use the --dir option for extracting
to that specified directory.
Those were the tools I looked at. I think using the -d option with -x
for the jar command is ruled out since it already is used for a
different purpose, although for a different "main" operation of the
jar command.
As for using --dir for this new feature, I don't think it alone will
be enough. Specifically, I couldn't find a "short form" option for
the --dir option in the jimage or jmod commands. For the jar extract
feature that we are discussing here, I think having a short form
option (in addition to the longer form) is necessary to have it match
the usage expectations of similar other options that the jar command
exposes. So even if we do choose --dir as the long form option, we
would still need a short form for it and since -d is already taken
for something else, we would still need to come up with a different
one. The short form of this option could be -C (see below).
I think reusing the -C option, for this new feature, perhaps is a
good thing. The -C
