Re: RFR: 8293041: --disable-@files option doesn't work and cause an error
On Mon, 21 Nov 2022 01:11:55 GMT, David Holmes wrote: > > tools/launcher/ArgsFileTest.java was working because it didn't contain any > > test with --disable-@files option verifying VM really starts > > What about the killswitch test ?? KillSwitch test did not construct valid command line, it didn't expect VM to start correctly and so it didn't test the VM exit value. KillSwitch test just checked expanded options presence in the debug log (before passing to ParseArguments). I've added one simple test composing valid cmd line 'java --disable-@files --version' and verifying successful VM execution. - PR: https://git.openjdk.org/jdk/pull/11183
Re: RFR: 8293041: --disable-@files option doesn't work and cause an error
On Fri, 18 Nov 2022 11:44:32 GMT, Adam Sotona wrote: > tools/launcher/ArgsFileTest.java was working because it didn't contain any > test with --disable-@files option verifying VM really starts What about the killswitch test ?? - PR: https://git.openjdk.org/jdk/pull/11183
Re: RFR: 8293041: --disable-@files option doesn't work and cause an error [v2]
On Fri, 18 Nov 2022 11:43:56 GMT, Adam Sotona wrote: >> Option --disable-@files is passed to VM option causing it to fail. >> Proposed patch skips --disable-@files option in >> src/java.base/share/native/libjli/java.c ParseArguments processing, so it is >> not passed to the VM. >> The patch also adds relevant test to ArgsFileTest. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > handling of --disable-@files moved from runtime/arguments.cpp to > libjli/java.c ParseArguments > DisableFilesOptionTest merged to ArgsFileTest::killSwitch > help typo moved to a new bug Good to see that this turned out to a simple change. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.org/jdk/pull/11183
Re: RFR: 8293041: --disable-@files option doesn't work and cause an error
On Thu, 17 Nov 2022 01:26:51 GMT, David Holmes wrote: >> Option --disable-@files is not recognised as valid JVM option, however it is >> already implemented in the launcher. >> Proposed patch skips --disable-@files option in >> src/hotspot/share/runtime/arguments.cpp processing, so it does not fall into >> the category of unknown options. >> The patch also corrects typo in the launcher.properties and adds simple >> DisableFilesOptionTest. >> >> Please review. >> >> Thank you, >> Adam > > BTW for future reference for the test case we are no longer creating tests > using the bug id as a directory, instead tests should be grouped into > functional areas. Thanks. > > I was also surprised to see the i8N changes here as I thought that was > normally handled seperately. @dholmes-ora and @AlanBateman thanks for pointing me to the right place for the fix. tools/launcher/ArgsFileTest.java was working because it didn't contain any test with --disable-@files option verifying VM really starts - PR: https://git.openjdk.org/jdk/pull/11183
Re: RFR: 8293041: --disable-@files option doesn't work and cause an error [v2]
> Option --disable-@files is not recognised as valid JVM option, however it is > already implemented in the launcher. > Proposed patch skips --disable-@files option in > src/hotspot/share/runtime/arguments.cpp processing, so it does not fall into > the category of unknown options. > The patch also corrects typo in the launcher.properties and adds simple > DisableFilesOptionTest. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: handling of --disable-@files moved from runtime/arguments.cpp to libjli/java.c ParseArguments DisableFilesOptionTest merged to ArgsFileTest::killSwitch help typo moved to a new bug - Changes: - all: https://git.openjdk.org/jdk/pull/11183/files - new: https://git.openjdk.org/jdk/pull/11183/files/ab754b8d..acddbffb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11183&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11183&range=00-01 Stats: 58 lines in 15 files changed: 6 ins; 40 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/11183.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11183/head:pull/11183 PR: https://git.openjdk.org/jdk/pull/11183
Re: RFR: 8293041: --disable-@files option doesn't work and cause an error
On Wed, 16 Nov 2022 12:34:29 GMT, Adam Sotona wrote: > Option --disable-@files is not recognised as valid JVM option, however it is > already implemented in the launcher. > Proposed patch skips --disable-@files option in > src/hotspot/share/runtime/arguments.cpp processing, so it does not fall into > the category of unknown options. > The patch also corrects typo in the launcher.properties and adds simple > DisableFilesOptionTest. > > Please review. > > Thank you, > Adam As far as I can see @-file processing is a launcher feature, including the expansion of JDK_JAVA_OPTIONS, so it should never be getting passed through to the VM in the first place. What I can't figure out is how the test tools/launcher/ArgsFileTest.java is working when the JVM won't start with that flag on the command-line ?? BTW for future reference for the test case we are no longer creating tests using the bug id as a directory, instead tests should be grouped into functional areas. Thanks. I was also surprised to see the i8N changes here as I thought that was normally handled seperately. - Changes requested by dholmes (Reviewer). PR: https://git.openjdk.org/jdk/pull/11183
Re: RFR: 8293041: --disable-@files option doesn't work and cause an error
On Wed, 16 Nov 2022 16:10:45 GMT, Adam Sotona wrote: > Alternate suggestions are welcome, or link to relevant documentation, or an > example of another filtered option. I think I would start in ParseArguments (libjli/java.c) to see the options that are handled, translated, or passed through. I can't be 100% sure how this one should be handled without studying it more closely. - PR: https://git.openjdk.org/jdk/pull/11183
Re: RFR: 8293041: --disable-@files option doesn't work and cause an error
On Wed, 16 Nov 2022 14:38:18 GMT, Alan Bateman wrote: > Are you sure this is right place to do this? There are other "launcher > options" that aren't passed through to CreateJavaVM and I'm surprised this > one is. I've found this place as a single spot handling (or skipping) tons of options and as a source of the error message. The launcher code I've investigated provides a lot of checking, however no suitable place to skip any option. Filtering in launcher would require significant refactoring, or repeated filtering on many spots to handle all alternative paths (unless I missed something important). Alternate suggestions are welcome, or link to relevant documentation, or an example of another filtered option. Thanks, Adam - PR: https://git.openjdk.org/jdk/pull/11183
Re: RFR: 8293041: --disable-@files option doesn't work and cause an error
On Wed, 16 Nov 2022 12:34:29 GMT, Adam Sotona wrote: > Option --disable-@files is not recognised as valid JVM option, however it is > already implemented in the launcher. > Proposed patch skips --disable-@files option in > src/hotspot/share/runtime/arguments.cpp processing, so it does not fall into > the category of unknown options. > The patch also corrects typo in the launcher.properties and adds simple > DisableFilesOptionTest. > > Please review. > > Thank you, > Adam Are you sure this is right place to do this? There are other "launcher options" that aren't passed through to CreateJavaVM and I'm surprised this one is. - PR: https://git.openjdk.org/jdk/pull/11183
