fix(regression): add created source files to spoon input #70

MartinWitt · 2020-05-15T15:19:44Z

See (INRIA/spoon#3366) for details. Currently there is only a test project with a simple javacc case and a addition to the CI as starting point for the bug.
Edit: this should now fully showcase the bug, with a minimal working example

MartinWitt · 2020-05-15T17:20:59Z

Looks fixed, but

Changelog still missing
Remove hardcoded version
Refactor the added source code
Try fix for the xwiki bug
Create a regression test from the xwiki snippet

Everything expect the last bulletpoint needs to be done before a merge.
@vmassol are there some deadlines you like to keep?

src/main/java/fr/inria/gforge/spoon/configuration/AbstractSpoonConfigurationBuilder.java

vmassol · 2020-05-15T17:26:26Z

@MartinWitt what I don't understand is that getCompileSourceRoots() should also return generated sources paths.

@vmassol are there some deadlines you like to keep?

No it's ok, no urgency, we'll release XWiki 12.4 without this and it'll go in 12.5 (next month release).

Thanks a lot.

MartinWitt · 2020-05-15T17:33:27Z

My first thought was, that it should return it too. But looking https://maven.apache.org/guides/introduction/introduction-to-the-standard-directory-layout.html and https://stackoverflow.com/questions/30039613/maven-plugin-get-generated-sources-directory it seems like maven has no standard defined for this and cant resolve generated-source for you. And I found no method to get generated-sources.

vmassol · 2020-05-15T17:47:25Z

Normally when you code a plugin that generates sources you add the target source directory to the source roots. Maybe the javacc plugin is not compliant and doesn't do it?

vmassol · 2020-05-15T17:51:41Z

Maybe the javacc plugin is not compliant and doesn't do it?

ok I think that's the reason, the old codehaus javacc mojo is not respecting the rules from what I see: https://github.com/mojohaus/javacc-maven-plugin/blob/master/src/main/java/org/codehaus/mojo/javacc/JavaCCMojo.java#L170

vmassol · 2020-05-15T17:53:15Z

I see there's a new plugin version at https://github.com/phax/ph-javacc-maven-plugin

However, I don't see much changes:

https://github.com/phax/ph-javacc-maven-plugin/blob/master/src/main/java/org/codehaus/mojo/javacc/JavaCCMojo.java#L161

MartinWitt · 2020-05-15T17:57:22Z

Oh thank you for your research i have only searched the bug on our side. Now we change from a bug to supporting maven plugins that work "incorrectly", but we could fix this on our side. Javacc looks like a often used code generator, so dropping support is huge step.

vmassol · 2020-05-15T17:57:38Z

So I've upgraded to this new plugin and I have the same error on the spoon side.

vmassol · 2020-05-15T18:02:44Z

This is an example of a well-written plugin: https://github.com/antlr/antlr4/blob/master/antlr4-maven-plugin/src/main/java/org/antlr/mojo/antlr4/Antlr4Mojo.java#L307

As you can see it calls addCompileSourceRoot() at https://github.com/antlr/antlr4/blob/master/antlr4-maven-plugin/src/main/java/org/antlr/mojo/antlr4/Antlr4Mojo.java#L201

vmassol · 2020-05-15T18:05:06Z

Issue opened on the javacc plugin side at tulipcc/tulipcc-maven-plugin#17

MartinWitt · 2020-05-15T18:07:49Z

You are really fast, thank you for your effort. Lets see what javacc developers say to your issue.
But we should keep in mind, that we could fix things like this on our site, if more maven plugins are not "good Maven citizen".

vmassol · 2020-05-15T18:10:19Z

Yes I don't think we should wait on them.

vmassol · 2020-05-15T19:18:08Z

@MartinWitt they have answered and it seems they call the addCompileSourceRoot() so normally you should get it. See tulipcc/tulipcc-maven-plugin#17 (comment)

It's also called with the older codehaus javacc maven plugin, see https://github.com/mojohaus/javacc-maven-plugin/blob/739194d58700260c4b600cd1bbc40979dfd8f851/src/main/java/org/codehaus/mojo/javacc/AbstractJavaCCMojo.java#L657

… Lets test it

MartinWitt · 2020-05-16T15:09:40Z

hmm looks like my bad. You are 100% correct, it works without adding the lines.
Edit: testing the "new" version on xwiki as soon, as the nexus.xwiki server seems working.

MartinWitt · 2020-05-18T18:18:45Z

I changed the pom xwiki-rendering-wikimodel to force spoon 3.5-SNAPSHOT (version with fix) and rann mvn install -DskipTests > spoon.log. As you see in the log it works now?

      <plugin>
        <groupId>fr.inria.gforge.spoon</groupId>
        <artifactId>spoon-maven-plugin</artifactId>
        <version>3.5-SNAPSHOT</version>
      <executions>
        <execution>
          <goals>
            <goal>check</goal>
          </goals>
        </execution>
        </executions>
        <configuration>
          <includeTest>true</includeTest>
          <includeSource>true</includeSource>
          <noClasspath>false</noClasspath>
        </configuration>
        </plugin>```

[spoon.log](https://github.com/SpoonLabs/spoon-maven-plugin/files/4645876/spoon.log)

vmassol · 2020-05-18T18:21:16Z

As you see in the log it works now?

It does! Cool, thanks a lot :)

MartinWitt · 2020-05-18T18:25:03Z

The hardcoded versions can only be removed after release, because maven does not support use newest version like gradle. Only point left is create a real regression testcase for this. I hope I can get this done tomorrow/tonight.

vmassol · 2020-05-18T18:53:48Z

@MartinWitt what hardcoded versions?

MartinWitt · 2020-05-18T18:55:28Z

https://github.com/SpoonLabs/spoon-maven-plugin/pull/70/files#diff-403631b4cad466f44faaca502d672427R38 like this.
It's unrelated to your issue and more something i need to fix after a release.

vmassol · 2020-05-18T19:08:33Z

src/test/projects/javacc-generate/pom.xml

+    <plugin>
+      <groupId>fr.inria.gforge.spoon</groupId>
+      <artifactId>spoon-maven-plugin</artifactId>
+      <version>3.5-SNAPSHOT</version>


Why don't you use a filtered resource for example?

See:

https://github.com/xwiki/xwiki-commons/blob/46e0ef7112577803a00e0c7f1abc035be38a8bf2/xwiki-commons-tools/xwiki-commons-tool-xar/xwiki-commons-tool-xar-integration-tests/pom.xml#L55

https://github.com/xwiki/xwiki-commons/blob/0a56fb5fb8c59ea93bc09a5897c929322b64265a/xwiki-commons-tools/xwiki-commons-tool-xar/xwiki-commons-tool-xar-integration-tests/src/test/resources/wrongVersion/pom.xml

because i normally use gradle and didn't knew it. Going to have a look thx.

What about simply removing <version>3.5-SNAPSHOT</version> as you did above?

If you remove the version the build won't be reproducible and maven will use the latest version available. You need a version (and you should actually use the maven enforcer to always make sure all versions are set).

MartinWitt · 2020-05-20T16:18:01Z

To finish this bugfix/issue, the task were:

Changelog still missing
Remove hardcoded version
Refactor the added source code
Try fix for the xwiki bug
Create a regression test from the xwiki snippet

is added, see link
happens after merge, when we have a new version number.( @vmassol solution requires a resource file or i understood something wrong ( i'm far away from maven expert))
Done
See logfile, it works now.
I believe adding xwiki with generated files as a regression test only slows down the CI and creates a strange dependency if some errors occur. The current test should test the same in in smaller scale( use a generated java file in fullClassPath).

Looks like all is done and @monperrus time for a review?

INRIA/spoon#3345 is still open, but i believe a version like 3.4.1 shouldn't contain new features. This issue is part of patch 3.5?

monperrus · 2020-05-21T05:40:30Z

Thanks a lot for the progress. LGTM, see last comment.

Do we already have a fix for INRIA/spoon#3345?

If yes, let's merge it now and release 3.5 directly and save one release.
If no, let's release directly 3.4.1, per this PR.

MartinWitt · 2020-05-21T11:39:28Z

Looking here we use fullclasspathmode by default. The issue is only changing a parameter?

monperrus · 2020-05-22T05:21:55Z

Looking here we use fullclasspathmode by default.

So INRIA/spoon#3345 is already fixed, and we can add it in the Changelog.

I think we can proceed, merge this PR and release 3.4.1. WDYT?

MartinWitt · 2020-05-22T12:05:38Z

just did a small readme update and yes i believe you can merge it.
@vmassol maybe a unrelated question but could we link to the xwiki usage, when you integrated it into it? A real world example is always nice to have.

vmassol · 2020-05-22T12:11:05Z

@vmassol maybe a unrelated question but could we link to the xwiki usage, when you integrated it into it? A real world example is always nice to have.

Definitely, with pleasure :)

FYI, on the xwiki.org site we documented our spoon usages at https://dev.xwiki.org/xwiki/bin/view/Community/Building/#HAutomaticChecks

monperrus · 2020-05-22T12:45:27Z

Merged, thanks a lot @MartinWitt

Now releasing 3.4.1

monperrus · 2020-05-22T13:12:45Z

FYI, 3.4.1 is in Maven Central

vmassol · 2020-05-22T19:55:14Z

Thanks @MartinWitt and @monperrus

I've now tested v3.4.1 and I've found a regression: INRIA/spoon#3376

I've rolled-back to v3.3 for now.

Thanks! (and sorry ;))

MartinWitt added 7 commits May 15, 2020 17:16

add project to showcase the problem with javacc

6220d47

ignore testcase tempoary

d1e70d8

remove version tag

f5c324b

remove version tag

aa88130

add dependency

1c1c46e

fix pom

74f3d70

fix generated src file inclusion, change mvn install -> package

1486774

MartinWitt commented May 15, 2020

View reviewed changes

src/main/java/fr/inria/gforge/spoon/configuration/AbstractSpoonConfigurationBuilder.java Outdated Show resolved Hide resolved

add missing changelog for version 3.4 and 3.4.1

859c194

according to the research, adding to compileSourceRoot is not needed.…

d412d6c

… Lets test it

MartinWitt added 2 commits May 16, 2020 17:10

remove unneeded lines

9cf0d45

update testcase, because file input has changed

ba44287

MartinWitt changed the title ~~add project to showcase the problem with javacc~~ fix(regression): add created source files to spoon input May 16, 2020

vmassol reviewed May 18, 2020

View reviewed changes

MartinWitt added 4 commits May 19, 2020 10:42

remove unused import

a076af1

update changelog

63d86dc

update javadoc

fecece2

update javadoc

b6e5f5a

MartinWitt added 2 commits May 22, 2020 14:01

update changelog and readme

fa271fc

fix formatting

86a1c17

monperrus merged commit f2bc42c into SpoonLabs:master May 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(regression): add created source files to spoon input #70

fix(regression): add created source files to spoon input #70

MartinWitt commented May 15, 2020 •

edited

Loading

MartinWitt commented May 15, 2020 •

edited

Loading

vmassol commented May 15, 2020

MartinWitt commented May 15, 2020 •

edited

Loading

vmassol commented May 15, 2020

vmassol commented May 15, 2020

vmassol commented May 15, 2020

MartinWitt commented May 15, 2020

vmassol commented May 15, 2020

vmassol commented May 15, 2020 •

edited

Loading

vmassol commented May 15, 2020

MartinWitt commented May 15, 2020

vmassol commented May 15, 2020

vmassol commented May 15, 2020 •

edited

Loading

MartinWitt commented May 16, 2020 •

edited

Loading

MartinWitt commented May 18, 2020

vmassol commented May 18, 2020

MartinWitt commented May 18, 2020

vmassol commented May 18, 2020

MartinWitt commented May 18, 2020 •

edited

Loading

vmassol May 18, 2020

MartinWitt May 18, 2020

monperrus May 21, 2020

vmassol May 21, 2020

MartinWitt commented May 20, 2020 •

edited

Loading

monperrus commented May 21, 2020

MartinWitt commented May 21, 2020

monperrus commented May 22, 2020

MartinWitt commented May 22, 2020

vmassol commented May 22, 2020

monperrus commented May 22, 2020

monperrus commented May 22, 2020

vmassol commented May 22, 2020

fix(regression): add created source files to spoon input #70

fix(regression): add created source files to spoon input #70

Conversation

MartinWitt commented May 15, 2020 • edited Loading

MartinWitt commented May 15, 2020 • edited Loading

vmassol commented May 15, 2020

MartinWitt commented May 15, 2020 • edited Loading

vmassol commented May 15, 2020

vmassol commented May 15, 2020

vmassol commented May 15, 2020

MartinWitt commented May 15, 2020

vmassol commented May 15, 2020

vmassol commented May 15, 2020 • edited Loading

vmassol commented May 15, 2020

MartinWitt commented May 15, 2020

vmassol commented May 15, 2020

vmassol commented May 15, 2020 • edited Loading

MartinWitt commented May 16, 2020 • edited Loading

MartinWitt commented May 18, 2020

vmassol commented May 18, 2020

MartinWitt commented May 18, 2020

vmassol commented May 18, 2020

MartinWitt commented May 18, 2020 • edited Loading

vmassol May 18, 2020

Choose a reason for hiding this comment

MartinWitt May 18, 2020

Choose a reason for hiding this comment

monperrus May 21, 2020

Choose a reason for hiding this comment

vmassol May 21, 2020

Choose a reason for hiding this comment

MartinWitt commented May 20, 2020 • edited Loading

monperrus commented May 21, 2020

MartinWitt commented May 21, 2020

monperrus commented May 22, 2020

MartinWitt commented May 22, 2020

vmassol commented May 22, 2020

monperrus commented May 22, 2020

monperrus commented May 22, 2020

vmassol commented May 22, 2020

MartinWitt commented May 15, 2020 •

edited

Loading

MartinWitt commented May 15, 2020 •

edited

Loading

MartinWitt commented May 15, 2020 •

edited

Loading

vmassol commented May 15, 2020 •

edited

Loading

vmassol commented May 15, 2020 •

edited

Loading

MartinWitt commented May 16, 2020 •

edited

Loading

MartinWitt commented May 18, 2020 •

edited

Loading

MartinWitt commented May 20, 2020 •

edited

Loading