groovy
  1. groovy
  2. GROOVY-5191

Running script with '--enoding' param and some script parameters

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0-beta-2, 1.8.6
    • Fix Version/s: 2.0.6, 2.1.0-beta-1
    • Labels:
      None
    • Environment:
      win 7 x64, jdk 1.6.0.27, groovy 1.9-beta-3
    • Number of attachments :
      2

      Description

      Create simple script 'main.groovy':

      args.each {println it}
      

      and try to run it by the following command:
      groovy --encoding=UTF-8 main.groovy -script -param

      The output is:
      script
      -p
      -a
      ram

      while the expected output is:
      -script
      -param

      1. patch.diff
        0.9 kB
        Jan Weitz
      1. proof.png
        46 kB

        Issue Links

          Activity

          Hide
          blackdrag blackdrag added a comment -

          that sounds very much like a commons cli problem to me.

          Show
          blackdrag blackdrag added a comment - that sounds very much like a commons cli problem to me.
          Hide
          Maxim Medvedev added a comment -

          The same result is in IntelliJ IDEA if file have encoding which differs from system default one.

          BTW the command without --encoding param produces the correct output (from cmd and IntelliJ iDEA)

          groovy main.groovy -script -param
          
          Show
          Maxim Medvedev added a comment - The same result is in IntelliJ IDEA if file have encoding which differs from system default one. BTW the command without --encoding param produces the correct output (from cmd and IntelliJ iDEA) groovy main.groovy -script -param
          Hide
          Jan Weitz added a comment - - edited

          Issue is universal. e.g. MacOSX, 2.0.0-beta-3
          Fix: Pull Request - https://github.com/groovy/groovy-core/pull/28

          Hi,

          it's not a commons cli problem. The problems is inside the logic how GroovyMain parses it's args and determines whether it will need the params to itself or pass them to your scriptFile.

          Since '-p' and '-a' are Options in GroovyMain and you specified '-param', GroovyMain will extract '-p', and '-a' and give you the rest, which it did not recognize => 'ram'

          Therefore the --encoding param is just one example on how this fails.
          You can take any params abbreviation, which is used by GroovyMain.

          The current logic determines the name of the scriptfile you want to run, too late, which explains, why

          groovy --encoding=UTF-8 main.groovy -script -param

          fails, while

          groovy main.groovy -script -param

          does not fail.

          So, I think we should force the convention that you first enter all params for the GrooyMain part, then an optional scriptFile and optional scriptArgs.

          groovy [options] [scriptName scriptArgs]

          Show
          Jan Weitz added a comment - - edited Issue is universal. e.g. MacOSX, 2.0.0-beta-3 Fix: Pull Request - https://github.com/groovy/groovy-core/pull/28 Hi, it's not a commons cli problem. The problems is inside the logic how GroovyMain parses it's args and determines whether it will need the params to itself or pass them to your scriptFile. Since '-p' and '-a' are Options in GroovyMain and you specified '-param', GroovyMain will extract '-p', and '-a' and give you the rest, which it did not recognize => 'ram' Therefore the --encoding param is just one example on how this fails. You can take any params abbreviation, which is used by GroovyMain. The current logic determines the name of the scriptfile you want to run, too late, which explains, why groovy --encoding=UTF-8 main.groovy -script -param fails, while groovy main.groovy -script -param does not fail. So, I think we should force the convention that you first enter all params for the GrooyMain part, then an optional scriptFile and optional scriptArgs. groovy [options] [scriptName scriptArgs]
          Show
          Jan Weitz added a comment - Related tickets: https://jira.codehaus.org/browse/GROOVY-5020 https://jira.codehaus.org/browse/GROOVY-5282
          Hide
          Paul King added a comment -

          link related issues

          Show
          Paul King added a comment - link related issues
          Hide
          Paul King added a comment -

          The following currently works:

          groovy -e "println args[0]" hi
          

          but produces "java.lang.ArrayIndexOutOfBoundsException: 0" after the patch.

          As a minimum this would need to be flagged as a breaking change but I am wondering how many people rely on the above behavior and changing it would not be good for them.

          Show
          Paul King added a comment - The following currently works: groovy -e "println args[0]" hi but produces " java.lang.ArrayIndexOutOfBoundsException: 0 " after the patch. As a minimum this would need to be flagged as a breaking change but I am wondering how many people rely on the above behavior and changing it would not be good for them.
          Hide
          Jan Weitz added a comment - - edited

          Hi Paul,
          thanks. This was a small error. I did not catch the -e option. You can apply the uploaded patch, which fixes this or get everything from the pull request on Github.
          https://github.com/weitzj/groovy-core/commit/bfd6870f8513c5443b7822ef74dbee449ecf3d1e

          or change line 386 in groovy.ui.GroovyMain.java to read the following:

          if(main.processFiles) --> if(!main.isScriptFile || main.processFiles)

          The logic is now:

          • if the user uses the -n, -p or -e option, everything should stay the same as it has before.

          Otherwise we know we have a groovy script, which should get executed, and change the args array.

          Greetings,

          Jan

          Show
          Jan Weitz added a comment - - edited Hi Paul, thanks. This was a small error. I did not catch the -e option. You can apply the uploaded patch, which fixes this or get everything from the pull request on Github. https://github.com/weitzj/groovy-core/commit/bfd6870f8513c5443b7822ef74dbee449ecf3d1e or change line 386 in groovy.ui.GroovyMain.java to read the following: if(main.processFiles) --> if(!main.isScriptFile || main.processFiles) The logic is now: if the user uses the -n, -p or -e option, everything should stay the same as it has before. Otherwise we know we have a groovy script, which should get executed, and change the args array. Greetings, Jan
          Hide
          Jan Weitz added a comment -

          Do you need any more input on this? Is this all crap?

          Greets,

          Jan

          Show
          Jan Weitz added a comment - Do you need any more input on this? Is this all crap? Greets, Jan
          Hide
          Paul King added a comment -

          Your suggestions are probably all spot on - speaking for myself, I just need more time to test and have been pretty busy lately. Parameter passing has traditionally been a very fragile area - mostly not to do with changes we make in Java code; usually related to surrounding windows batch scripts or bugs in commons CLI (mostly earlier versions). And on top of that we have low (automated) test coverage of this area. In any case, I am always loathe to make any changes in this space without sufficient additional manual testing. So, hopefully with a bit more time we can progress this issue to everyone's satisfaction.

          Show
          Paul King added a comment - Your suggestions are probably all spot on - speaking for myself, I just need more time to test and have been pretty busy lately. Parameter passing has traditionally been a very fragile area - mostly not to do with changes we make in Java code; usually related to surrounding windows batch scripts or bugs in commons CLI (mostly earlier versions). And on top of that we have low (automated) test coverage of this area. In any case, I am always loathe to make any changes in this space without sufficient additional manual testing. So, hopefully with a bit more time we can progress this issue to everyone's satisfaction.
          Hide
          Paul King added a comment -

          OK, finally got some more time to test things out. Jan's proposed patch definitely works around the problem but it seems to me that this is really a workaround that we shouldn't have to do and in fact there is indeed a bug in Commons-CLI. If I swap out the PosixParser that we currently use and replace it with the GnuParser (or the DefaultParser in Commons-CLI 1.3-SNAPSHOT) then the problem goes away. And looking at the source code for PosixParser, it never seems to clear out the "currentOption" field in between iterations - which it should do to work properly when "stopAtNonOption" is set to true as per how we use that parser.

          Show
          Paul King added a comment - OK, finally got some more time to test things out. Jan's proposed patch definitely works around the problem but it seems to me that this is really a workaround that we shouldn't have to do and in fact there is indeed a bug in Commons-CLI. If I swap out the PosixParser that we currently use and replace it with the GnuParser (or the DefaultParser in Commons-CLI 1.3-SNAPSHOT) then the problem goes away. And looking at the source code for PosixParser, it never seems to clear out the "currentOption" field in between iterations - which it should do to work properly when "stopAtNonOption" is set to true as per how we use that parser.
          Hide
          Paul King added a comment -

          There is now a custom GroovyPosixParser which I hope fixes (some of) the bugs in PosixParser. This should just be an interim measure until Commons-CLI 1.3 is released at which point we should be able to delete that class and switch over to DefaultParser from commons-cli itself.

          Note: in 2.1, the "posix" flag for CliBuilder is deprecated and GroovyPosixParser becomes the default parser in use if no explicit class is specified via the parser parameter.

          Show
          Paul King added a comment - There is now a custom GroovyPosixParser which I hope fixes (some of) the bugs in PosixParser. This should just be an interim measure until Commons-CLI 1.3 is released at which point we should be able to delete that class and switch over to DefaultParser from commons-cli itself. Note: in 2.1, the "posix" flag for CliBuilder is deprecated and GroovyPosixParser becomes the default parser in use if no explicit class is specified via the parser parameter.
          Hide
          blackdrag blackdrag added a comment -

          Paul, can't we then close this issue and the pull request? I think the issue does not need to stay open until CLI 1.3 eventually comes into existence

          Show
          blackdrag blackdrag added a comment - Paul, can't we then close this issue and the pull request? I think the issue does not need to stay open until CLI 1.3 eventually comes into existence
          Hide
          Paul King added a comment -

          Yes, closing is fine.

          Show
          Paul King added a comment - Yes, closing is fine.

            People

            • Assignee:
              Paul King
              Reporter:
              Maxim Medvedev
            • Votes:
              1 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: