dtrott / maven-thrift-plugin

Maven Thrift Plugin that executes the thrift code generator (base on protocol buffers plugin)
120 stars 68 forks source link

Support multiple invocations? #3

Open cherron opened 14 years ago

cherron commented 14 years ago

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.

dtrott commented 14 years ago

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?

cherron commented 14 years ago

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?

cherron commented 14 years ago

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 / ?

cherron commented 14 years ago

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?

cherron commented 14 years ago

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: 19dd7083e4a90172976c

dtrott commented 14 years ago

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.

cherron commented 14 years ago

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:

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.

cherron commented 14 years ago

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>
nwparker commented 7 years ago

This would be nice to have