[CSV-303] Add revapi to fail build on breaking API changes

[spannm]

This pull request solves ticket CSV-303 by adding org.revapi:revapi-maven-plugin to the build section of the library's maven POM. Please note that config file revapi-config.json is added to exclude CSVFormat.serialVersionUID from checks. The field should probably be updated to something != 1 along with custom de-serialization for backward compatibility (see ticket CSV-302).

[garydgregory]

-1 We already do that using JApiCmp: See https://github.com/apache/commons-csv/blob/0fd5302689d7e366ad0ac398030f774f364756fb/pom.xml#L190

[spannm]

-1 We already do that using JApiCmp: See [..]

which failed to recognize the serialization bug you introduced in CSV-302

revapi is the best available tool to enforce api compatibility.

IMO you're being too hasty closing CSV-303.

[garydgregory]

Hello @sman-81 One way to maintain sanity when developing and maintaining the over 20 components that makeup Apache Commons is to centralize standard plugins in commons-parent. I really do not want to special case one component to use one API checking plugin when all other components use another. If the community wants to change from JApiCmp to RevAPI, that's fine, but let's attempt to maintain consistency for (my) sanity's sake (at least). I'll let others opine...

[kinow]

I searched for posts/tweets about revapi & jacimp. Found cases where one performed better than the other. But revapi appears to be under more active work.

@garydgregory do you think this could be something like jacoco & cobertura, where both are allowed in components, but with the configuration in the parent? Or would it be best to have a single library for all commons for consistency?

I don't have a preference, but if revapi is able to detect issues that escape jacimp, Id' be in favour of switching to revapi in the parent (after also checking with others in the mailing list).

[spannm]

-1 We already do that using JApiCmp: See

https://github.com/apache/commons-csv/blob/0fd5302689d7e366ad0ac398030f774f364756fb/pom.xml#L190

I don't think many developers use simply "mvn" to run Maven builds (I don't). I use mvn clean verify most of the time and expect essential build steps to be part of the configured build lifecycle. japicmp does not bind by default to any lifecycle phase and as there is no execution configured, it won't be executed in builds.

The configuration of defaultGoal is nice, but I am afraid most of us won't spot it unless pointed to it (e.g. in a readme). Shall we add a line to CONTRIBUTING.md? I also suggest to add an execution for japicmp-maven-plugin to the POM to bind goal cmp to phase verify. Wdyt?

          <execution>
            <phase>verify</phase>
            <goals>
              <goal>cmp</goal>
            </goals>
          </execution>

I am not much familiar with japicmp but I know revapi (log4j2 uses it btw). If japicmp does all that revapi does, then there is no need for revapi. If coverage is different, then the two can co-exist as @kinow suggested. I think @garydgregory is right, such configuration -if any- should go to commons-parent (impressive POM btw!). Fun fact: commons-parent has japicmp and clirr :)

[kinow]

I don't think many developers use simply "mvn" to run Maven builds (I don't). I use mvn clean verify most of the time and expect essential build steps to be part of the configured build lifecycle.

@sman-81 that's more of a convention that you might find in other Apache Commons components.

The configuration of defaultGoal is nice, but I am afraid most of us won't spot it unless pointed to it (e.g. in a readme). Shall we add a line to CONTRIBUTING.md?

Good idea, but I'm not sure if that's common to all components. The CONTRIBUTING.md file is auto-generated in some components during the release process - https://github.com/apache/commons-build-plugin/blob/master/src/main/resources/commons-xdoc-templates/contributing-md-template.md

So modifying that information would have to be in the commons-build-plugin, used by the parent component. But if that's not common to all components, then we'd have to think in some way to handle that… maybe just word it as something like “Some components have adopted a default Maven target in the pom.xml, etc.... You can also find how to build the project by looking at the CI files (GitHub Actions, Travis CI, etc.)....”

I think @garydgregory is right, such configuration -if any- should go to commons-parent (impressive POM btw!).

:tada:

Fun fact: commons-parent has japicmp and clirr :)

Yup, there's some room for choosing different plug-ins and libraries in the Commons components, when these are inherited from the parent, e.g. https://github.com/apache/commons-parent/blob/8ca044d50e8eb64cf841ad4c758c02d86b92f44f/pom.xml#L1483-L1508

@sman-81 let's wait for @garydgregory 's reply, and see if we will have to move the question about revapi to a place where other committers are subscribed—not everybody gets github notifications, and within ASF the mailing lists are normally the meeting-place :)

Thanks!

[aherbert]

A more typical approach for projects derived from Commons Parent would be to create:

src/site/resources/profile.japicmp

This will activate the JApiCmp profile in commons parent which binds the cmp goal to the verify phase. It also includes a reporting profile for the site build. So by doing it this way you get to collect all the configuration added to Commons Parent and adopt any changes as that evolves over time.

Note that CSV has a profile.jacoco but not one for japicmp. So you just copy the file and rename as appropriate.

FYI: Commons RNG uses both japicmp and revapi because neither tool is perfect. Using either one on their own would not work for that component. However each can be configured to ignore non-issues that the other tool does not identify; and used to spot real issues that the other tool does not identify.

(Note: edited to drop comments about clirr:check as I had an old version of CSV master. The latest default goal does use japicmp:cmp.)

[garydgregory]

Hi All:

The developer's mailing is a better place for this conversation to have better exposure.

There are a lot of plugins to configure for an Apache Commons component build. The goal of avoiding binary breakage is one of them. CLIRR still exists in the parent POM because not all 20+ components have migrated to JApiCmp, though, at this point, they are also likely behind in commons-parent versions and Java versions. Most if not all active components are now on Java 8 which CLIRR does not know about.

Step one would be to drop CLIRR from the POM. Step two would be to discuss if JApiCmp should stay or be replaced.

You usually and ideally run the following in a default build which most of our default goals do: Apache RAT, Checkstyle, Spotbugs, Javadoc, PMD, JApiCmp. That's how our GitHub builds validate PRs, using the default goal. We can tell devs "Run mvn on the command line to check your PR.", but nothing more complicated.

Using the default Maven goal is a no-brainer IMO: One line in the POM gives you the ability to say "mvn" on the command line, and in CI builds like our GitHub Actions builds. Arguing that adding 6 lines x 6 plugins instead of using the default Maven goal is just arguing for more noise and copypasta in the POM, and a POM is verbose enough as it is. In addition, you are now forcing more Maven black magic on the command line builds a dev runs. When something goes wrong or the build takes too long, I have to know more about Maven to turn off these plugins that insert themselves when I am trying to do something specific on the command line.

These are goals for me:

• A simplest command that runs everything: mvn
• The ability to run other Maven plugins without other plugins inserting themselves all over the place when I did not ask.

You can experiment with some broken Maven magic today when you see the same plugin run more than once like RAT, Javadoc, the compiler, that only a Maven master can explain away.

[spannm]

FYI: Commons RNG uses both japicmp and revapi because neither tool is perfect [..]

I agree with @aherbert's statement and think japicmp and revapi should be allowed to co-exist, and ideally do so in commons-parent. commons-rng (which has a custom revapi config much like the one suggested in my pr) and possibly other commons libraries would benefit from having a suitable configuration in one place: commons-parent

clirr should be dropped, it is hopelessly outdated, fully agree with @garydgregory. revapi should be added and configured in a way that resembles the japicmp config as much as possible (using a profile.revapi file etc.)

All projects that derive from commons-parent should upgrade to the latest released version sooner rather than later.

If this summarizes the points and you all are in agreement I will write a message on [email protected].