dtrott/maven-thrift-plugin

Support multiple invocations?

cherron opened this issue · 9 comments

Feature suggestion: it would be cool if it were possible to have multiple invocations of the thrift compiler, varying the param in order to generate source for multiple languages.

I understand that the plug in itself may not be configurable enough to define all the options necessary for non-java code however I think it should be possible to invoke the compiler multiple times using standard maven syntax.

What is it that you are trying to do?

I apologize, I didn't realize that I could configure two Maven executions of the same plugin
with different config. My aim is to generate both Java and C++ source from my Thrift IDL.
I updated my pom to run the plugin twice like so:

        <plugin>
            <groupId>org.apache.thrift.tools</groupId>
            <artifactId>maven-thrift-plugin</artifactId>
            <version>0.1.8</version>
            <configuration>
                <thriftExecutable>/usr/local/bin/thrift</thriftExecutable>                    
            </configuration>
            <executions>
                <execution>
                    <id>thrift-java-generation</id>
                    <configuration>
                        <generator>java</generator>
                    </configuration>
                    <goals>
                        <goal>compile</goal>
                    </goals>
                </execution>
                <execution>
                    <id>thrift-cpp-generation</id>
                    <configuration>
                        <generator>cpp</generator>
                    </configuration>
                    <goals>
                        <goal>compile</goal>
                    </goals>
                </execution>
            </executions>
        </plugin> 

Running with cpp (whether alone or in addition to the java generation) results
in an NPE:

 java.lang.NullPointerException
at org.apache.thrift.maven.Thrift.moveGeneratedFiles(Thrift.java:115)
at org.apache.thrift.maven.Thrift.compile(Thrift.java:84)
at org.apache.thrift.maven.AbstractThriftMojo.execute(AbstractThriftMojo.java:158)
at org.apache.thrift.maven.ThriftCompileMojo.execute(ThriftCompileMojo.java:22)
at org.apache.maven.plugin.DefaultPluginManager.executeMojo(DefaultPluginManager.java:490)

... looking at the code, the Thrift class assumes that the generated source is placed in "gen-java". Can I suggest that the outputDirectory be made configurable and passed to the thrift compiler?

Clarification: outputDirectory is indeed configurable, but the method Thrift.moveGeneratedFiles() assumes genDir to be outputDirectory/GENERATED_JAVA.
It looks like the output subdirectory always matches the generator value, so perhaps it would be safe to assume a convention of / ?

Doh, markdown hijacked my last sentence. I meant to say: perhaps it would be safe to assume a convention of outputDirectory + generator for the genDir?

Turned out the issue was some file deletes and moves that assume only a single run for one generator configuration. I forked and committed some tweaks that allowed me to successfully run multiple executions and retain all generated source: 19dd708

I have reviewed your fixes.
Originally the moves were put in to address an issue with some IDE's not liking the additional directory nesting (gen-java).

That said I agree that, the moving files "solution" was a hack, I am trying to think of a better solution, one option may be to have two directories:

Working directory - Where the thrift compiler outputs to.
Output directory - Which maven will add to the build path, to trigger the standard java compile cycle.

Instead of cleaning and moving at two points in the cycle we could just delete the output directory and overwrite with the entire contents of the workingdir/gen-java

Three questions:
1.) Would something like that work for you?
2.) Is there anything specical you need for output directories from multiple runs?
3.) Any chance you want to take at coding it ;-)

PS I know that this sounds a bit convoluted but I am trying not to break anyone who is already using the plug-in.

  1. Yes, that might work.
  2. Not really, as long as there's a way to avoid one run clobbering another's output.
  3. I had a go at coding it: cherro/maven-thrift-plugin@068b89c71ea284682966fad0650492cadd4f506d
    (hopefully my GitHub markdown is correct!)

Some notes:
I assumed it was reasonable to use a temporary directory for the working directory, in order to avoid having to require another user-specified parameter.

I added two boolean configuration flags:

  • cleanBeforeOutput, which is true by default. This would delete the specified outputDirectory before adding the generated source. Setting to false would accommodate the scenario where there are other things that need to be retained (e.g. writing directly into ${project.basedir}/src/main/java in order to share the generated source via the VCS).
  • compileOutput, which is true by default. This would cause the outputDirectory to be added Setting to false would accommodate the scenario where we don't want to compile, or maven cannot compile the generated source (e.g. C++).

The behavior for each invocation is:

  1. Create a uniquely named temporary directory for the workDirectory.
  2. Pass that directory path to the thrift executable
  3. After the thrift execution, expect to find a single directory in the workDirectory, with the prefix "gen-".
  4. Move all contents beneath that single "gen-" parent directory into the outputDirectory.
  5. If compileOutput = true, then add the outputDirectory to the maven build path.

Would you mind reviewing, in particular to see if there's any obvious impact to other use cases? If the changes are reasonable, there would need to be some more cleanup (JavaDoc etc.) to reflect the broader usage beyond Java-only generation.

Here is some sample config for two runs:

<properties>
    <thrift.executable>/usr/local/bin/thrift</thrift.executable>
    <maven-thrift-plugin.base.output.dir>
        ${project.basedir}/target/generated-sources/thrift
    </maven-thrift-plugin.base.output.dir>
    <maven-thrift-plugin.thrift.src.dir>
        ${project.basedir}/src/main/resources
    </maven-thrift-plugin.thrift.src.dir>
</properties>

<build>
    <plugins>
        <plugin>
            <groupId>org.apache.maven.plugins</groupId>
            <artifactId>maven-compiler-plugin</artifactId>
            <configuration>
                <source>1.6</source>
                <target>1.6</target>
            </configuration>
        </plugin>
        <plugin>
            <groupId>org.apache.thrift.tools</groupId>
            <artifactId>maven-thrift-plugin</artifactId>
            <version>0.1.9-SNAPSHOT</version>
            <configuration>
                <thriftExecutable>${thrift.executable}</thriftExecutable>
                <thriftSourceRoot>${maven-thrift-plugin.thrift.src.dir}
                </thriftSourceRoot>
                <cleanBeforeOutput>true</cleanBeforeOutput>
            </configuration>
            <executions>
                <execution>
                    <id>thrift-cpp-generation</id>
                    <configuration>
                        <generator>cpp</generator>
                        <outputDirectory>
                            ${maven-thrift-plugin.base.output.dir}/cpp
                        </outputDirectory>
                        <compileOutput>false</compileOutput>
                    </configuration>
                    <goals>
                        <goal>compile</goal>
                    </goals>
                </execution>
                <execution>
                    <id>thrift-java-generation</id>
                    <configuration>
                        <generator>java</generator>
                        <outputDirectory>
                            ${maven-thrift-plugin.base.output.dir}/java
                        </outputDirectory>
                        <compileOutput>true</compileOutput>
                    </configuration>
                    <goals>
                        <goal>compile</goal>
                    </goals>
                </execution>
            </executions>
        </plugin>
    </plugins>
</build>

This would be nice to have