Conversation
mbien
added
CI
mbien
force-pushed
the
ci-bump-lower-bound-21
branch
7 times, most recently
from
e958a12 to
0e4b268
Compare
| js.runUserActionTask(new Task<CompilationController>() { | ||
| public void run(CompilationController parameter) throws Exception { | ||
| parameter.toPhase(Phase.RESOLVED); | ||
|
|
||
| TypeElement string = parameter.getElements().getTypeElement("test.test2"); | ||
|
|
||
| SecurityManager old = System.getSecurityManager(); | ||
|
|
||
| System.setSecurityManager(new SecMan()); | ||
|
|
||
| TreePathHandle.create(string, parameter); | ||
|
|
||
| System.setSecurityManager(old); | ||
| AtomicBoolean wasRead = new AtomicBoolean(); | ||
|
|
||
| js.runUserActionTask(controller -> { | ||
| controller.toPhase(Phase.RESOLVED); | ||
|
|
||
| TypeElement string = controller.getElements().getTypeElement("test.test2"); | ||
|
|
||
| try (RecordingStream rs = new RecordingStream()) { | ||
| rs.setMaxSize(Long.MAX_VALUE); | ||
| rs.enable("jdk.FileRead").withoutThreshold().withStackTrace(); | ||
| rs.onEvent("jdk.FileRead", e -> { | ||
| if (e.getString("path").endsWith("test2.java")) { | ||
| wasRead.set(true); | ||
| System.err.println(e); | ||
| } | ||
| }); | ||
| rs.startAsync(); | ||
| // file2.getInputStream().read(); // check: commenting this in should fail the test | ||
| TreePathHandle.create(string, controller); | ||
| // TODO call directly post JDK 21 bump | ||
| RecordingStream.class.getMethod("stop").invoke(rs); // flushes stream | ||
| } | ||
| }, true); | ||
|
|
||
| assertFalse("file was read", wasRead.get()); |
There was a problem hiding this comment.
POC of how to replace SecurityManager based IO monitoring with JFR. This wont work in all cases since the jdk.FileRead/Write events are for read/write operations in the literal sense, not for file attribute access. (e.g the tests in the versioning module will likely require a different approach to replace SM usage there)
|
squashed a little bit, will squash more before merge |
mbien
requested review from
ebarboni,
lahodaj,
lkishalmi,
matthiasblaesing and
neilcsmith-net
new test and build range: JDK 21-26 few modules are still stuck on 17 and got special treatment
- added unimportant classes to the ignore list so that the test data doesn't have to be updated. - refactored file read monitoring from SecurityManager to JFR
- remove no longer needed SM usage in ide/xsl test - remove NBBundleWithSecurityManager test - remove NbLifecycleManagerEDTTest which is similar to LifecycleManagerEDTTest in openide.util.ui
mbien
force-pushed
the
ci-bump-lower-bound-21
branch
from
0e4b268 to
b9fb370
Compare
|
added commit for readme+launcher update |
ebarboni
left a comment
ebarboni
left a comment
There was a problem hiding this comment.
LGTM,
there is not possiblility to have kind of constant aka '17' or '21' in named baseline_jdk or other semantic in the workflow file juste to write less :D ?
There was a problem hiding this comment.
Looks good. Thanks for working on this.
Minor thought might be to rename test.jms.flags which is somewhat misused here by that name. And could we move the -J-Dbootstrap.disableJDKCheck=true under a TODO there too? But that's just a thought - no reason not to merge as is either!
matthiasblaesing
left a comment
matthiasblaesing
left a comment
There was a problem hiding this comment.
Looks sane to me. Thank you.
would be nice, we already have a constant for the JDK vendor. We probably have too many edge cases still but the way its set up we should be able to put the lower/mid/upper bounds into constants. Its just that sometimes we have releases when we want to test an extra version on a specific job or something like that.
yeah the flag isn't used as intended. But this hopefully only a temporary situation, until the tests are rewritten/removed. As permanent solution it would probably better to add an extra flag instead of renaming. Since some tests use it to reset the jms flags to empty.
will either add TODOs or check if those added in d0793ac are still needed and maybe remove some. thanks for the reviews! Will try to squash this somehow so that it still makes sense before merge. |
4 participants
new test and build range for NB 30+: JDK 21-26
some remain on 17 for due to #7871
this is also updating tests / test data if needed
Some of the tests still use the SM to inspect changing state. I began reimplementing it using JFR, but then realized that we can't do that yet due to an chicken-egg problem. It would require bumping some modules to JDK 21 which we could do now, but it would cause CI failures due to #7871. Workaround is to set
-Djava.security.manager=allowwhile trying to decide what do with the GraalVM job etc. Maybe we could exclude clusters from the GraalVM job.todo: update readme