Skip to content

Conversation

@magicus
Copy link
Member

@magicus magicus commented Sep 17, 2025

The only remaining shell script that is being used in the gensrc phase to generate Java code is for the nio exceptions. This should be removed as well, and replaced with a standard solution (Java buildtool or makefile API calls).


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Integration blocker

 ⚠️ Title mismatch between PR and JBS for issue JDK-8367859

Issue

  • JDK-8367859: Remove nio exception gensrc (Enhancement - P3) ⚠️ Title mismatch between PR and JBS.

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27338/head:pull/27338
$ git checkout pull/27338

Update a local copy of the PR:
$ git checkout pull/27338
$ git pull https://git.openjdk.org/jdk.git pull/27338/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27338

View PR using the GUI difftool:
$ git pr show -t 27338

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27338.diff

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 17, 2025

👋 Welcome back ihse! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Sep 17, 2025

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

@openjdk
Copy link

openjdk bot commented Sep 17, 2025

@magicus The following labels will be automatically applied to this pull request:

  • build
  • core-libs
  • nio

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added build build-dev@openjdk.org nio nio-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Sep 17, 2025
@magicus
Copy link
Member Author

magicus commented Sep 17, 2025

Some implementation notes:

I considered writing a Java buildtool to read the existing exceptions files, but that seemed like a lot of work to keep up with a format that was not really ideal anyway, but was in fact a shell script snippet, disguised as a data file.

The *.java.template method aligns better with most other gensrc solutions. This also makes it easier to get a better understanding of how the resulting file will look. As a matter of fact, the generated file with the old solution did not match up with how our files should look. I modified the template to follow this pattern. As an effect of this, all generated files has a diff (in whitespace or comments only) with this code compared to the old, like this:

--- ReadOnlyBufferException.java        2025-09-11 11:55:45
+++ NEW/ReadOnlyBufferException.java    2025-09-12 17:40:57
@@ -1,6 +1,5 @@
 /*
- * Copyright (c) 2000, 2021, Oracle and/or its affiliates. All rights reserved.
- *
+ * Copyright (c) 2000, 2025, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -22,14 +21,10 @@
  * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
  * or visit www.oracle.com if you need additional information or have any
  * questions.
- *
  */

-// -- This file was mechanically generated: Do not edit! -- //
-
 package java.nio;

-
 /**
  * Unchecked exception thrown when a content-mutation method such as
  * <code>put</code> or <code>compact</code> is invoked upon a read-only buffer.
@@ -48,5 +43,4 @@
      * Constructs an instance of this class.
      */
     public ReadOnlyBufferException() { }
-
 }

I consider this an improvement, not a bug.

I also discovered a real bug. Due to how the old exceptions files were actually shell script snippets that were included and executed, proper care needed to be taken in how to handle quotes. This was not done for IllegalCharsetNameException.java. The " were eaten by the shell, so the generated Java code was:

 * <a href=Charset.html#names>legal charset name</a> is used as such.

instead of (the intended, and correct)

 * <a href="Charset.html#names">legal charset name</a> is used as such.

That is fixed with this PR.

Apart from these changes, the generated files are identical before and after this PR.

@magicus
Copy link
Member Author

magicus commented Sep 17, 2025

I don't understand what is going on with GHA. It works perfectly well on my local computer and Oracle's internal CI system.

@magicus
Copy link
Member Author

magicus commented Sep 17, 2025

I don't understand what is going on with GHA. It works perfectly well on my local computer and Oracle's internal CI system.

The answer to that is "GNU Make 4.3 is going on". I have a tentative fix...

@magicus magicus marked this pull request as ready for review September 17, 2025 15:09
@openjdk openjdk bot added the rfr Pull request is ready for review label Sep 17, 2025
@mlbridge
Copy link

mlbridge bot commented Sep 17, 2025

Webrevs

@AlanBateman
Copy link
Contributor

AlanBateman commented Sep 17, 2025

I really dislike this. Can you please look at putting a template in java.nio.channels and java.nio.charsets for the exceptions in those packages? There will be a naming discussion to have on this too. Same thing for CharsetNameExceptions.java.template.

Alternatively, maybe we should just check in the exceptions in the src tree and avoid this. @naotoj @bplb, can you provide some opinion on this?

@AlanBateman
Copy link
Contributor

/reviewers 2 reviewer

@openjdk
Copy link

openjdk bot commented Sep 17, 2025

@AlanBateman
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

@naotoj
Copy link
Member

naotoj commented Sep 17, 2025

Alternatively, maybe we should just check into the exceptions in the src tree and avoid this. @naotoj @bplb, can you provide some opinion on this?

I agree. Since those exceptions are not likely to change, It would be easier to maintain just to check-in those generated sources into the repository.

@magicus
Copy link
Member Author

magicus commented Sep 17, 2025

I'm glad to hear you say that. That was my initial thinking also, but since it was not done that way originally, I thought it would not be acceptable. I think that is a much better solution. I'll create a new PR with checked-in code, but I'll keep this open for the time being, if it turns out a generated solution is preferred.

@magicus
Copy link
Member Author

magicus commented Sep 17, 2025

See #27352.

@bplb
Copy link
Member

bplb commented Sep 17, 2025

Alternatively, maybe we should just check into the exceptions in the src tree and avoid this. @naotoj @bplb, can you provide some opinion on this?

I agree. Since those exceptions are not likely to change, It would be easier to maintain just to check-in those generated sources into the repository.

I concur. Given that the only change since JDK 8 is this

59c59
<  * <tt>put</tt> or <tt>compact</tt> is invoked upon a read-only buffer." \
---
>  * <code>put</code> or <code>compact</code> is invoked upon a read-only buffer." \

these are pretty stable.

@magicus magicus closed this Sep 19, 2025
@magicus magicus deleted the remove-genExceptions.sh branch September 19, 2025 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build build-dev@openjdk.org core-libs core-libs-dev@openjdk.org nio nio-dev@openjdk.org rfr Pull request is ready for review

Development

Successfully merging this pull request may close these issues.

4 participants