Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK 20 preparations #5122

Merged
merged 1 commit into from
Dec 21, 2022
Merged

JDK 20 preparations #5122

merged 1 commit into from
Dec 21, 2022

Conversation

mbien
Copy link
Member

@mbien mbien commented Dec 19, 2022

add JDK 20ea to build/test matrix

use 1.8 source/target as new baseline since JDK 20 removed support for 1.7 (14 removed support for 1.6).

had to add -Xlint:-options to modules which use -Werror since the compiler would print:

   [repeat] warning: [options] source value 8 is obsolete and will be removed in a future release
   [repeat] warning: [options] target value 8 is obsolete and will be removed in a future release
   [repeat] warning: [options] To suppress warnings about obsolete options, use -Xlint:-options.
   [repeat] error: warnings found and -Werror specified

It is unclear why since this should be release 8. nbjavac should not set source/target anymore (unless bootclasspath is set too).

tbh I would like to move source/target=1.8 to release=8 to switch to the new model without all the nb specific mapping rules (e.g #3278) in follow ups.

@mbien mbien added Need Squashing CI Continuous Integration build labels Dec 19, 2022
@mbien mbien added this to the NB17 milestone Dec 19, 2022
@mbien mbien force-pushed the seven-no-more branch 2 times, most recently from 7c1389f to d32d693 Compare December 19, 2022 06:02
@@ -170,7 +170,6 @@ jobs:
distribution: ${{ env.default_java_distribution }}

- name: Setup Xvfb
if: ${{ matrix.java != '20-ea' }} # see #4299
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this problem disappeared for some reason, which is a good thing

@mbien mbien marked this pull request as ready for review December 19, 2022 16:24
<javac srcdir="build/external-patch/sources"
destdir="build/external-patch/classes" source="${javac.source}" target="${javac.target}">
<javac srcdir="build/external-patch/sources" destdir="build/external-patch/classes" release="8">
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks weird but I hope that we can do a simple search and replace from
release="8" to
release="${javac.release}" in future once we change the format to feature version (instead of 1.x).

I also would like to set the property by default to 8 and remove it from all module project.properties so that it can be managed centrally. Modules can still set it if they diverge from the default, but they don't have to set the default.

@mbien
Copy link
Member Author

mbien commented Dec 20, 2022

to make sure that PRs like this here don't cause unexpected issues by compiling on the wrong bytecode level I run this script:

#!/bin/bash
set -e

max=8

for c in `find . -name "*.class" -type f`; do

    info=$(file "$c")
    version=$(echo $info | cut -d " " -f 7)

    if [[ "$version" == "" ]]; then
        echo $info
        continue
    fi

    major=$(echo $version | cut -d "." -f 1)
    minor=$(echo $version | cut -d "." -f 2)

    if (( $major > 44+$max )); then
        echo $info
        continue
    fi
    if (( $minor != 0 )); then
        echo $info
    fi

done

sort the output and diff before/after PR. It had no changes here.

it creates output like:
./java/java.disco/build/classes/org/netbeans/modules/java/disco/WizardState.class: compiled Java class data, version 55.0 (Java SE 11)
for every class > 8+44 bytecode level

Copy link
Contributor

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks sane to me. I had some doubts regarding profiler/lib.profiler as the code is loaded into the target VM. However I looked at the generated classes before your change and it is already 8, so there is no regression.

Regarding switching to --release=8: If I remember correctly, this might not work for all modules, when internal classes are accessed. I agree, that it would be good to do, but it might not work.

Regarding warning about --release=8 triggering --source and --target warnings: I expect support for release=8 to be removed at the same time source=8 and target=8 are removed. Everything else makes no sense from my perpective, as the argument to remove support for old source and targets is based on making it easier to maintain javac, which would be invalidated, if you need to retain the code paths to support release

@mbien
Copy link
Member Author

mbien commented Dec 20, 2022

I expect support for release=8 to be removed at the same time source=8 and target=8 are removed. Everything else makes no sense from my perpective,

@matthiasblaesing agreed. my guess is that the warning isn't about the value 8 even though it explicitly says value, it is about source/target itself. Since release is the new best practice which is stricter and safer (unless you have to specify bootcp, then you are forced to stick to source/target, but then javac probably links against the bootcp so that would be safe too I believe).

edit: i was wrong the warning is only printed for the value 8, 9+ wouldn't print it anymore

$ /home/mbien/dev/java/jdk-20-ea+28/bin/javac -source 8 -target 8 src/main/java/dev/mbien/mavenproject1/Mavenproject1.java 
warning: [options] bootstrap class path not set in conjunction with -source 8
warning: [options] source value 8 is obsolete and will be removed in a future release
warning: [options] target value 8 is obsolete and will be removed in a future release
warning: [options] To suppress warnings about obsolete options, use -Xlint:-options.
4 warnings

$ /home/mbien/dev/java/jdk-20-ea+28/bin/javac -source 9 -target 9 src/main/java/dev/mbien/mavenproject1/Mavenproject1.java 
warning: [options] system modules path not set in conjunction with -source 9
1 warning

it is also printed for release:

$ /home/mbien/dev/java/jdk-20-ea+28/bin/javac --release 8 src/main/java/dev/mbien/mavenproject1/Mavenproject1.java 
warning: [options] source value 8 is obsolete and will be removed in a future release
warning: [options] target value 8 is obsolete and will be removed in a future release
warning: [options] To suppress warnings about obsolete options, use -Xlint:-options.
3 warnings

 - use -Xlint:-options due to obsolete source/target warning and -Werror
 - fix some javac warnings which fail due to -Werror in SuiteSources.java
 - add JDK 20ea to test matrix
@mbien
Copy link
Member Author

mbien commented Dec 20, 2022

squashed everything. No other changes.

going to merge tomorrow once green again.

@mbien mbien added the ci:all-tests [ci] enable all tests label Dec 21, 2022
@apache apache locked and limited conversation to collaborators Dec 21, 2022
@apache apache unlocked this conversation Dec 21, 2022
@mbien mbien changed the title JDK 20 preperations JDK 20 preparations Dec 21, 2022
@mbien mbien merged commit 133c14a into apache:master Dec 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build ci:all-tests [ci] enable all tests CI Continuous Integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants