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
Allow additional JVM args when running tests via toolchain #27089
Conversation
ZipEntry defines setLastModifiedTime, setTimeLocal, and other methods. Why do you need to hack the xdostime field? |
@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 |
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. |
@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. |
I opened #27100 to address the JarFileTests differently and reverted the additional params from the |
Closing in favor of #27100 |
Whoops. Sorry about that. |
@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. |
@AlanBateman While you're already in here, is there any possibility to reset |
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? |
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. |
Thanks very much, @dreis2211. |
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 (settingxdostime
onZipEntry
) and were no alternative exists to fix those (apart from disabling them for JDK 17)The idea is to extend the
ToolchainExtension
with a new property that sets the JVM args ofTest
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