Let me first try to summarize how this works
defines the plugins, javac.ExternalCompilerPlugin is the interface/abstract class for the plugins. This is according to the SPI convention used in java5. This class contains a method createCompilationUnit, which obviously means that each plugin should create its own CompilationUnit variation. This seems to imply, that one plugin decides for the others how to compile, as Groovy does not support multiple CompilationUnits for one compilation... The CompilationUnit is the compilation, so it wouldn't make sense to do so. But if we cannot have multiple plugins active, then why the SPI stuff? The getFileExt() (returns the file extension used for the compilation) IMHO suggests that only one file extension name is possible... what will then happen for java files?
Since ExternalCompilationUnit is taking a JavaAwareCompilationUnit and a plugin in the constructor, I would have expected that ExternalCompilationUnit is working as some kind of proxy or at last uses the JavaAwareCompilationUnit for configuration, but nothing of that seems to be the case. Instead protected fields are set, usage unclear. ExternalCompilationUnit seems to manage its own sources, addSource and clearSources suggest that. Unclear to me is why this has to be a Set (we used List so far).
Both compiler plugins seem to use JavaCompiler based classes, which theoretically enables the usage from ant and command line. But in case of the command line I see no change that would finally link that in and it does not happen automatically. So this seems to be something that we still have to do
The orchestration of the change should then be in one of the CompilationUnit... since only JavaAwareCompilationUnit is changed, it must happen there. JavaAwareCompilationUnit used a compilerFactory property to enable to plugin a different java compiler. The ant task is using that for example to use ant's javac frontend instead of any internal lookup done by Groovy directly. I also suggested that usage for the eclipse plugin and I think GMaven wanted to use that too. Since that part is important for compatibility any change here is to be seen as bad. From looking at current groovyc I see here a regression, probably because of the changes for a forked compilation mode. This needs to be fixed. But anyway... even if the compilerFactory property is currently not used internally, doesn't mean it can be just removed. Back to the orchestration... ExternalCompilerPlugin.getPlugins is used to get a list of plugins (2 by default), we do stub generation if any of these plugins has sources. As long as any plugin has sources we do compilation with each plugin even if the plugin has no sources. sources are added using ExternalCompilationUnit#addSource, so each "plugin" decides if it adds the source or not. In fact it will return true if it does, so a source consumed by one plugin is no longer available to another plugin. Since we have two plugins active by default, this means we have scala sources going to the scala plugin, java sources to the java plugin and all other sources are then added to the controlling CompilationUnit... in a mixed compilation of java, scala and groovy sources this means then all groovy sources. Since only one file extension is supported by this code the scala plugin cannot use steal java files. That also means that any joint compilation probably done by the scala plugin won't happen.
So once joint compilation is activated the groovyc task and the command line compiler will both be able to use joint complication with java sources.
Then some notes and discussion points:
- a initial patch should be more or less minimal and should not contain dead code or surplus code
- The code reading the plugin duplicates the code used for global transforms. Obviously we should make that code into a utility class instead and use it for both. But this should be done independent of this issue. See ExternalCompilerPlugin.getPlugins
- ExternalCompilerPlugin.getPlugins does a loop through all classloaders... why? The code for global transforms does not do this. So I see that as possible bug either here or there... or at last as surplus code, which means the patch has to be changed.
- public abstract ExternalCompilationUnit createCompilationUnit (JavaAwareCompilationUnit compilationUnit) in ExternalCompilerPlugin takes a JavaAwareCompilationUnit, isn't that leaky abstraction? It makes sense for the scala version alone, but we try to define here something more general and the above means, that watever it is, it has to depend on a CompilationUnit created for joint compilation of groovy and java files. And that would be the case even if no java files are involved.
- ExternalCompilerPlugin(String name, List<String> before, List<String> after) uses two list which are assigned to protected fields, but those are never used in your patch. This looks like dead code, left over from refactorings on your side. That means it can be removed, which is a good thing, since the protected fields would leak abstraction.
- ExternalCompilerPlugin#getPlugins does operate on a List and is itself void. It does populate the list from an empty one, so why does it not return the list and create it inside in the first place? Also this works on a List<ExternalCompilationUnit>, which is not the plugins itself, but the CompilationUnit. If we forget that the name obviously does not fit, why add the compilation units and not the plugins?
- After before/after is removed JavacExternalCompilerPlugin will no longer have to use Collections.EMPTY_LIST. Collections.EMPTY_LIST creates a list, yes, but one you can't add an element to. This IMHO strengthens my interpretation that before/after has no meaning.
- If ExternalCompilerPlugin.getName() and the static method are the only methods with an actual implementation, then it should be considered to make the abstract class into an interface.
- ExternalCompilerPlugin.getFileExt() should probably renamed to getDefaultFileExtension. For one we shouldn't fear the long method name and for second it is questionable if the mapping extension->language is always unique. So we shold think of a configuration option here. This doesn't have to be included in the initial design, but it should be considered when doing the infrastructure.
- ExternalCompilationUnit as a private field suffix and mask can be extracted from the plugin. In case of suffix keeping the value is IMHO ok, because it is a internal information, but in case of mask it is returned outside and there should be the question if that really is needed. Also depending here on getFileExt naturally excludes configurable file extensions. Mabye that is a bad idea after all?
- ScalacScalaCompiler#makeParameters seems to give all options to the scala compiler as they are, only target not. target instead goes to "jvm-"+namedValues[i + 1]. Scala seems to support jvm-1.5,jvm-1.4, msil and cldc, I think we should react with an error for the other two at last. And maybe we should do something about target 1.2 for example too, since that is a quite common option.
- ScalacScalaCompiler does overwrite the only visible method of JavacJavaCompiler and does not use super... then why extend JavacJavaCompiler at all?
- ScalacScalaCompiler implements ExternalCompiler, which more or less duplicates JavaCompiler. Now I agree that the naming doesn't fit any more, but ExternalCompiler is used only there and no where else. Not in a cast or method method definition - nowhere. This makes it surplus and should be removed. No need to add misleading dead code.
- in JavacJavaCompiler a field is changed from protected to private, this looks very surplus
- in JavacJavaCompiler the method signature is changed to accept a Set... no need to break with compatibility here, the same applies to JavaCompiler and the field changes in JavaAwareCompilationUnit for sources
- in FileSystemCompiler JavaAwareCompilationUnit is constructed with an additional null argument. The old constructor did the same. IMHO this change is surplus and touches just another files, that doesn't need to be touched. So that change should be removed.
- in build.xml the call order of -includeResources and -stagecompile is now switched... is this really intended?
- the changes to groovyc do need a rework, so I would exclude all related changes from the patch for now.