8334733: Remove obsolete @enablePreview from tests after JDK-8334714 #22420

liach · 2024-11-27T23:10:15Z

Remove the redundant @enablePreview and --enable-preview flags for enabling ClassFile API in the tests. The remainder of these flags in all tests seem to serve preview APIs (such as ScopedValue) or language features (primitive pattern, module imports, etc.), or testing the enable preview flag itself. Now there is fewer than 100 @enablePreview in the test directory.

To reviewers, there are some redundant changes and notes:

There's one security test that used ModuleInfoWriter that depends on ClassFile API.
Removed unnecessary exports of jdk.internal.classfile.impl. Remaining uses are:
- BoundAttribute::payloadLen for javac attribute tests
- Annotation reading/writing for javac annotation tests
Line number changes to:
- test/langtools/tools/javac/linenumbers/NestedLineNumberTest.java
- test/langtools/tools/javac/linenumbers/NullCheckLineNumberTest.java
Move from legacy jdk.internal.classfile to java.lang.classfile in:
- test/langtools/tools/javac/NoStringToLower.java and
- test/langtools/tools/javac/T8003967/DetectMutableStaticFields.java
Weird annotation processor behavior in test/langtools/tools/javac/annotations/parameter/ParameterAnnotations.java
- Without preview and using explicit file name, the javac task fails; using the builder live AP object works both with and without preview.

Testing: tier 1-5. Please inform me if any of these tests belong to higher tiers.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8334733: Remove obsolete @enablePreview from tests after JDK-8334714 (Task - P4)

Reviewers

Mandy Chung (@mlchung - Reviewer)
Adam Sotona (@asotona - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22420

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-11-27T23:11:12Z

👋 Welcome back liach! 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 · 2024-11-27T23:11:50Z

@liach This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8334733: Remove obsolete @enablePreview from tests after JDK-8334714

Reviewed-by: mchung, asotona

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 9 new commits pushed to the master branch:

2286fae: 8345159: RISCV: Fix -Wzero-as-null-pointer-constant warning in emit_static_call_stub
8403285: 8268145: [macos] Rendering artifacts is seen when text inside the JTable with TableCellEditor having JTextfield
aa38284: 8345435: Eliminate tier1_compiler_not_xcomp group
9284602: 8345628: [BACKOUT] JDK-8287122 Use gcc12 -ftrivial-auto-var-init=pattern in debug builds
41c8971: 8287122: Use gcc12 -ftrivial-auto-var-init=pattern in debug builds
5da0eee: 8285692: Enable _FORTIFY_SOURCE=2 when building with Clang
daa2ba5: 8339622: Regression in make open-hotspot-xcode-project
6f6bce5: 8344559: Log is spammed by missing pandoc warnings when building man pages
5f30a8d: 8345424: Move FindDebuginfoFiles out of FileUtils.gmk

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-11-27T23:12:32Z

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

compiler
core-libs
hotspot
i18n
javadoc
security
serviceability

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.

mlbridge · 2024-11-27T23:19:23Z

Webrevs

01: Full (87dce967)
00: Full (064520e8)

asotona · 2024-11-28T07:14:12Z

I would give a go to all trivial removals of @enablePreview after a brief check the tests pass.
However all the more complex changes potentially affecting various parts of the JDK infrastructure require deeper review and probably involve more reviewers.
My recommendation for easier review would be to split this 360-files patch into a patch with trivial removals of @enablePreview and one or more patches focused on particular area.

liach · 2024-11-28T07:45:05Z

I think the simple removal of @enablePreview already require multiple areas of engineers, so to involve a bit more changes to fix them throughoutly is fine; these are all test-only, so we can wait and this can go in after RDP1 began.

openjdk · 2024-12-05T17:12:32Z

@liach this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout cleanup/cf-preview
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

…cf-preview

mlchung

The change looks okay to me except ParameterAnnotations.java. This task is part of JEP 484 and test-only. It seems good to backport to 24.

mlchung · 2024-12-05T21:54:19Z

test/langtools/tools/javac/annotations/parameter/ParameterAnnotations.java

@@ -641,8 +640,8 @@ private void doTest(Path base, String code, String binaryNameToCheck,
        }

        Task.Result result = new JavacTask(tb)
+                .processors(new TestAP())


I assume this fix does not require this change, right? If so, better to keep the original code and do this cleanup as a separate issue.

It is required. If we use the command line and FQN to specify a processor like in the current code without preview, this test fails with some classpath error.

Weird annotation processor behavior in test/langtools/tools/javac/annotations/parameter/ParameterAnnotations.java

Without preview and using explicit file name, the javac task fails; using the builder live AP object works both with and without preview.

I should attach the exact error message:

test: testInnerClass [DIRECT]: - compiler.err.proc.processor.not.found: ParameterAnnotations$TestAP - compiler.err.proc.no.explicit.annotation.processing.requested: T$I 2 errors Exception running test testInnerClass: toolbox.Task$TaskError: Task javac failed: rc=1 toolbox.Task$TaskError: Task javac failed: rc=1 at toolbox.AbstractTask.checkExit(AbstractTask.java:154) at toolbox.JavacTask.run(JavacTask.java:381) at toolbox.AbstractTask.run(AbstractTask.java:102) at toolbox.JavacTask.run(JavacTask.java:52) at toolbox.JavacTask.run(JavacTask.java:321) at ParameterAnnotations.doTest(ParameterAnnotations.java:650) at ParameterAnnotations.testInnerClass(ParameterAnnotations.java:151) at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104) at java.base/java.lang.reflect.Method.invoke(Method.java:565) at toolbox.TestRunner.runTests(TestRunner.java:91) at ParameterAnnotations.runTests(ParameterAnnotations.java:82) at ParameterAnnotations.main(ParameterAnnotations.java:73) at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104) at java.base/java.lang.reflect.Method.invoke(Method.java:565) at com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:333) at java.base/java.lang.Thread.run(Thread.java:1447)

Same for all 5 tests in this file.

Filed https://bugs.openjdk.org/browse/JDK-8345622. There is no similar issues that happen as a conjunction of preview and annotation processing.

mlchung

Thanks for filing the issue to follow up.

asotona

Looks good to me, thanks.

liach · 2024-12-06T14:24:28Z

Thanks for the reviews!

/integrate

openjdk · 2024-12-06T14:25:06Z

Going to push as commit 4966419.
Since your change was applied there have been 13 commits pushed to the master branch:

0e2a285: 8344833: CTW: Make failing on zero classes optional
f6021a9: 8344668: Unnecessary array allocations and copying in TextLine
874d68a: 8343747: C2: TestReplicateAtConv.java crashes with -XX:MaxVectorSize=8
d9a2213: 8345299: C2: some nodes can still have incorrect control after do_range_check()
2286fae: 8345159: RISCV: Fix -Wzero-as-null-pointer-constant warning in emit_static_call_stub
8403285: 8268145: [macos] Rendering artifacts is seen when text inside the JTable with TableCellEditor having JTextfield
aa38284: 8345435: Eliminate tier1_compiler_not_xcomp group
9284602: 8345628: [BACKOUT] JDK-8287122 Use gcc12 -ftrivial-auto-var-init=pattern in debug builds
41c8971: 8287122: Use gcc12 -ftrivial-auto-var-init=pattern in debug builds
5da0eee: 8285692: Enable _FORTIFY_SOURCE=2 when building with Clang
... and 3 more: https://git.openjdk.org/jdk/compare/bf0debc023a42ccdf2f589039e4d98e11424b4dd...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-12-06T14:25:15Z

@liach Pushed as commit 4966419.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

liach · 2024-12-06T15:06:19Z

/backport :jdk24

From internal discussion, this task is logically associated with the finalization of ClassFile API and is best done for the same release 24.

openjdk · 2024-12-06T15:07:57Z

@liach the backport was successfully created on the branch backport-liach-49664195-jdk24 in my personal fork of openjdk/jdk. To create a pull request with this backport targeting openjdk/jdk:jdk24, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 49664195 from the openjdk/jdk repository.

The commit being backported was authored by Chen Liang on 6 Dec 2024 and was reviewed by Mandy Chung and Adam Sotona.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk:

$ git fetch https://github.com/openjdk-bots/jdk.git backport-liach-49664195-jdk24:backport-liach-49664195-jdk24
$ git checkout backport-liach-49664195-jdk24
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk.git backport-liach-49664195-jdk24

8334733: Remove obsolete @enablePreview from tests after JDK-83324714

064520e

openjdk bot added the rfr Pull request is ready for review label Nov 27, 2024

openjdk bot added javadoc javadoc-dev@openjdk.org serviceability serviceability-dev@openjdk.org security security-dev@openjdk.org hotspot hotspot-dev@openjdk.org core-libs core-libs-dev@openjdk.org compiler compiler-dev@openjdk.org i18n i18n-dev@openjdk.org labels Nov 27, 2024

liach changed the title ~~8334733: Remove obsolete @enablePreview from tests after JDK-83324714~~ 8334733: Remove obsolete @enablePreview from tests after JDK-8334714 Nov 28, 2024

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Dec 5, 2024

Merge branch 'master' of https://github.com/openjdk/jdk into cleanup/…

87dce96

…cf-preview

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Dec 5, 2024

mlchung reviewed Dec 5, 2024

View reviewed changes

mlchung approved these changes Dec 5, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 5, 2024

asotona approved these changes Dec 6, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Dec 6, 2024

openjdk bot closed this Dec 6, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 6, 2024

MaxXSoft mentioned this pull request Dec 24, 2024

8346773: Fix unmatched brackets in some misc files #22861

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8334733: Remove obsolete @enablePreview from tests after JDK-8334714 #22420

8334733: Remove obsolete @enablePreview from tests after JDK-8334714 #22420

liach commented Nov 27, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 27, 2024

openjdk bot commented Nov 27, 2024 •

edited

Loading

openjdk bot commented Nov 27, 2024

mlbridge bot commented Nov 27, 2024 •

edited

Loading

asotona commented Nov 28, 2024

liach commented Nov 28, 2024

openjdk bot commented Dec 5, 2024

mlchung left a comment

mlchung Dec 5, 2024

liach Dec 5, 2024 •

edited

Loading

liach Dec 5, 2024

mlchung left a comment

asotona left a comment

liach commented Dec 6, 2024

openjdk bot commented Dec 6, 2024

openjdk bot commented Dec 6, 2024

liach commented Dec 6, 2024

openjdk bot commented Dec 6, 2024

8334733: Remove obsolete @enablePreview from tests after JDK-8334714 #22420

8334733: Remove obsolete @enablePreview from tests after JDK-8334714 #22420

Conversation

liach commented Nov 27, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 27, 2024

openjdk bot commented Nov 27, 2024 • edited Loading

openjdk bot commented Nov 27, 2024

mlbridge bot commented Nov 27, 2024 • edited Loading

Webrevs

asotona commented Nov 28, 2024

liach commented Nov 28, 2024

openjdk bot commented Dec 5, 2024

mlchung left a comment

Choose a reason for hiding this comment

mlchung Dec 5, 2024

Choose a reason for hiding this comment

liach Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

liach Dec 5, 2024

Choose a reason for hiding this comment

mlchung left a comment

Choose a reason for hiding this comment

asotona left a comment

Choose a reason for hiding this comment

liach commented Dec 6, 2024

openjdk bot commented Dec 6, 2024

openjdk bot commented Dec 6, 2024

liach commented Dec 6, 2024

openjdk bot commented Dec 6, 2024

liach commented Nov 27, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Nov 27, 2024 •

edited

Loading

mlbridge bot commented Nov 27, 2024 •

edited

Loading

liach Dec 5, 2024 •

edited

Loading