[rfc][icedtea-web] Option Parser
Jiri Vanek
jvanek at redhat.com
Fri Sep 12 11:25:53 UTC 2014
> Yeah your right, I got rid of the method and just make a new ParserSettings and set the globalParserSettings after.
>
One more nit here inline later.
>> >If you decide to have helepr method here, then the bestargument for it is
>> >optionParser, and this
...snip...
>> >where this happens in code. AFAIK it does not.
> Any invalid args are caught in the first if statement which does not return, so it logs the Not Valid Args and displays a help message and the JNLPRuntime exits after.
>
>> >So validateMainArg should throw lunchError with invalid options found.
> I'm sorry but I don't quite understand what/how validateMainArg would do ?
> MainArgs are in the parser the arguments that weren't found in Options or as a value to an option.
> For ex. update only takes one value, therefore something like 'command update 5 File' would place File in
> the list as a main arg.
And thats correct :)
>
> The logic in boot, the first if checks that if you have more than one main arg, or you have a main arg and a
> jnlp has been specified that means more main args have been specified than expected in boot, hence throw InvalidArgumentException. The other two else if return the jnlp file.
>
agree
> To me it seems like validating the main args would be handled differently based on each application, in this case Boot is looking for only one arg that is to be a file, itweb-settings is probably not expecting a main arg. I am probably overlooking something so if you could explain to me what you mean/want out of validateMainArg that would be helpful, thanks !
agree.
This part now seems ok to me.
>
>> >This is actually still wrong. You shoudl remove only leading dashes.
>> >So you may change your algorithm to some while (s.starts) or use repalceAll
>> >with "^-*" regex
>> >
>>> > >+ s = s.replace("-","").split("=")[0];
>> >This is s surprising but not guaranted that if split argument mathces nothing
>> >then whole original is
>> >returens. But I guess we can live with that.
>> >
>> >Anyway this method needs test.
>> >
> This method is private so I can't test it directly, I added a few tests to check that a dash in the middle such as 'a-rg' would not be read as arg or multiple dashes would not replace the second dash such as '-ar-g' and just check the 'ar-g' part against the arg option. Also tested that with a '=' and no initial dash it recognizes the option aswell.
>
+- ok....
>> >You have list here, and it is correct. Do not vaste time converting it to
>> >array. Rather fix the
>> >users of those methods.
>> >
>> >and here - do not connvert to array. Wotking with list is much more
>> >comfortable. Keep returning
>> >list, and fix the users of those calls rather.
> Okay, I had to refactor Launcher.
Looks much better now, except one nit inline.
>
> Thank you,
> Lukasz Dracz
>
>
> optionParser-17.patch
>
>
> diff -r e30d71ab91c6 NEWS
...
> * extra information
> */
> - private void addProperties(JNLPFile file, String[] props) throws LaunchException {
> + private void addProperties(JNLPFile file, List<String> props) throws LaunchException {
> ResourcesDesc resources = file.getResources();
>
> - for (int i = 0; i < props.length; i++) {
> + for (String input : props) {
> // allows empty property, not sure about validity of that.
> - int equals = props[i].indexOf("=");
> - if (equals == -1) {
> - throw launchError(new LaunchException(R("BBadProp", props[i])));
> + if (!input.contains("=")) {
> + throw launchError(new LaunchException(R("BBadProp", input)));
> }
>
> - String key = props[i].substring(0, equals);
> - String value = props[i].substring(equals + 1, props[i].length());
> + String key = input.split("=")[0];
> + String value = input.split("=")[1];
Unluckily the behaviour changed here. What if vale contains equals chat to?
Please keep prevoius indexOf approach:(
>
> resources.addResource(new PropertyDesc(key, value));
> }
> @@ -344,18 +343,17 @@
> * @throws LaunchException if an exception occurs while extracting
> * extra information
> */
> - private void addParameters(JNLPFile file, String[] params) throws LaunchException {
> + private void addParameters(JNLPFile file, List<String> params) throws LaunchException {
> AppletDesc applet = file.getApplet();
>
> - for (int i = 0; i < params.length; i++) {
> + for (String input : params) {
> // allows empty param, not sure about validity of that.
> - int equals = params[i].indexOf("=");
> - if (equals == -1) {
> - throw launchError(new LaunchException(R("BBadParam", params[i])));
> + if (!input.contains("=")) {
> + throw launchError(new LaunchException(R("BBadParam", input)));
> }
>
> - String name = params[i].substring(0, equals);
> - String value = params[i].substring(equals + 1, params[i].length());
> + String name = input.split("=")[0];
> + String value = input.split("=")[1];
>
same here. Sorry.
tbh - those two deserves test as you are refactoring them.
> applet.addParameter(name, value);
> }
> @@ -365,11 +363,11 @@
> * Add the arguments to the JNLP file; only call if file is
> * actually an application (not installer).
> */
> - private void addArguments(JNLPFile file, String[] args) {
> + private void addArguments(JNLPFile file, List<String> args) {
> ApplicationDesc app = file.getApplication();
>
> - for (int i = 0; i < args.length; i++) {
> - app.addArgument(args[i]);
> + for (String input : args ) {
> + app.addArgument(input);
> }
> }
>
> diff -r e30d71ab91c6 netx/net/sourceforge/jnlp/OptionsDefinitions.java
> --- a/netx/net/sourceforge/jnlp/OptionsDefinitions.java Wed Sep 10 10:22:46 2014 -0400
> +++ b/netx/net/sourceforge/jnlp/OptionsDefinitions.java Thu Sep 11 15:30:54 2014 -0400
> @@ -71,6 +71,7 @@
> NOHEADERS("-Xignoreheaders", "BXignoreheaders"),
> OFFLINE("-Xoffline", "BXoffline"),
> TRUSTNONE("-Xtrustnone","BOTrustnone"),
> + JNLP("-jnlp","BOJnlp", NumberOfArguments.ONE),
> //itweb settings
> NODASHHELP("help", "IBOHelp"),
> LIST("list", "IBOList"),
> @@ -200,7 +201,8 @@
> OPTIONS.NOFORK,
> OPTIONS.NOHEADERS,
> OPTIONS.OFFLINE,
> - OPTIONS.TRUSTNONE});
> + OPTIONS.TRUSTNONE,
> + OPTIONS.JNLP});
> }
>
> public static List<OPTIONS> getJavaWsOptions() {
> diff -r e30d71ab91c6 netx/net/sourceforge/jnlp/ParserSettings.java
> --- a/netx/net/sourceforge/jnlp/ParserSettings.java Wed Sep 10 10:22:46 2014 -0400
> +++ b/netx/net/sourceforge/jnlp/ParserSettings.java Thu Sep 11 15:30:54 2014 -0400
> @@ -37,9 +37,6 @@
>
> package net.sourceforge.jnlp;
>
> -import java.util.Arrays;
> -import java.util.List;
> -
> /**
> * Contains settings to be used by the Parser while parsing JNLP files.
> *
> @@ -94,18 +91,4 @@
> globalParserSettings = parserSettings;
> }
>
> - /**
> - * Return the ParserSettings to be used according to arguments specified
> - * at boot on the command line. These settings are also stored so they
> - * can be retrieved at a later time.
> - */
> - public static ParserSettings setGlobalParserSettingsFromArgs(String[] cmdArgs) {
> - List<String> argList = Arrays.asList(cmdArgs);
> - boolean strict = argList.contains("-strict");
> - boolean malformedXmlAllowed = !argList.contains("-xml");
> -
> - globalParserSettings = new ParserSettings(strict, true, malformedXmlAllowed);
> - return globalParserSettings;
> - }
XXXX!!! :)
> -
> }
> diff -r e30d71ab91c6 netx/net/sourceforge/jnlp/runtime/Boot.java
> --- a/netx/net/sourceforge/jnlp/runtime/Boot.java Wed Sep 10 10:22:46 2014 -0400
> +++ b/netx/net/sourceforge/jnlp/runtime/Boot.java Thu Sep 11 15:30:54 2014 -0400
> @@ -22,7 +22,6 @@
> import java.net.URL;
> import java.security.AccessController;
> import java.security.PrivilegedAction;
> -import java.util.ArrayList;
> import java.util.Arrays;
> import java.util.HashMap;
> import java.util.List;
> @@ -32,6 +31,7 @@
>
...
> }
>
> - Map<String, String[]> extra = new HashMap<String, String[]>();
> - extra.put("arguments", getOptions("-arg"));
> - extra.put("parameters", getOptions("-param"));
> - extra.put("properties", getOptions("-property"));
> + Map<String, List<String>> extra = new HashMap<String, List<String>>();
> + extra.put("arguments", optionParser.getValues(OptionsDefinitions.OPTIONS.ARG));
> + extra.put("parameters", optionParser.getValues(OptionsDefinitions.OPTIONS.PARAM));
> + extra.put("properties", optionParser.getValues(OptionsDefinitions.OPTIONS.PROPERTY));
>
> - ParserSettings settings = ParserSettings.setGlobalParserSettingsFromArgs(args);
> + ParserSettings settings = new ParserSettings(optionParser.hasOption(OptionsDefinitions.OPTIONS.STRICT), true, !optionParser.hasOption(OptionsDefinitions.OPTIONS.XML));
> + ParserSettings.setGlobalParserSettings(settings);
Please hide those two lines.
add
public whatever setGlobalParserSettingsFromOptionParser(Optionparser optionParser){
ParserSettings settings = new
ParserSettings(optionParser.hasOption(OptionsDefinitions.OPTIONS.STRICT), true,
!optionParser.hasOption(OptionsDefinitions.OPTIONS.XML));
this.setGlobalParserSettings(settings);
}
in ParserSettings
and here call setGlobalParserSettingsFromOptionParser(optionParser)
only
Consequentially return (modified) the rmeoved test.
>
> try {
> Launcher launcher = new Launcher(false);
> @@ -266,7 +271,13 @@
> */
> private static URL getFileLocation() {
>
> - String location = getJNLPFile();
> + String location = null;
> + try {
> + location = getJNLPFile();
> + } catch (InvalidArgumentException e) {
> + OutputController.getLogger().log(e);
> + fatalError("Invalid argument: "+e);
> + }
>
> if (location == null) {
> handleMessage();
> @@ -294,64 +305,19 @@
> /**
> * Gets the JNLP file from the command line arguments, or exits upon error.
> */
> - private static String getJNLPFile() {
> + private static String getJNLPFile() throws InvalidArgumentException {
> + if (optionParser.getMainArgs().size() > 1 || (optionParser.mainArgExists()
> + && optionParser.hasOption(OptionsDefinitions.OPTIONS.JNLP))) {
> + throw new InvalidArgumentException(optionParser.getMainArg());
> + } else if (optionParser.hasOption(OptionsDefinitions.OPTIONS.JNLP)) {
> + return optionParser.getValue(OptionsDefinitions.OPTIONS.JNLP);
> + } else if (optionParser.mainArgExists()) {
> + return optionParser.getMainArg();
> + }
As you explained, fair enough.
>
> - if (args.length == 0) {
> - handleMessage();
> - JNLPRuntime.exit(0);
> - } else if (args.length == 1) {
> - return args[args.length - 1];
> - } else {
> - String lastArg = args[args.length - 1];
> - String secondLastArg = args[args.length - 2];
> -
> - if (doubleArgs.indexOf(secondLastArg) == -1) {
> - return lastArg;
> - } else {
> - handleMessage();
> - JNLPRuntime.exit(0);
> - }
> - }
> + handleMessage();
> + JNLPRuntime.exit(0);
> return null;
> }
>
> - /**
> - * Return value of the first occurence of the specified
> - * option, or null if the option is not present. If the
> - * option is a flag (0-parameter) and is present then the
> - * option name is returned.
> - */
> - private static String getOption(String option) {
> - String result[] = getOptions(option);
> -
> - if (result.length == 0)
> - return null;
> - else
> - return result[0];
> - }
> -
> - /**
> - * Return all the values of the specified option, or an empty
> - * array if the option is not present. If the option is a
> - * flag (0-parameter) and is present then the option name is
> - * returned once for each occurrence.
> - */
> - private static String[] getOptions(String option) {
> - List<String> result = new ArrayList<String>();
> -
> - for (int i = 0; i < args.length; i++) {
> - if (option.equals(args[i])) {
> - if (-1 == doubleArgs.indexOf(option))
> - result.add(option);
> - else if (i + 1 < args.length)
> - result.add(args[i + 1]);
> - }
> -
> - if (args[i].startsWith("-") && -1 != doubleArgs.indexOf(args[i]))
> - i++;
> - }
> -
> - return result.toArray(new String[result.size()]);
> - }
> -
> }
> diff -r e30d71ab91c6 netx/net/sourceforge/jnlp/util/optionparser/InvalidArgumentException.java
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/util/optionparser/InvalidArgumentException.java Thu Sep 11 15:30:54 2014 -0400
> @@ -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 InvalidArgumentException extends Exception {
> + public InvalidArgumentException(String argument) {
> + super(argument);
> + }
> +}
...
If you fulfill all nits above, please add also chnagelog in next iteration. I believe it will be
last one.. Otherwise we both grow old and turn gray before it is finish.
Also remember, your next task is to port ITWsettings and polieditor to use this class.
Looking forward to have this in!
J.
More information about the distro-pkg-dev
mailing list