You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2022/08/19 17:45:03 UTC
[GitHub] [commons-csv] sman-81 opened a new pull request, #252: [CSV-303] Add revapi to fail build on breaking API changes
sman-81 opened a new pull request, #252:
URL: https://github.com/apache/commons-csv/pull/252
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] sman-81 commented on pull request #252: [CSV-303] Add revapi to fail build on breaking API changes
Posted by GitBox <gi...@apache.org>.
sman-81 commented on PR #252:
URL: https://github.com/apache/commons-csv/pull/252#issuecomment-1221113335
> -1 We already do that using JApiCmp: See [..]
which failed to recognize the serialization bug you introduced in [CSV-302](https://issues.apache.org/jira/browse/CSV-302)
revapi is the best available tool to enforce api compatibility.
IMO you're being too hasty closing [CSV-303](https://issues.apache.org/jira/browse/CSV-303).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] kinow commented on pull request #252: [CSV-303] Add revapi to fail build on breaking API changes
Posted by GitBox <gi...@apache.org>.
kinow commented on PR #252:
URL: https://github.com/apache/commons-csv/pull/252#issuecomment-1222119971
>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…
>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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory commented on pull request #252: [CSV-303] Add revapi to fail build on breaking API changes
Posted by GitBox <gi...@apache.org>.
garydgregory commented on PR #252:
URL: https://github.com/apache/commons-csv/pull/252#issuecomment-1221383364
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...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] sman-81 commented on pull request #252: [CSV-303] Add revapi to fail build on breaking API changes
Posted by GitBox <gi...@apache.org>.
sman-81 commented on PR #252:
URL: https://github.com/apache/commons-csv/pull/252#issuecomment-1221957534
> -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 :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] kinow commented on pull request #252: [CSV-303] Add revapi to fail build on breaking API changes
Posted by GitBox <gi...@apache.org>.
kinow commented on PR #252:
URL: https://github.com/apache/commons-csv/pull/252#issuecomment-1221623770
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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CSV-303] Add revapi to fail build on breaking API changes [commons-csv]
Posted by "spannm (via GitHub)" <gi...@apache.org>.
spannm closed pull request #252: [CSV-303] Add revapi to fail build on breaking API changes
URL: https://github.com/apache/commons-csv/pull/252
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] aherbert commented on pull request #252: [CSV-303] Add revapi to fail build on breaking API changes
Posted by GitBox <gi...@apache.org>.
aherbert commented on PR #252:
URL: https://github.com/apache/commons-csv/pull/252#issuecomment-1222190349
A more typical approach for projects derived from Commons Parent would be to create:
```
src/main/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.
CSV's default goal uses the clirr:check. This was OK when CSV was on an older version of Java but is not appropriate for new Java 8 features (the current build target). So switching to japicmp and dropping clirr seems to be the simplest solution rather than adding `revapi` will the goal of moving configuration to commons parent.
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory commented on pull request #252: [CSV-303] Add revapi to fail build on breaking API changes
Posted by GitBox <gi...@apache.org>.
garydgregory commented on PR #252:
URL: https://github.com/apache/commons-csv/pull/252#issuecomment-1222263959
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 build. 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory commented on pull request #252: [CSV-303] Add revapi to fail build on breaking API changes
Posted by GitBox <gi...@apache.org>.
garydgregory commented on PR #252:
URL: https://github.com/apache/commons-csv/pull/252#issuecomment-1221086726
-1 We already do that using JApiCmp: See https://github.com/apache/commons-csv/blob/0fd5302689d7e366ad0ac398030f774f364756fb/pom.xml#L190
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] sman-81 commented on pull request #252: [CSV-303] Add revapi to fail build on breaking API changes
Posted by GitBox <gi...@apache.org>.
sman-81 commented on PR #252:
URL: https://github.com/apache/commons-csv/pull/252#issuecomment-1228507019
> 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 _dev@commons.apache.org_.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org