[rfc][icedtea-web] Option Parser

[Lukasz Dracz]

Hello,

> As we agreed, drop basedri, and update NEWS
> 
> Something like "dropped support for long unmaintained -basedir argument"
> And also "maybe returned support for -jnlp argument " :)

Yes

> Much more nits later
> >
> >
> > optionParser-13.patch
> >   /**
> >    * Contains settings to be used by the Parser while parsing JNLP files.
> >    *
> > @@ -99,12 +96,8 @@
> >        * 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);
> > +    public static ParserSettings setGlobalParserSettingsFromArgs(boolean
> > strict, boolean xml) {
> > +        globalParserSettings = new ParserSettings(strict, true, !xml);
> >           return globalParserSettings;
> >       }
> 
> Looking to the code, I really do not see an reason for this to do exists
> anymore (even withyour
> explanation).
> This is just creating new object. You can create it and set is reguraly

Yeah your right, I got rid of the method and just make a new ParserSettings and set the globalParserSettings after.

> If you decide to have helepr method here, then the bestargument for it is
> optionParser, and this
> helping method will read it on its own.
> 
> eeing this:
>           ParserSettings defaultSettings, noArgs;
>           defaultSettings = new ParserSettings();
> -        noArgs = ParserSettings.setGlobalParserSettingsFromArgs(new
> String[0]);
> +        noArgs = ParserSettings.setGlobalParserSettingsFromArgs(false,
> true);
> 
> 
> later, makes me think that combination of
> setGlobalParserSettingsFromArgs(boolean strict, boolean xml) {
> and
> setGlobalParserSettingsFromArgs(OptionParser)) {
> setGlobalParserSettingsFromArgs(getParsed1, getPArsed2...) {
> }
> 
> Is the pointer...

I removed the second Test as I didn't see a point to it with the removal of that method.

> 
> Dont forget to drop also those lines and thirs mutations

Done.

> ...
> >        */
> >       private static String getJNLPFile() {
> > +        if (optionParser.getMainArgs().length > 1 ||
> > optionParser.mainArgExists()
> > +                &&
> > (optionParser.hasOption(OptionsDefinitions.OPTIONS.JNLP)
> > +                ||
> > optionParser.hasOption(OptionsDefinitions.OPTIONS.BASEDIR))) {
> > +          for (String s : optionParser.getMainArgs()) {
> > +              OutputController.getLogger().printOutLn("The argument '"+ s
> > +"' is not a valid expected argument");
> 
> use getLogger.log() for all outputs. There is actually no exception...

Okay, changed it to log and also added a new exception called InvalidArgumentException.

> > +          }
> > +        } else if
> > (optionParser.hasOption(OptionsDefinitions.OPTIONS.JNLP)) {
> > +            return optionParser.getValue(OptionsDefinitions.OPTIONS.JNLP);
> 
> I would encapsulate this in OptionParser
> 
> something like optionParser.validateMainArg() or similar.
> 
> 
> There is still question how to handle them.
> 
> But right now, the best way is to die once some unrecognized arg is found.
> Its not celar for me
> 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.

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. 

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 !

> 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.

> 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.

Thank you,
Lukasz Dracz
-------------- next part --------------
A non-text attachment was scrubbed...
Name: optionParser-17.patch
Type: text/x-patch
Size: 44383 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140911/4d762be1/optionParser-17-0001.patch>