Hi,

this is part of the overall Java 17 story #26767 and is a solution for points 6 & 7, where we access internals to either reset state (clearing URL.factory in Servlet tests) or create a test scenario (setting xdostime on ZipEntry) and were no alternative exists to fix those (apart from disabling them for JDK 17)

  1. Similar problem as 5. but in AbstractServletWebServerFactoryTests.tearDown() - Possibly similar solution with opening modules
  2. JarFileTests.jarFileEntryWithEpochTimeOfZeroShouldNotFail() fails because it tries to set xdostime on ZipEntry, which is internal. We could disable that test for JDK 17 and higher or again fiddle around with opening up modules.

The idea is to extend the ToolchainExtension with a new property that sets the JVM args of Test tasks - if given. I designed this to be flexible, so any kind of JVM args could be passed instead of just providing a way to open up modules.

While being on that, I also removed --illegal-access=warn already which has no effect on Java 17. I can keep it though on lower versions if you'd like me to keep it, but effectively it would only be set on the Java 16 pipeline, because that's the only toolchain based build at the moment. And likely the pipeline for 16 is going away once we have one for 17.

Let me know what you think. Cheers, Christoph

Comment From: AlanBateman

ZipEntry defines setLastModifiedTime, setTimeLocal, and other methods. Why do you need to hack the xdostime field?

Comment From: dreis2211

@AlanBateman If I remember correctly, there were JARs which where created with times of 0 (well 1970), which is not in the DOS time range where a bug in Boot failed to read the time in CentralDirectoryFileHeader.java#L119. See #19518.

The test at hand tries to mimic that, if I understand it correctly. It actually already calls setLastModifiedTime but that alone wouldn't help to cover the scenario, because it's the DOS time that's read in CentralDirectoryFileHeader and the Java methods don't allow to set things to 0 here. Maybe @mbhave as the creator of the test has some more insights on that, though.

Comment From: dreis2211

Let me see if I can write the time without reflection, but I'd like to separate this from the PR then...I actually see some tests in the JDK itself that try to cover similar scenarios.

Comment From: wilkinsona

@AlanBateman Thanks for asking about this.

@dreis2211 FWIW, I believe your understanding of that test is correct.

If it comes to it, I think we could avoid the use of reflection by checking in an appropriately configured jar file rather than generating it as part of the test. It'd be nice to be able to stick with the generation approach if possible, though.

Comment From: dreis2211

I opened #27100 to address the JarFileTests differently and reverted the additional params from the spring-boot-loader build.gradle for this PR.

Comment From: philwebb

Closing in favor of #27100

Comment From: dreis2211

@philwebb #27100 is not a replacement for the complete PR, just a replacement for the part of spring-boot-loader package that I rolled back already. The other changes are still needed.

Comment From: philwebb

Whoops. Sorry about that.

Comment From: dreis2211

@philwebb I'm wondering about the merge-with-amendments label. Just let me know in case you want something changed and I'll do so to spare you some time.

Comment From: dreis2211

@AlanBateman While you're already in here, is there any possibility to reset URL.factory I'm not aware of?

Comment From: AlanBateman

@AlanBateman While you're already in here, is there any possibility to reset URL.factory I'm not aware of?

Java 9 introduced URLStreamHandlerProvider to make it possible to deploy providers with other URL stream handler implementations, this avoids the need to use the setURLStreamHandlerFactory in some cases. I don't know if it helps here. If not, could you start a discussion on the OpenJDK net-dev mailing list with the summary of the need?

Comment From: dreis2211

I guess raising the baseline to 11 - whenever that's done - might open up the possibility to address this differently in the future then. For now, the current solution seems like the most pragmatic thing to do.

Comment From: wilkinsona

Thanks very much, @dreis2211.