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.

Sign up for GitHub

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

Jump to bottom

move java modules job to JDK 11 and fix tests where needed #5138

Merged
merged 1 commit into from
Jan 10, 2023
Merged

move java modules job to JDK 11 and fix tests where needed #5138

merged 1 commit into from
Jan 10, 2023

Conversation

mbien
Copy link
Member

@mbien mbien commented Dec 23, 2022 •
edited
Loading

move java modules job to JDK 11 and fix tests where needed.

  • put java module system flags into project properties instead of CI config.
    • allows running the tests out-of-the-box from IDE
    • has the benefit of not overriding other JVM flag defaults for CI
  • move whole job from 8 to 11 and remove (now) redundant steps which reduces max job time by >30mins
  • ModuleTest: fix JRTFS module root for JDK 9-12.
  • remove empty ConstantPoolTest

meta issue #4904

@mbien mbien added Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) CI continuous integration changes tests labels Dec 23, 2022
@mbien mbien force-pushed the java-modules-to-11 branch from 809a07e to 4cdfe38 Compare December 23, 2022 20:56
@mbien mbien changed the title move Java modules job to 11 move java modules job to JDK 11 and fix tests where needed Dec 23, 2022
@mbien mbien added this to the NB17 milestone Dec 23, 2022
@mbien mbien mentioned this pull request Dec 23, 2022
Closed
@mbien mbien marked this pull request as ready for review December 23, 2022 21:44
@mbien mbien requested a review from jlahoda December 23, 2022 21:44
mbien
mbien commented Dec 23, 2022
View reviewed changes
java/java.completion/nbproject/project.properties
Comment on lines +35 to +38
test.jms.flags=--limit-modules=java.base,java.logging,java.xml,java.prefs,java.desktop,java.management,java.instrument,jdk.zipfs,java.scripting,java.naming
test.bootclasspath.prepend.args=-Dno.netbeans.bootclasspath.prepend.needed=true
Copy link
Member Author

@mbien mbien Dec 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

this has almost the same effect as the old CI config line:

OPTS_11=-Dtest.run.args=--limit-modules=java.base,java.logging,java.xml,java.prefs,java.desktop,java.management,java.instrument,jdk.zipfs,java.scripting,java.naming -Dtest.bootclasspath.prepend.args=-Dno.netbeans.bootclasspath.prepend.needed=true

the difference is that the entire run args property isn't overwritten which is set up here:

netbeans/nbbuild/templates/projectized.xml

Line 293 in 30a8cef

<property name="test.run.args" value="-ea -Xms1200m -Xmx1200m -XX:+UseParallelGC ${metabuild.jms-flags.jvm} ${test.jms.flags} -XX:+IgnoreUnrecognizedVMOptions"/>

this has however the side effect of warnings like:
WARNING: Unknown module: jdk.compiler specified to --add-exports

Copy link
Member Author

@mbien mbien Dec 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

actually, i got an idea while waiting for Santa.

we can simply unset the default flags of the compiler category:

# remove default compiler JMS flags so that we don't get "Unknown module" warnings due to --limit-modules
jms-compiler.flags.jvm=

since we split them into categories already (#3658) we may as well make use of that fact :)

@mbien
move java modules job to JDK 11 and fix tests where needed.
26e47e1 
 - put java module system flags into project properties instead of CI config.
   - allows running the tests out-of-the-box from IDE
   - has the benefit of not overriding other JVM flag defaults for CI
 - move whole job from 8 to 11 and remove (now) redundant steps
 - ModuleTest: fix JRTFS module root for JDK 9-12.
 - remove empty ConstantPoolTest
@mbien mbien force-pushed the java-modules-to-11 branch from 4cdfe38 to 26e47e1 Compare December 24, 2022 18:27
@mbien
Copy link
Member Author

mbien commented Dec 31, 2022

@jlahoda would be good if you could take a look at this one.

@mbien
Copy link
Member Author

mbien commented Jan 7, 2023

@neilcsmith-net I have this one set for NB17 since this reduces the longest job by about 30mins, since releasing does require a lot of building I thought it would be good to get this one in.

@mbien mbien mentioned this pull request Jan 9, 2023
Merged
@mbien
Copy link
Member Author

mbien commented Jan 10, 2023

thanks for the review. Lets get this in.

@mbien mbien merged commit 19fa823 into apache:master Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthiasblaesing matthiasblaesing matthiasblaesing approved these changes

@jlahoda jlahoda Awaiting requested review from jlahoda

@neilcsmith-net neilcsmith-net Awaiting requested review from neilcsmith-net

@sdedic sdedic Awaiting requested review from sdedic

Assignees
No one assigned
Labels
CI continuous integration changes Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) tests
Projects
None yet
Milestone
NB17
Development

Successfully merging this pull request may close these issues.
2 participants
@mbien @matthiasblaesing