[rfc][icedtea-web] use Option Parser in itweb-settings
Jiri Vanek
jvanek at redhat.com
Mon Dec 8 13:31:00 UTC 2014
Yup! Realy cool work. I have only small nits, and the bigest ones I'm not sure how to deal:(
minor nit (little bit unrelated, but should be included in patch):
our docs says this:
DESCRIPTION
-check name - Checks that the current settings have valid values.(No argument expected)
-get name - Shows the value of the named setting.(Expected one or more arguments)
-headless - Disables download window, other UIs.(No argument expected)
-help - Prints out information about supported command and basic usage.(No argument
expected)
-info name - Shows additional information about the named setting. Includes a description,
the current value, the possible values, and the source of the setting.(Expected one or more arguments)
-list - Shows a list of all settings.(No argument expected)
-reset name - Resets the named setting to its original value.(Expected one or more arguments)
-reset all - Resets all settings to their original values.(No argument expected)
-set name value - Sets the setting to the new value, after checking that it is an appropriate
value.(Expected even number of arguments with param=value as valid argument)
You are affecting those, so try to align them with your real impl.
main issues are - described functionality is really excellent, but the impl have few flaws:
[jvanek at jvanek bin]$ ./itweb-settings check dsf sd
No issues found.
- sounds strnage doesnt it? But I'mnot sure if worthy to fix...
[jvanek at jvanek bin]$ ./itweb-settings get dsf sd
Unknown property-name "dsf"
[jvanek at jvanek bin]$ ./itweb-settings get deployment.user.security.policy
deployment.user.security.policy: file:///home/jvanek/.config/icedtea-web/security/java.policy
[jvanek at jvanek bin]$ ./itweb-settings get deployment.user.security.policy dsf sfd
deployment.user.security.policy: file:///home/jvanek/.config/icedtea-web/security/java.policy
Unknown property-name "dsf"
- so it dies with first unrecognized property... Not sure what to do here, wheter to keep it as
you have it, or invest time and dye iumidiately (not worthy probably) , or after Unknown
property-name just continue with other one... Whether the first is definitly the most correct, not
sure if it is worthy....
on contrary:
[jvanek at jvanek bin]$ ./itweb-settings set dsf sfd
WARNING: Unknown property name "dsf" - creating new property
[jvanek at jvanek bin]$ ./itweb-settings set dsf sfd fdwf sfg
Property "dsf" is unknown.
WARNING: Unknown property name "fdwf" - creating new property
[jvanek at jvanek bin]$ ./itweb-settings set dsf sfd fdw
net.sourceforge.jnlp.util.optionparser.UnevenParameterException: For option -set expected an even
number of params.
at net.sourceforge.jnlp.util.optionparser.OptionParser.checkOptionHasEvenNumber(OptionParser.java:129)
....
- this loks correct.But the previous issue may be more spread...
--details
well this is strange. On one side, it do not seem to work. On second, it seems to be undefined n
general lsit of swithces, and so *undocumented*.
Unless I missed some functionality, I'm for to drop it. (but if it have some, please tell before
removing :))
On 12/03/2014 09:24 PM, Lukasz Dracz wrote:
> Hello,
>
> This patch changes itweb-settings to use the Option Parser. Only one itweb-settings command can be used at a time in combinations with help and headless. Help specified with a command will return the help for that command. The commands however have been changed to allow the use of multiple arguments ex. itweb-settings get deployment.manifest.attributes.check deployment.console.startup.mode deployment.log.file will get all three arguments. Added functionality for EVEN_NUMBER_SUPPORTS_EQUALS_CHAR which is used in the set command so now it is possible to enter arguments for the set as property=value or property value or a combination of this.
>
> Thank you,
> Lukasz Dracz
>
>
> itweb-settings-OptionParser-2.patch
>
>
> diff --git a/netx/net/sourceforge/jnlp/OptionsDefinitions.java b/netx/net/sourceforge/jnlp/OptionsDefinitions.java
> --- a/netx/net/sourceforge/jnlp/OptionsDefinitions.java
> +++ b/netx/net/sourceforge/jnlp/OptionsDefinitions.java
> @@ -76,7 +76,7 @@
> LIST("-list", "IBOList"),
> GET("-get", "name", "IBOGet", NumberOfArguments.ONE_OR_MORE),
> INFO("-info", "name", "IBOInfo", NumberOfArguments.ONE_OR_MORE),
> - SET("-set", "name value", "IBOSet", NumberOfArguments.ONE_OR_MORE),
> + SET("-set", "name value", "IBOSet", NumberOfArguments.EVEN_NUMBER_SUPPORTS_EQUALS_CHAR),
pelase fix the messages properties to be accurate.
> RESETALL("-reset", "all", "IBOResetAll"),
> RESET("-reset", "name", "IBOReset", NumberOfArguments.ONE_OR_MORE),
> /**
> * Handles the 'list' command
> *
> - * @param args the arguments to the list command
> * @return result of handling the command. SUCCESS if no errors occurred.
> */
> - public int handleListCommand(List<String> args) {
The advantage of this declaration ^ is that it is more t
You can always have:
public int handleListCommand() {
handleListCommand(optionParser)
}
public int handleListCommand(OptionParser op) {...
But does nto meter, its up to you.
> - if (args.contains("help")) {
> + public int handleListCommand() {
> + if (optionParser.hasOption(OptionsDefinitions.OPTIONS.HELP)) {
> printListHelp();
> return SUCCESS;
> }
>
> boolean verbose = false;
>
> - if (args.contains("--details")) {
> + if (optionParser.getMainArgs().contains("--details")) {
hmhhm.. seesm nt to be working see top of email.
(update 10 minutes later:)
Well, aybe it may work, if istead of this --details, the itwebsettings recognize general verbose
switch... what do you think?
> verbose = true;
> - args.remove("--details");
....
> - old.getValidator().validate(value);
> - } catch (IllegalArgumentException e) {
> - OutputController.getLogger().log(OutputController.Level.WARNING_ALL, R("CLIncorrectValue", old.getName(), value, old.getValidator().getPossibleValues()));
> - OutputController.getLogger().log(e);
> - return ERROR;
> + for (String arg : args) {
> + if (args.indexOf(arg) % 2 == 0) {
ok, I dont understand this. What this 17 lines should do? Can ti be more readable? More "use
parser" like? Or at least commented?
> +
> + key = arg;
> + } else {
> +
> + value = arg;
> + if (config.getRaw().containsKey(key)) {
> + Setting<String> old = config.getRaw().get(key);
> + if (old.getValidator() != null) {
> + try {
> + old.getValidator().validate(value);
> + } catch (IllegalArgumentException e) {
> + OutputController.getLogger().log(OutputController.Level.WARNING_ALL, R("CLIncorrectValue", old.getName(), value, old.getValidator().getPossibleValues()));
> + OutputController.getLogger().log(e);
> + return ERROR;
yes.. return or not return? Handle in parser? See lower...
> + }
> + }
> +
> + config.setProperty(key, value);
> + } else {
> + OutputController.getLogger().printOutLn(R("CLWarningUnknownProperty", key));
> + config.setProperty(key, value);
> }
> }
> - config.setProperty(key, value);
> - } else {
> - OutputController.getLogger().printOutLn(R("CLWarningUnknownProperty", key));
> - config.setProperty(key, value);
> }
>
.. big snip! :)
> -
> + public int handle() {
> int val;
> - if (command.equals(OptionsDefinitions.OPTIONS.HELP.option)) {
> + if (getNumberOfOptions() > 1) {
> + OutputController.getLogger().log(OutputController.Level.MESSAGE_ALL, R("CLUnexpectedNumberOfCommands"));
> val = handleHelpCommand();
I think those exceptions may be handeld in parser, but i dont insists (as help may have different
meaning in javaws / itwebsettings/policy editor
But if it does so, then our documentation will be always wrong... Maybe to have two different
"help" declarations? I think it is possible to do so. One willbe used in javaws/polici editor and
second (HELP1, help, info1, no argument)in itweb-settings (HELP2, help, info2, no or more args)
Thougths?
> - } else if (command.equals(OptionsDefinitions.OPTIONS.LIST.option)) {
> - val = handleListCommand(arguments);
> - } else if (command.equals(OptionsDefinitions.OPTIONS.SET.option)) {
> - val = handleSetCommand(arguments);
> - } else if (command.equals(OptionsDefinitions.OPTIONS.RESET.option)) {
> - val = handleResetCommand(arguments);
> - } else if (command.equals(OptionsDefinitions.OPTIONS.GET.option)) {
> - val = handleGetCommand(arguments);
> - } else if (command.equals(OptionsDefinitions.OPTIONS.INFO.option)) {
> - val = handleInfoCommand(arguments);
> - } else if (command.equals(OptionsDefinitions.OPTIONS.CHECK.option)) {
> - val = handleCheckCommand(arguments);
> + } else if (optionParser.hasOption(OptionsDefinitions.OPTIONS.LIST)) {
> + val = handleListCommand();
> + } else if (optionParser.hasOption(OptionsDefinitions.OPTIONS.SET)) {
> + val = handleSetCommand();
> + } else if (optionParser.hasOption(OptionsDefinitions.OPTIONS.RESET)) {
> + val = handleResetCommand();
> + } else if (optionParser.hasOption(OptionsDefinitions.OPTIONS.GET)) {
> + val = handleGetCommand();
> + } else if (optionParser.hasOption(OptionsDefinitions.OPTIONS.INFO)) {
> + val = handleInfoCommand();
> + } else if (optionParser.hasOption(OptionsDefinitions.OPTIONS.CHECK)) {
> + val = handleCheckCommand();
> + } else if (optionParser.hasOption(OptionsDefinitions.OPTIONS.HELP)) {
> + val = handleHelpCommand();
> } else {
> - OutputController.getLogger().log(OutputController.Level.MESSAGE_ALL, R("CLUnknownCommand", command));
> + for (String unknown : optionParser.getMainArgs()) {
> + OutputController.getLogger().log(OutputController.Level.MESSAGE_ALL, R("CLUnknownCommand", unknown));
> + }
> handleHelpCommand();
> val = ERROR;
> }
> -
> return val;
> }
>
> + private int getNumberOfOptions() {
> + int number = optionParser.getNumberOfOptions();
> + if (optionParser.hasOption(OptionsDefinitions.OPTIONS.HELP)) {
> + number--;
> + }
> + if (optionParser.hasOption(OptionsDefinitions.OPTIONS.HEADLESS)) {
> + number--;
> + }
> + return number;
> + }
> /**
> * The starting point of the program
> * @param args the command line arguments to this program
> */
> public static void main(String[] args) throws Exception {
> - DeploymentConfiguration.move14AndOlderFilesTo15StructureCatched();
> - if (args.length == 0) {
> - ControlPanel.main(new String[] {});
> - } else {
> - CommandLine cli = new CommandLine();
> - int result = cli.handle(args);
> + try {
> + optionParser = new OptionParser(args, OptionsDefinitions.getItwsettingsCommands());
>
> - // instead of returning, use JNLPRuntime.exit() so we can pass back
> - // error codes indicating success or failure. Otherwise using
> - // this program for scripting will become much more challenging
> - JNLPRuntime.exit(result);
> + if (optionParser.hasOption(OptionsDefinitions.OPTIONS.HEADLESS)) {
> + JNLPRuntime.setHeadless(true);
> + }
> + DeploymentConfiguration.move14AndOlderFilesTo15StructureCatched();
Not sure If I got this. It was missing here?? Crap.. It would be worthy for 1.5 too :(
> + if (args.length == 0) {
> + ControlPanel.main(new String[] {});
> + } else {
> + CommandLine cli = new CommandLine();
> + int result = cli.handle();
> +
> + // instead of returning, use JNLPRuntime.exit() so we can pass back
> + // error codes indicating success or failure. Otherwise using
> + // this program for scripting will become much more challenging
> + JNLPRuntime.exit(result);
> + }
> + } catch (Exception e) {
> + OutputController.getLogger().log(OutputController.Level.WARNING_ALL, e);
> + JNLPRuntime.exit(1);
> }
> }
> }
> diff --git a/netx/net/sourceforge/jnlp/resources/Messages.properties b/netx/net/sourceforge/jnlp/resources/Messages.properties
> --- a/netx/net/sourceforge/jnlp/resources/Messages.properties
> +++ b/netx/net/sourceforge/jnlp/resources/Messages.properties
> @@ -338,6 +338,9 @@
> be added and selected. Multiple URLs may also be given with a single -codebase flag by separating them with spaces. In this case, the last codebase given will be selected, and all will be \
> added. If this flag is given more than once, only the first is used.
>
> +# Option Parser
> +OPUnevenParams=For option {0} expected an even number of params.
> +
> # NumberOfArguments descriptions.
> NOAnone=No argument expected
> NOAone=Exactly one argument expected
> @@ -909,6 +912,7 @@
> CLResetDescription=Resets the value for property-name to it\'s default value.\nall resets all properties recognized by IcedTea-Web to their default value.
> CLInfoDescription=Shows more information about the given property
> CLCheckDescription=Shows any properties that have been defined but are not recognized by IcedTea-Web
> +CLUnexpectedNumberOfCommands=Itweb-settings can only run one command at a time.
>
> # splash screen related
> SPLASHerror = Click here for details. An exception has occurred.
> diff --git a/netx/net/sourceforge/jnlp/runtime/Boot.java b/netx/net/sourceforge/jnlp/runtime/Boot.java
> --- a/netx/net/sourceforge/jnlp/runtime/Boot.java
> +++ b/netx/net/sourceforge/jnlp/runtime/Boot.java
> @@ -46,6 +46,7 @@
> import net.sourceforge.jnlp.util.logging.OutputController;
> import net.sourceforge.jnlp.util.optionparser.InvalidArgumentException;
> import net.sourceforge.jnlp.util.optionparser.OptionParser;
> +import net.sourceforge.jnlp.util.optionparser.UnevenParameterException;
> import sun.awt.AppContext;
> import sun.awt.SunToolkit;
>
> @@ -96,7 +97,12 @@
> * Launch the JNLP file specified by the command-line arguments.
> */
> public static void main(String[] argsIn) {
> - optionParser = new OptionParser(argsIn, OptionsDefinitions.getJavaWsOptions());
> + try {
> + optionParser = new OptionParser(argsIn, OptionsDefinitions.getJavaWsOptions());
> + } catch (UnevenParameterException upe) {
> + OutputController.getLogger().log(OutputController.Level.WARNING_ALL, upe);
> + JNLPRuntime.exit(1);
Wby exit? Isnt rethrow better? Why rethrow at all.. this exception may continue up and up until
uttermost top end death, or not?
> + }
>
> if (optionParser.hasOption(OptionsDefinitions.OPTIONS.VERBOSE)) {
> JNLPRuntime.setDebug(true);
> diff --git a/netx/net/sourceforge/jnlp/util/optionparser/OptionParser.java b/netx/net/sourceforge/jnlp/util/optionparser/OptionParser.java
> --- a/netx/net/sourceforge/jnlp/util/optionparser/OptionParser.java
> +++ b/netx/net/sourceforge/jnlp/util/optionparser/OptionParser.java
> @@ -41,6 +41,9 @@
> import java.util.List;
>
> import net.sourceforge.jnlp.OptionsDefinitions;
> +import net.sourceforge.jnlp.util.logging.OutputController;
> +
> +import static net.sourceforge.jnlp.runtime.Translator.R;
>
> /** Parses for options (passed in as arguments)
> * To use add entries to OPTIONS enum in OptionsDefinitions
> @@ -54,13 +57,18 @@
> private final List<OptionsDefinitions.OPTIONS> possibleOptions;
> //List of all possible main arguments
> private final List<String> mainArgumentList = new ArrayList<>();
> + private boolean evenNumberFound;
>
> - public OptionParser(String[] args, List<OptionsDefinitions.OPTIONS> options) {
> + public OptionParser(String[] args, List<OptionsDefinitions.OPTIONS> options) throws UnevenParameterException {
> this.args = Arrays.copyOf(args, args.length);
> this.possibleOptions = options;
>
> + evenNumberFound = false;
> parsedOptions = new ArrayList<>();
> parseContents();
> + if (evenNumberFound) {
> + checkOptionHasEvenNumber();
> + }
>
> }
>
> @@ -71,12 +79,23 @@
> lastOption = addOptionToList(arg);
> } else if (shouldAddParam(lastOption)) {
> lastOption.addParam(arg);
> + } else if (isEvenNumberSupportingEqualsChars(lastOption)) {
> + evenNumberFound = true;
> + handleEvenNumberSupportingEqualsChar(lastOption, arg);
> } else {
> mainArgumentList.add(arg);
> }
> }
> }
>
> + private void handleEvenNumberSupportingEqualsChar(final ParsedOption lastOption, String arg) {
> + if (arg.contains("=")) {
> + lastOption.addParam(arg.split("=")[0]);
> + lastOption.addParam(arg.split("=")[1]);
> + } else {
> + lastOption.addParam(arg);
> + }
> + }
> private boolean shouldAddParam(final ParsedOption lastOption) {
> return lastOption != null &&
> (oneOrMoreArguments(lastOption) || isOneArgumentNotFull(lastOption));
> @@ -90,6 +109,10 @@
> return lastOption.getOption().hasOneOrMoreArguments();
> }
>
> + private boolean isEvenNumberSupportingEqualsChars(final ParsedOption lastOption) {
> + return lastOption.getOption().hasEvenNumberSupportingEqualsChar();
> + }
> +
> private ParsedOption addOptionToList(final String arg) {
> ParsedOption option = new ParsedOption(argumentToOption(arg));
> if (arg.contains("=")) {
> @@ -99,6 +122,16 @@
> return option;
> }
>
> + private void checkOptionHasEvenNumber() throws UnevenParameterException {
> + for (ParsedOption option : parsedOptions) {
> + if (isEvenNumberSupportingEqualsChars(option)) {
> + if (option.getParams().size() % 2 != 0){
> + throw new UnevenParameterException(R("OPUnevenParams", option.getOption().option));
> + }
> + }
> + }
> + }
> +
> private OptionsDefinitions.OPTIONS argumentToOption(final String arg) {
> for (OptionsDefinitions.OPTIONS opt : possibleOptions) {
> if (stringEqualsOption(arg, opt)) {
> @@ -171,4 +204,8 @@
> }
> return result;
> }
> +
> + public int getNumberOfOptions() {
> + return parsedOptions.size();
> + }
Some tests of those new methods would be good.
> }
> diff --git a/netx/net/sourceforge/jnlp/util/optionparser/UnevenParameterException.java b/netx/net/sourceforge/jnlp/util/optionparser/UnevenParameterException.java
> new file mode 100644
> --- /dev/null
> +++ b/netx/net/sourceforge/jnlp/util/optionparser/UnevenParameterException.java
> @@ -0,0 +1,43 @@
> +/*Copyright (C) 2014 Red Hat, Inc.
> +
> +This file is part of IcedTea.
> +
> +IcedTea is free software; you can redistribute it and/or
> +modify it under the terms of the GNU General Public License as published by
> +the Free Software Foundation, version 2.
> +
> +IcedTea is distributed in the hope that it will be useful,
> +but WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with IcedTea; see the file COPYING. If not, write to
> +the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> +02110-1301 USA.
> +
> +Linking this library statically or dynamically with other modules is
> +making a combined work based on this library. Thus, the terms and
> +conditions of the GNU General Public License cover the whole
> +combination.
> +
> +As a special exception, the copyright holders of this library give you
> +permission to link this library with independent modules to produce an
> +executable, regardless of the license terms of these independent
> +modules, and to copy and distribute the resulting executable under
> +terms of your choice, provided that you also meet, for each linked
> +independent module, the terms and conditions of the license of that
> +module. An independent module is a module which is not derived from
> +or based on this library. If you modify this library, you may extend
> +this exception to your version of the library, but you are not
> +obligated to do so. If you do not wish to do so, delete this
> +exception statement from your version.
> +*/
> +
> +package net.sourceforge.jnlp.util.optionparser;
> +
> +public class UnevenParameterException extends Exception {
> + public UnevenParameterException(String argument) {
> + super(argument);
> + }
> +}
> \ No newline at end of file
>
Nioce job! Looking forward to have this in!
J.
More information about the distro-pkg-dev
mailing list