groovy
  1. groovy
  2. GROOVY-3169

New compiler class loading strategy

    Details

    • Type: Improvement Improvement
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Incomplete
    • Affects Version/s: 1.6-rc-1
    • Fix Version/s: None
    • Component/s: Compiler
    • Labels:
      None
    • Testcase included:
      yes
    • Patch Submitted:
      Yes
    • Number of attachments :
      1

      Description

      The Groovy compiler suffers from the problem that its compile classpath (i.e. the classpath used to search for types during compilation) is polluted by the environment in which the compiler is run. The underlying problem is that the class loader used for loading classes from the compile classpath is not strictly separated from the class loader used to run the compiler itself. Several compiler clients try to work around this problem: Gradle and the Groovy Ant task by forking a new JVM, GMaven by providing a custom class loader. However, these workarounds can lead to new problems. For example, AST transformations introduced in Groovy 1.6 don't work under GMaven, and there doesn't seem to be an easy fix.

      This patch provides an implementation of a new compiler class loading strategy that aims to:

      • avoid any pollution of the compile classpath
      • load AST transformations in a reliable manner
      • make AST transformations work under GMaven
      • enable cross-compilation against other Groovy versions (once supported by the compiler)
      • allow compiler clients to upgrade to the new class loading strategy individually and at their own decision
      • be careful about changes to the existing code base

      The patch is based around a new class loader named CompilationClassLoader that may replace GroovyClassLoader as a CompilationUnit's class loader. By default, CompilationClassLoader searches for classes in the following locations (in the given order): the bootstrap classpath, the extension classpath, and the compile classpath. Only classes on the bootstrap classpath are shared with other class loaders.

      AST transformations are loaded with a separate URLClassLoader that shares with the compiler all classes on the compiler's classpath but also searches the compile classpath. Although it might be worthwile to try and reduce the number of classes shared with the compiler, this approach should be good enough for the moment.

      So far, only FileSystemCompiler has been adapted to (by default) use CompilationClassLoader instead of GroovyClassLoader. Judging from the fact that the Groovy build still succeeds, this works fine for the Groovy Ant task with fork enabled (which uses FileSystemCompiler under the hood). Building my personal Groovy project with one of groovyc, Gradle, or GMaven also works as expected. Finally, AST transformations now work under GMaven. However, since class loading is a delicate topic, more extensive tests, with different inputs and in different environments, should be conducted.

      Note: When compiling programs that reference types from the Groovy Jar, the latter must be available from one of CompilationClassLoader's search locations (see above). While this is usually not an issue with IDEs and build tools (because they tend to put the Groovy Jar on the compile classpath anyway), it means a change in behavior for groovyc. One way to avoid this is to adapt the groovyc script to add the Groovy Jar automatically.

      Please give me some feedback as to whether you consider to approve this patch, in one form or another.

        Issue Links

          Activity

          Hide
          Guillaume Laforge added a comment -

          Scheduled for 1.7, perhaps a bit late for 1.6?

          Show
          Guillaume Laforge added a comment - Scheduled for 1.7, perhaps a bit late for 1.6?
          Hide
          Peter Niederwieser added a comment -

          I agree that it's a bit late for 1.6. My main concern with scheduling this for 1.7 is that 1.6 might be released without GMaven supporting AST transformations. If that happened, many projects would not be able to use AST transformations, possibly for a long time to come. (This would especially hurt the adoption of upcoming frameworks like my own which are completely based on AST transformations.)

          Show
          Peter Niederwieser added a comment - I agree that it's a bit late for 1.6. My main concern with scheduling this for 1.7 is that 1.6 might be released without GMaven supporting AST transformations. If that happened, many projects would not be able to use AST transformations, possibly for a long time to come. (This would especially hurt the adoption of upcoming frameworks like my own which are completely based on AST transformations.)
          Hide
          Guillaume Laforge added a comment -

          Good point about GMaven and AST Transformations.
          We'll have a closer look at the patch to see if it's doable to include it in 1.6 or not.

          I'd also be curious to hear more about your upcoming BDD framework and when/if it's going to be released some day

          Show
          Guillaume Laforge added a comment - Good point about GMaven and AST Transformations. We'll have a closer look at the patch to see if it's doable to include it in 1.6 or not. I'd also be curious to hear more about your upcoming BDD framework and when/if it's going to be released some day
          Hide
          Peter Niederwieser added a comment -

          My plan has always been to release my project shortly after the release of Groovy 1.6, so I guess I'm still on track.

          Show
          Peter Niederwieser added a comment - My plan has always been to release my project shortly after the release of Groovy 1.6, so I guess I'm still on track.
          Hide
          Pascal Schumacher added a comment -

          Is there still interest in applying this or should this issue be closed?

          Show
          Pascal Schumacher added a comment - Is there still interest in applying this or should this issue be closed?
          Hide
          Pascal Schumacher added a comment -

          I guess nobody is interested in applying this anymore, so I'm closing this. Please reopen if I'm wrong.

          Show
          Pascal Schumacher added a comment - I guess nobody is interested in applying this anymore, so I'm closing this. Please reopen if I'm wrong.

            People

            • Assignee:
              Unassigned
              Reporter:
              Peter Niederwieser
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: