Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Build] Add a script to execute revapi without gradle plugin #10386

Closed
wants to merge 1 commit into from
Closed

[Build] Add a script to execute revapi without gradle plugin #10386

wants to merge 1 commit into from

Conversation

jbonofre
Copy link
Member

This closes #10368

@jbonofre
Copy link
Member Author

jbonofre commented May 27, 2024 •
edited
Loading

Some details:

  1. dev/revapi script checks the different project.
  2. I just create a simple gradle task to execute the script with gradle revapi
  3. I propose to include dev/revapi in the GitHub Action (as we do for rat), replacing the api-binary-compatibility.yml workflow
  4. I propose to have dev/revapi/.revapi-excludes to add "accepted breaks" and use it in the script
  5. As soon as we will merge this one, I will update Gradle version 😄

@Fokko @nastra thoughts ?

@jbonofre jbonofre force-pushed the GRADLE branch 2 times, most recently from aa8e8c0 to 94ea373 Compare May 27, 2024 07:13
nastra
nastra reviewed May 27, 2024
View reviewed changes
Fokko
Fokko reviewed May 28, 2024
View reviewed changes
Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jbonofre Does the script run clean on your end? When I run it, I'm seeing breaking changes:

cat lib/revapi.out
Analysis results
----------------

Old API: iceberg-api-1.5.2.jar
New API: iceberg-api-1.6.0-SNAPSHOT.jar
old: enum org.apache.iceberg.SnapshotRefType
new: enum org.apache.iceberg.SnapshotRefType
java.class.nonPublicPartOfAPI: Class 'org.apache.iceberg.SnapshotRefType' is indirectly included in the API (by the means of method return type for example) but the class is not accessible (neither public nor protected).
BINARY: NON_BREAKING, SEMANTIC: BREAKING, SOURCE: NON_BREAKING

old: class org.apache.iceberg.UnboundPartitionSpec.UnboundPartitionField
new: class org.apache.iceberg.UnboundPartitionSpec.UnboundPartitionField
java.class.nonPublicPartOfAPI: Class 'org.apache.iceberg.UnboundPartitionSpec.UnboundPartitionField' is indirectly included in the API (by the means of method return type for example) but the class is not accessible (neither public nor protected).
BINARY: NON_BREAKING, SEMANTIC: BREAKING, SOURCE: NON_BREAKING

old: interface org.apache.iceberg.expressions.BoundAggregate.Aggregator<R>
new: interface org.apache.iceberg.expressions.BoundAggregate.Aggregator<R>
java.class.nonPublicPartOfAPI: Class 'org.apache.iceberg.expressions.BoundAggregate.Aggregator<R>' is indirectly included in the API (by the means of method return type for example) but the class is not accessible (neither public nor protected).
BINARY: NON_BREAKING, SEMANTIC: BREAKING, SOURCE: NON_BREAKING

Analysis results
----------------

Old API: iceberg-core-1.5.2.jar
New API: iceberg-core-1.6.0-SNAPSHOT.jar
old: class org.apache.iceberg.DeleteFileIndex
new: class org.apache.iceberg.DeleteFileIndex
java.class.nonPublicPartOfAPI: Class 'org.apache.iceberg.DeleteFileIndex' is indirectly included in the API (by the means of method return type for example) but the class is not accessible (neither public nor protected).
BINARY: NON_BREAKING, SOURCE: NON_BREAKING, SEMANTIC: BREAKING

old: interface org.apache.iceberg.InheritableMetadata
new: interface org.apache.iceberg.InheritableMetadata
java.class.nonPublicPartOfAPI: Class 'org.apache.iceberg.InheritableMetadata' is indirectly included in the API (by the means of method return type for example) but the class is not accessible (neither public nor protected).
BINARY: NON_BREAKING, SOURCE: NON_BREAKING, SEMANTIC: BREAKING

old: interface org.apache.iceberg.ManifestEntry<F extends org.apache.iceberg.ContentFile<F>>
new: interface org.apache.iceberg.ManifestEntry<F extends org.apache.iceberg.ContentFile<F>>
java.class.nonPublicPartOfAPI: Class 'org.apache.iceberg.ManifestEntry<F extends org.apache.iceberg.ContentFile<F>>' is indirectly included in the API (by the means of method return type for example) but the class is not accessible (neither public nor protected).
BINARY: NON_BREAKING, SOURCE: NON_BREAKING, SEMANTIC: BREAKING

old: class org.apache.iceberg.TableScanContext
new: class org.apache.iceberg.TableScanContext
java.class.nonPublicPartOfAPI: Class 'org.apache.iceberg.TableScanContext' is indirectly included in the API (by the means of method return type for example) but the class is not accessible (neither public nor protected).
BINARY: NON_BREAKING, SOURCE: NON_BREAKING, SEMANTIC: BREAKING

old: interface org.apache.iceberg.encryption.KeyManagementClient
new: interface org.apache.iceberg.encryption.KeyManagementClient
java.class.nonPublicPartOfAPI: Class 'org.apache.iceberg.encryption.KeyManagementClient' is indirectly included in the API (by the means of method return type for example) but the class is not accessible (neither public nor protected).
BINARY: NON_BREAKING, SOURCE: NON_BREAKING, SEMANTIC: BREAKING

old: class org.apache.iceberg.inmemory.InMemoryCatalog.InMemoryViewOperations
new: class org.apache.iceberg.inmemory.InMemoryCatalog.InMemoryViewOperations
java.class.nonPublicPartOfAPI: Class 'org.apache.iceberg.inmemory.InMemoryCatalog.InMemoryViewOperations' is indirectly included in the API (by the means of method return type for example) but the class is not accessible (neither public nor protected).
BINARY: NON_BREAKING, SOURCE: NON_BREAKING, SEMANTIC: BREAKING

old: class org.apache.iceberg.io.ContentCache.CacheEntry
new: class org.apache.iceberg.io.ContentCache.CacheEntry
java.class.nonPublicPartOfAPI: Class 'org.apache.iceberg.io.ContentCache.CacheEntry' is indirectly included in the API (by the means of method return type for example) but the class is not accessible (neither public nor protected).
BINARY: NON_BREAKING, SOURCE: NON_BREAKING, SEMANTIC: BREAKING

old: class org.apache.iceberg.io.ContentCache.FileContent
new: class org.apache.iceberg.io.ContentCache.FileContent
java.class.nonPublicPartOfAPI: Class 'org.apache.iceberg.io.ContentCache.FileContent' is indirectly included in the API (by the means of method return type for example) but the class is not accessible (neither public nor protected).
BINARY: NON_BREAKING, SOURCE: NON_BREAKING, SEMANTIC: BREAKING

old: class org.apache.iceberg.rest.RESTSessionCatalog.RESTViewBuilder
new: class org.apache.iceberg.rest.RESTSessionCatalog.RESTViewBuilder
java.class.nonPublicPartOfAPI: Class 'org.apache.iceberg.rest.RESTSessionCatalog.RESTViewBuilder' is indirectly included in the API (by the means of method return type for example) but the class is not accessible (neither public nor protected).
BINARY: NON_BREAKING, SOURCE: NON_BREAKING, SEMANTIC: BREAKING

old: interface org.apache.iceberg.view.BaseViewHistoryEntry
new: interface org.apache.iceberg.view.BaseViewHistoryEntry
java.class.nonPublicPartOfAPI: Class 'org.apache.iceberg.view.BaseViewHistoryEntry' is indirectly included in the API (by the means of method return type for example) but the class is not accessible (neither public nor protected).
BINARY: NON_BREAKING, SOURCE: NON_BREAKING, SEMANTIC: BREAKING

old: interface org.apache.iceberg.view.BaseViewVersion
new: interface org.apache.iceberg.view.BaseViewVersion
java.class.nonPublicPartOfAPI: Class 'org.apache.iceberg.view.BaseViewVersion' is indirectly included in the API (by the means of method return type for example) but the class is not accessible (neither public nor protected).
BINARY: NON_BREAKING, SOURCE: NON_BREAKING, SEMANTIC: BREAKING

Analysis results
----------------

Old API: iceberg-parquet-1.5.2.jar
New API: iceberg-parquet-1.6.0-SNAPSHOT.jar
old: class org.apache.iceberg.parquet.ParquetCodecFactory
new: <none>
java.class.removed: Class was removed.
BINARY: BREAKING, SOURCE: BREAKING

old: class org.apache.iceberg.parquet.ParquetValueWriters.CollectionWriter<E>
new: class org.apache.iceberg.parquet.ParquetValueWriters.CollectionWriter<E>
java.class.nonPublicPartOfAPI: Class 'org.apache.iceberg.parquet.ParquetValueWriters.CollectionWriter<E>' is indirectly included in the API (by the means of method return type for example) but the class is not accessible (neither public nor protected).
SEMANTIC: BREAKING, BINARY: NON_BREAKING, SOURCE: NON_BREAKING

old: class org.apache.iceberg.parquet.ParquetValueWriters.MapWriter<K, V>
new: class org.apache.iceberg.parquet.ParquetValueWriters.MapWriter<K, V>
java.class.nonPublicPartOfAPI: Class 'org.apache.iceberg.parquet.ParquetValueWriters.MapWriter<K, V>' is indirectly included in the API (by the means of method return type for example) but the class is not accessible (neither public nor protected).
SEMANTIC: BREAKING, BINARY: NON_BREAKING, SOURCE: NON_BREAKING

old: class org.apache.iceberg.parquet.ParquetValueWriters.UnboxedWriter<T>
new: class org.apache.iceberg.parquet.ParquetValueWriters.UnboxedWriter<T>
java.class.nonPublicPartOfAPI: Class 'org.apache.iceberg.parquet.ParquetValueWriters.UnboxedWriter<T>' is indirectly included in the API (by the means of method return type for example) but the class is not accessible (neither public nor protected).
SEMANTIC: BREAKING, BINARY: NON_BREAKING, SOURCE: NON_BREAKING

old: interface org.apache.iceberg.parquet.TripleIterator<T>
new: interface org.apache.iceberg.parquet.TripleIterator<T>
java.class.nonPublicPartOfAPI: Class 'org.apache.iceberg.parquet.TripleIterator<T>' is indirectly included in the API (by the means of method return type for example) but the class is not accessible (neither public nor protected).
SEMANTIC: BREAKING, BINARY: NON_BREAKING, SOURCE: NON_BREAKING

@jbonofre
Copy link
Member Author

You can see it's not source breaking change. We just check source breaks (not semantic or binary).

@Fokko Fokko added this to the Iceberg 1.6.0 milestone May 28, 2024
@jbonofre
Copy link
Member Author

@Fokko I pushed a freemarker template to filter on only source breaking detected. Now, I will address the "accepted breaks" feature.

@jbonofre jbonofre force-pushed the GRADLE branch 5 times, most recently from 2c66921 to 10dc67f Compare May 30, 2024 12:26
@Fokko
Copy link
Contributor

Fokko commented May 31, 2024

@jbonofre I gave it another swing, looks good. One thing I noticed when I tried to trigger rev-API, it didn't report the breaking change. Example is in #10408 which might be helpful for debugging

Fokko
Fokko reviewed May 31, 2024
View reviewed changes
dev/revapi Outdated
printf "You do not have curl or wget installed, please download artifact manually.\n"
exit 1
fi
$REVAPI_HOME/revapi.sh --extensions=org.revapi:revapi-java:0.28.1,org.revapi:revapi-reporter-text:0.15.0 --old "$REVAPI_WORKING_DIR/iceberg-$REVAPI_PROJECT-$OLD_VERSION.jar" --new $REVAPI_PROJECT/build/libs/iceberg-$REVAPI_PROJECT*SNAPSHOT.jar -c dev/.revapi.json -Drevapi.java.missing-classes.behavior=ignore -Drevapi.reporter.text.minCriticality=error -Drevapi.reporter.text.append=true -Drevapi.reporter.text.output=$REVAPI_OUT -Drevapi.reporter.text.minSeverity=BREAKING -Drevapi.reporter.text.template=dev/.revapi.template
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a check to ensure that the snapshot files are there:

 ./dev/revapi         
rm: lib/revapi.report: No such file or directory
========================
Checking iceberg-api
Downloading iceberg-api 1.5.2
09:56:05.771 INFO  Downloading checked archives
Exception in thread "main" java.lang.IllegalArgumentException: New API files '/Users/fokkodriesprong/Desktop/iceberg/api/build/libs/iceberg-api*SNAPSHOT.jar' does not exist.
	at org.revapi.standalone.Main.checkCanRead(Main.java:508)
	at org.revapi.standalone.Main.convertPaths(Main.java:422)
	at org.revapi.standalone.Main.main(Main.java:272)
grep: lib/revapi.report: No such file or directory
========================
Checking iceberg-core
Downloading iceberg-core 1.5.2
09:56:06.360 INFO  Downloading checked archives
Exception in thread "main" java.lang.IllegalArgumentException: New API files '/Users/fokkodriesprong/Desktop/iceberg/core/build/libs/iceberg-core*SNAPSHOT.jar' does not exist.
	at org.revapi.standalone.Main.checkCanRead(Main.java:508)
	at org.revapi.standalone.Main.convertPaths(Main.java:422)
	at org.revapi.standalone.Main.main(Main.java:272)
grep: lib/revapi.report: No such file or directory
========================
Checking iceberg-parquet
Downloading iceberg-parquet 1.5.2
09:56:06.749 INFO  Downloading checked archives
Exception in thread "main" java.lang.IllegalArgumentException: New API files '/Users/fokkodriesprong/Desktop/iceberg/parquet/build/libs/iceberg-parquet*SNAPSHOT.jar' does not exist.
	at org.revapi.standalone.Main.checkCanRead(Main.java:508)
	at org.revapi.standalone.Main.convertPaths(Main.java:422)
	at org.revapi.standalone.Main.main(Main.java:272)
grep: lib/revapi.report: No such file or directory
========================
Checking iceberg-orc
Downloading iceberg-orc 1.5.2
09:56:07.145 INFO  Downloading checked archives
Exception in thread "main" java.lang.IllegalArgumentException: New API files '/Users/fokkodriesprong/Desktop/iceberg/orc/build/libs/iceberg-orc*SNAPSHOT.jar' does not exist.
	at org.revapi.standalone.Main.checkCanRead(Main.java:508)
	at org.revapi.standalone.Main.convertPaths(Main.java:422)
	at org.revapi.standalone.Main.main(Main.java:272)
grep: lib/revapi.report: No such file or directory
========================
Checking iceberg-common
Downloading iceberg-common 1.5.2
09:56:07.499 INFO  Downloading checked archives
Exception in thread "main" java.lang.IllegalArgumentException: New API files '/Users/fokkodriesprong/Desktop/iceberg/common/build/libs/iceberg-common*SNAPSHOT.jar' does not exist.
	at org.revapi.standalone.Main.checkCanRead(Main.java:508)
	at org.revapi.standalone.Main.convertPaths(Main.java:422)
	at org.revapi.standalone.Main.main(Main.java:272)
grep: lib/revapi.report: No such file or directory
========================
Checking iceberg-data
Downloading iceberg-data 1.5.2
09:56:07.861 INFO  Downloading checked archives
Exception in thread "main" java.lang.IllegalArgumentException: New API files '/Users/fokkodriesprong/Desktop/iceberg/data/build/libs/iceberg-data*SNAPSHOT.jar' does not exist.
	at org.revapi.standalone.Main.checkCanRead(Main.java:508)
	at org.revapi.standalone.Main.convertPaths(Main.java:422)
	at org.revapi.standalone.Main.main(Main.java:272)
grep: lib/revapi.report: No such file or directory
revapi check passed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Actually we should check both old and new artifact files. In your case, for instance, the problem is probably because you didn't build (at least with gradlew build -x test -x integrationTest) the artifacts before executing dev/revapi: the script expects the SNAPSHOT artifacts present.

Let me add a test to be sure about that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason, I'm unable to trigger the Rev-API: #10408. Also locally, I'm unable to reproduce it.

Let me do some investigation

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I talked with @nastra . In the mean time, and in order to move forward fast and clear, I'm working on a gradle revapi plugin fix. So, let's keep this PR as "fallback" but we can try to fix and release gradle revapi plugin (@nastra knows how to push a gradle plugin release).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me! Thanks for the update 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the script to check old/new artifacts. Can you please do a new run ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Fokko in the mean time, I'm checking #10408

@Fokko Fokko mentioned this pull request Jun 11, 2024
Closed
@jbonofre
Copy link
Member Author

@Fokko @nastra as we did a gradle-revapi fork and 1.8.1 release, I propose to remove 1.6.0 milestone from this PR.

@Fokko Fokko removed this from the Iceberg 1.6.0 milestone Jun 25, 2024
@Fokko
Copy link
Contributor

Fokko commented Jun 25, 2024

@jbonofre I agree, thanks again for working on this 👍

@Fokko
Copy link
Contributor

Fokko commented Jun 25, 2024

Closing this one since Gradle has been updated 👍

@Fokko Fokko closed this Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nastra nastra nastra left review comments

@Fokko Fokko Fokko left review comments

Assignees
No one assigned
Labels
build INFRA
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Run RevAPI without Gradle
3 participants
@jbonofre @Fokko @nastra