Mojo's JavaCC Maven Plugin
  1. Mojo's JavaCC Maven Plugin
  2. MJAVACC-30

Use generated Java files themselves for stale source detection

    Details

    • Type: Improvement Improvement
    • Status: Closed Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.4
    • Component/s: javacc
    • Labels:
      None
    • Environment:
      Maven 2.0.7, JDK 1.5.0_12, WinXP
    • Patch Submitted:
      Yes
    • Number of attachments :
      1

      Description

      If I understand the StaleSourceScanner correctly, the parameter timestampDirectory should be unnecessary. The same effect of output generation only upon modified/updated grammar files could be achieved by comparing the timestamps of the grammar files against the timestamps of the java files (and not copies of the grammar files). The required code changes should be minimal, i.e. simply change lines like

      SuffixMapping mapping = new SuffixMapping( ".jj", ".jj" );

      to

      SuffixMapping mapping = new SuffixMapping( ".jj", ".java" );

      and pass the (base) output directory rather than the timestamp directory into computeStaleGrammars().

      The proposed change should yield the same conditional output generation for the use case where the output directory is set to something under $

      {basedir}/target. However, it should perform better for such use cases where the java files are generated into a non-temporary/version-controlled directory like ${basedir}

      /src/main/java. Currently, a "mvn clean generate-sources" will always trigger the generation of output regardless whether necessary or not because the timestamp files do not exist (any more). Taking the output files for the timestamp test should avoid this unwanted effect.

        Issue Links

          Activity

          Hide
          Paul Gier added a comment -

          I think the problem is that the StaleSourceScanner compares files in matching directory structures. So what you are suggesting would work fine in most project setups, but in the occasional case where the package names of your generated files does not match the source location of the .jj files, the scanner would not be able to find them.
          I think you would have to compare each file individually to get around this.

          Show
          Paul Gier added a comment - I think the problem is that the StaleSourceScanner compares files in matching directory structures. So what you are suggesting would work fine in most project setups, but in the occasional case where the package names of your generated files does not match the source location of the .jj files, the scanner would not be able to find them. I think you would have to compare each file individually to get around this.
          Hide
          Benjamin Bentmann added a comment -

          Oh, I see, that might as well be the reason why the timestamp directory was introduced in the first place ...

          Nevertheless, I still consider checking timestamps against the generated java files a useful feature (not to mention that this was the default behavior of the corresponding Ant task). For the moment, all that seems to be needed is some hint for the plugin which usage scenario applies. What about a simple boolean configuration parameter that would do the switch, i.e. tell the plugin that the directory structure for the grammar files matches the final package structure and enable it to use the java files for timestamp checking?

          In theory, there should also be a way to avoid both the timestamp directory and some additional parameter like sketched above. The plugin knows fairly well where javacc stores the output files so telling the location of an output file for a given grammar file is no magic. All that causes troubles is the combination of StaleSourceScanner and SuffixMapping that is not quite up the job. Maybe we can just use a customized SourceMapping and all would be fine. I will investigate this possibility.

          Show
          Benjamin Bentmann added a comment - Oh, I see, that might as well be the reason why the timestamp directory was introduced in the first place ... Nevertheless, I still consider checking timestamps against the generated java files a useful feature (not to mention that this was the default behavior of the corresponding Ant task). For the moment, all that seems to be needed is some hint for the plugin which usage scenario applies. What about a simple boolean configuration parameter that would do the switch, i.e. tell the plugin that the directory structure for the grammar files matches the final package structure and enable it to use the java files for timestamp checking? In theory, there should also be a way to avoid both the timestamp directory and some additional parameter like sketched above. The plugin knows fairly well where javacc stores the output files so telling the location of an output file for a given grammar file is no magic. All that causes troubles is the combination of StaleSourceScanner and SuffixMapping that is not quite up the job. Maybe we can just use a customized SourceMapping and all would be fine. I will investigate this possibility.
          Hide
          Benjamin Bentmann added a comment - - edited

          Unfortunately, Plexus' SourceMapping does not offer a method like getTargetFiles( File targetDir, File sourceDir, String source ) so my current solution is not really pretty.

          The new file JavaCCGrammarInfo is intended to replace JavaCCUtil (from which most of its code is copied). In addition to determing the package name for a parser file, it also gets the parser name. Having both pieces of information in a single bean (and possibly avoiding multiple scans of the grammar file contents) is the sole purpose of this class.

          The algo to get the parser name is currently simplistic: It just assumes that "MyParser.jj" produces "MyParser.java". A proper solution would be to get the PARSER_BEGIN() statement from the grammar file. The corresponding Ant task also follows the simplistic filename based approach and apparently works well enough, so excuse my lazyness.

          The major part of the (yet incomplete) patch is JavaCCSourceMapping. It is fed with the source directory being scanned (to workaround the above mentioned limitation of SourceMapping) and the package name configured for the plugin. Now, JavaCCSourceMapping can point the StaleSourceScanner to the actual output files of javacc.

          To integrate this into the JavaCCMojo, the following changes to computeStaleGrammars() should be sufficient:

          SourceMapping mapping = new JavaCCSourceMapping( sourceDir, packageName );
          
          SourceInclusionScanner scanner = new StaleSourceScanner( staleMillis, includes, excludes );
          
          scanner.addSourceMapping( mapping );
          ...
              staleSources.addAll( scanner.getIncludedSources( sourceDir, outputDir ) );
          

          Wouldn't this work for everybody?

          Show
          Benjamin Bentmann added a comment - - edited Unfortunately, Plexus' SourceMapping does not offer a method like getTargetFiles( File targetDir, File sourceDir, String source ) so my current solution is not really pretty. The new file JavaCCGrammarInfo is intended to replace JavaCCUtil (from which most of its code is copied). In addition to determing the package name for a parser file, it also gets the parser name. Having both pieces of information in a single bean (and possibly avoiding multiple scans of the grammar file contents) is the sole purpose of this class. The algo to get the parser name is currently simplistic: It just assumes that "MyParser.jj" produces "MyParser.java". A proper solution would be to get the PARSER_BEGIN() statement from the grammar file. The corresponding Ant task also follows the simplistic filename based approach and apparently works well enough, so excuse my lazyness. The major part of the (yet incomplete) patch is JavaCCSourceMapping. It is fed with the source directory being scanned (to workaround the above mentioned limitation of SourceMapping) and the package name configured for the plugin. Now, JavaCCSourceMapping can point the StaleSourceScanner to the actual output files of javacc. To integrate this into the JavaCCMojo, the following changes to computeStaleGrammars() should be sufficient: SourceMapping mapping = new JavaCCSourceMapping( sourceDir, packageName ); SourceInclusionScanner scanner = new StaleSourceScanner( staleMillis, includes, excludes ); scanner.addSourceMapping( mapping ); ... staleSources.addAll( scanner.getIncludedSources( sourceDir, outputDir ) ); Wouldn't this work for everybody?
          Hide
          Benjamin Bentmann added a comment -

          Unfortunately, Plexus' SourceMapping does not offer a method like getTargetFiles( File targetDir, File sourceDir, String source ) so my current solution is not really pretty.

          Some time ago I created a feature request for this (PLXCOMP-85).

          Show
          Benjamin Bentmann added a comment - Unfortunately, Plexus' SourceMapping does not offer a method like getTargetFiles( File targetDir, File sourceDir, String source ) so my current solution is not really pretty. Some time ago I created a feature request for this ( PLXCOMP-85 ).
          Hide
          Paul Gier added a comment - - edited

          I was hesitant to apply this patch since the current timestamp directory method seems to be working fine. For now I'm just going to schedule it for a future release (2.x). If the issue is important for you, we can put it in the 2.3 release, otherwise I'll just wait to see if it gets any more votes.

          Show
          Paul Gier added a comment - - edited I was hesitant to apply this patch since the current timestamp directory method seems to be working fine. For now I'm just going to schedule it for a future release (2.x). If the issue is important for you, we can put it in the 2.3 release, otherwise I'll just wait to see if it gets any more votes.
          Hide
          Benjamin Bentmann added a comment -

          I consider the patch quite a big change, so I totally agree to schedule it for a later release. The focus should be to get v2.3 out quickly as it will resolve a blocker.

          Show
          Benjamin Bentmann added a comment - I consider the patch quite a big change, so I totally agree to schedule it for a later release. The focus should be to get v2.3 out quickly as it will resolve a blocker.
          Hide
          Benjamin Bentmann added a comment -

          Committed in r6233.

          Show
          Benjamin Bentmann added a comment - Committed in r6233.

            People

            • Assignee:
              Benjamin Bentmann
              Reporter:
              Benjamin Bentmann
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: