Groovy
  1. Groovy
  2. GROOVY-5266

GroovyEngine not creating friendly script names for ant files

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.8.4
    • Fix Version/s: 1.8.6, 2.0-beta-3
    • Component/s: Ant integration
    • Labels:
      None

      Description

      See this thread: http://groovy-eclipse-plugin.42567.n3.nabble.com/Eclipse-Debugging-Ant-and-Groovy-td3689281.html

      GroovyEngine is not creating script names that are understandable by the java debugger. It is basing the generated script name off of the name passed to it by the BSFManager. The only way that the Java debugger can link a compiled script to its source is if the script name matches the name of the source file.

      In this case, scriptdef_ is prepended to the script name and breakpoints will not be reached inside of scripts loaded by BSFManager (ie- all scripts loaded by ant tasks).

      The solution is simple. Change the logic of org.codehaus.groovy.bsf.GroovyEngine.convertToValidJavaClassname(String) so that it checks the name passed in to see if it starts with scriptdef_. If so, then remove it.

      1. GroovyEngine.patch
        2 kB
        Jochen Theodorou

        Activity

        Hide
        Steve Amerige added a comment -

        This is an important issue for us as most of the complex logic, of course, is in the Groovy code. If we're not able to have breakpoints work in the Groovy code, it is a significant handicap for us. We encourage adopting a fix at the earliest opportunity.

        Is there any workaround that we can employ when debugging in existing environments?

        Show
        Steve Amerige added a comment - This is an important issue for us as most of the complex logic, of course, is in the Groovy code. If we're not able to have breakpoints work in the Groovy code, it is a significant handicap for us. We encourage adopting a fix at the earliest opportunity. Is there any workaround that we can employ when debugging in existing environments?
        Hide
        Andrew Eisenberg added a comment -

        I tested a fix locally and made a change to the Groovy-Eclipse groovy core patch to get this working. I committed the change and it will be available in the next dev build, but it would be nice to get this fixed in groovy-core itself before 1.8.6 is out.

        Here is the change I made to GroovyEngine.convertToValidJavaClassname(String), added after the initial if statement:

                // GRECLIPSE: Groovy bug 5266. 
                // ensure that the script name matches the file name for ant scripts
                if (inName.startsWith("scriptdef_")) {
                	inName = inName.substring("scriptdef_".length());
                }
                // GRECLIPSE: End
        

        To get this working, you need to name your script in the build.xml the same as the file name, so in the example that Steve sent me, it would have to look like this:

        <scriptdef name="doit.groovy" id="doit" language="groovy" src="src/doit.groovy"> 
            <element name="sequential" classname="org.apache.tools.ant.taskdefs.Sequential"/>
        <![CDATA[ 
           groovydoit()
        ]]> 
        </scriptdef> 
        

        And then of course referencing it would be like this:

        
        <macrodef name="antdoit"> 
            <element name="body" implicit="true"/>
        <sequential> 
            <doit.groovy>
                <sequential>
                    <body/>
                </sequential>
            </doit.groovy>
        </sequential> 
        </macrodef> 
        

        Until this is incorporated into groovy-core, you need to use Groovy-Eclipse. Install the latest snapshot, and on your ant classpath, include the following jars from inside the org.codehaus.groovy 1.8.5 plugin: groovy-eclipse.jar and groovy-1.8.5.jar. Make sure that groovy-eclipse.jar is before groovy.jar in the classpath.

        Show
        Andrew Eisenberg added a comment - I tested a fix locally and made a change to the Groovy-Eclipse groovy core patch to get this working. I committed the change and it will be available in the next dev build, but it would be nice to get this fixed in groovy-core itself before 1.8.6 is out. Here is the change I made to GroovyEngine.convertToValidJavaClassname(String) , added after the initial if statement: // GRECLIPSE: Groovy bug 5266. // ensure that the script name matches the file name for ant scripts if (inName.startsWith( "scriptdef_" )) { inName = inName.substring( "scriptdef_" .length()); } // GRECLIPSE: End To get this working, you need to name your script in the build.xml the same as the file name, so in the example that Steve sent me, it would have to look like this: <scriptdef name= "doit.groovy" id= "doit" language= "groovy" src= "src/doit.groovy" > <element name= "sequential" classname= "org.apache.tools.ant.taskdefs.Sequential" /> <![CDATA[ groovydoit() ]]> </scriptdef> And then of course referencing it would be like this: <macrodef name= "antdoit" > <element name= "body" implicit= " true " /> <sequential> <doit.groovy> <sequential> <body/> </sequential> </doit.groovy> </sequential> </macrodef> Until this is incorporated into groovy-core, you need to use Groovy-Eclipse. Install the latest snapshot, and on your ant classpath, include the following jars from inside the org.codehaus.groovy 1.8.5 plugin: groovy-eclipse.jar and groovy-1.8.5.jar. Make sure that groovy-eclipse.jar is before groovy.jar in the classpath.
        Hide
        Paul King added a comment -

        So shouldn't the fix be to raise a bug request on the ant project? Asking them not to add this prefix in their ScriptDef#executeScript() method? Or ask them to provide a way to override the full name? I presume if you just use the Groovy ant task for running scripts then debugging is OK?

        I am not necessarily against the proposed fix - it just seems we are adding some ugly code into the Groovy codebase to offset some arguably ugly code in Ant. I regard the proposed Groovy hack as ugly because it stops anyone having a file that actually starts with "scriptdef_".

        Show
        Paul King added a comment - So shouldn't the fix be to raise a bug request on the ant project? Asking them not to add this prefix in their ScriptDef#executeScript() method? Or ask them to provide a way to override the full name? I presume if you just use the Groovy ant task for running scripts then debugging is OK? I am not necessarily against the proposed fix - it just seems we are adding some ugly code into the Groovy codebase to offset some arguably ugly code in Ant. I regard the proposed Groovy hack as ugly because it stops anyone having a file that actually starts with "scriptdef_".
        Hide
        Guillaume Delcroix added a comment -

        I agree with Paul here, that a bug should be raised to the Ant project.
        As a temporary but ugly fix, we can implement that workaround, and remove it once the Ant project developers agree to fix that problem.
        Steve, or Andrew, could either of you please raise the problem to the Ant folks?

        Show
        Guillaume Delcroix added a comment - I agree with Paul here, that a bug should be raised to the Ant project. As a temporary but ugly fix, we can implement that workaround, and remove it once the Ant project developers agree to fix that problem. Steve, or Andrew, could either of you please raise the problem to the Ant folks?
        Hide
        Jochen Theodorou added a comment -

        An addition to Andrew's fix. Afaik the emthod is supposed to return "" for an empty name. If the name was ebfore "scriptdef" only, then removingthat part will lead to "" being used as name. I am pretty sure the fixhas this problem, because the patch doesn't show the line before where this check is done together with the null check and is thus unchanged.

        Show
        Jochen Theodorou added a comment - An addition to Andrew's fix. Afaik the emthod is supposed to return " " for an empty name. If the name was ebfore "scriptdef " only, then removingthat part will lead to "" being used as name. I am pretty sure the fixhas this problem, because the patch doesn't show the line before where this check is done together with the null check and is thus unchanged.
        Hide
        Andrew Eisenberg added a comment -

        I agree that the final fix belongs in ant. My experience with the ant project is that they are generally receptive to small fixes like this, but the turn-around time is long. Steve, any desire to raise a bug on the ant project?

        Show
        Andrew Eisenberg added a comment - I agree that the final fix belongs in ant. My experience with the ant project is that they are generally receptive to small fixes like this, but the turn-around time is long. Steve, any desire to raise a bug on the ant project?
        Hide
        Paul King added a comment -

        Ant is looking at a 1.8.3 release shortly but I don't believe have got a code freeze yet - there might be a small window of opportunity to get a fix relatively quickly.

        Show
        Paul King added a comment - Ant is looking at a 1.8.3 release shortly but I don't believe have got a code freeze yet - there might be a small window of opportunity to get a fix relatively quickly.
        Hide
        Jochen Theodorou added a comment -

        I propose this patch including a little test

        Show
        Jochen Theodorou added a comment - I propose this patch including a little test
        Hide
        Guillaume Delcroix added a comment -

        Patch looking good to me.

        Show
        Guillaume Delcroix added a comment - Patch looking good to me.
        Hide
        Andrew Eisenberg added a comment -

        Fine with me.

        Show
        Andrew Eisenberg added a comment - Fine with me.
        Hide
        Jochen Theodorou added a comment -

        fixed

        Show
        Jochen Theodorou added a comment - fixed

          People

          • Assignee:
            Jochen Theodorou
            Reporter:
            Andrew Eisenberg
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development